Opened 7 years ago

Closed 7 years ago

#13868 closed enhancement (fixed)

Deal with hooked tp_* functions when using a debug build of Python

Reported by: jpflori Owned by: GeorgSWeber
Priority: major Milestone: sage-5.6
Component: build Keywords: hook debug
Cc: vbraun, SimonKing, jdemeyer Merged in: sage-5.6.beta3
Authors: Volker Braun Reviewers: Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jpflori)

Python debug checks are confused by the fast tp_* functions we hook in integer.pyx and real_double.pyx

Apply trac_13868_tp_hooks_and_debug-typo.patch.

Attachments (2)

trac_13868_tp_hooks_and_debug.patch (9.9 KB) - added by vbraun 7 years ago.
Initial patch
trac_13868_tp_hooks_and_debug-typo.patch (9.9 KB) - added by jpflori 7 years ago.
Initial patch + typo fixed.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by vbraun

In fact, thats commented out:

        # This line is only needed if Python is compiled in debugging
        # mode './configure --with-pydebug'. If that is the case a Python
        # object has a bunch of debugging fields which are initialized
        # with this macro. For speed reasons, we don't call it if Python
        # is not compiled in debug mode. So uncomment the following line
        # if you are debugging Python.

        #PyObject_INIT(new, (<RichPyObject*>global_dummy_Integer).ob_type)

comment:2 Changed 7 years ago by jpflori

I tried to uncomment both these lines in integer.pyx and real_double.pyx but this lead to segfaults when starting Sage.

If I prevent the use of the fast tp_*, Sage starts (although I get a TypeError? in sage/libs/gen/late_import, I've not recompiled everything after rebuilding Python)

So this will need more work.

comment:3 follow-up: Changed 7 years ago by vbraun

I found the bug, the PyObject_INIT was only called when a new integer / real object was created and not when a recycled object from the pool was used. You have to call it in both branches, of course. I'll work out a fix where the debug preprocessor macro is used to conditionally compile it in.

Changed 7 years ago by vbraun

Initial patch

comment:4 in reply to: ↑ 3 Changed 7 years ago by SimonKing

Replying to vbraun:

I found the bug, the PyObject_INIT was only called when a new integer / real object was created and not when a recycled object from the pool was used. You have to call it in both branches, of course. I'll work out a fix where the debug preprocessor macro is used to conditionally compile it in.

I am a bit puzzled. Why is this only done when the debug preprocessor macro is used? Do you claim that this is a bug only in debug mode? Why could it not create trouble without debug mode?

comment:5 follow-up: Changed 7 years ago by SimonKing

Anyway, using your patch plus the new cython plus the latest version of the python spkg in debug mode, I am now able to start Sage without a crash. It "only" reports

ImportError: /home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.so: undefined symbol: _Z7_p_TestP8spolyrecP9sip_sringi

But on #13867 you said that for this one needs a new Singular spkg (hopefully one that still allows for using xalloc!).

comment:6 in reply to: ↑ 5 Changed 7 years ago by SimonKing

Replying to SimonKing:

But on #13867 you said that for this one needs a new Singular spkg

Found it! #13876, right?

comment:7 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

Yes!

comment:8 Changed 7 years ago by SimonKing

With the patches listed at #13877, I get:

/home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/sage/all.py:76: RuntimeWarning: tp_compare didn't return -1 or -2 for exception
  import sage.symbolic.pynac
---------------------------------------------------------------------------
SystemError                               Traceback (most recent call last)

/home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/IPython/ipmaker.py in force_import(modname, force_reload)
     61         reload(sys.modules[modname])
     62     else:
---> 63         __import__(modname)
     64         
     65 

/home/simon/SAGE/debug/sage-5.5.rc0/local/bin/ipy_profile_sage.py in <module>()
      5     preparser(True)
      6     
----> 7     import sage.all_cmdline
      8     sage.all_cmdline._init_cmdline(globals())
      9     

/home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/sage/all_cmdline.py in <module>()
     12 try:
     13 
---> 14     from sage.all import *
     15     from sage.calculus.predefined import x
     16     preparser(on=True)

/home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/sage/all.py in <module>()
     74 
     75 # This must come before Calculus -- it initializes the Pynac library.
---> 76 import sage.symbolic.pynac
     77 
     78 from sage.modules.all    import *

SystemError: Objects/object.c:854: bad argument to internal function
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

So, tp_compare does something unexpected here.

comment:9 Changed 7 years ago by vbraun

Did you compile everything from scratch? You definitely need to recompile pynac with debugging, maybe more?

comment:10 Changed 7 years ago by SimonKing

I did sage -ba after installing all stuff on top of an existing sage-5.5.

comment:11 follow-up: Changed 7 years ago by vbraun

If you start on sage-5.5 you at least need the patch from #13740, maybe more...

comment:12 in reply to: ↑ 11 Changed 7 years ago by SimonKing

Replying to vbraun:

If you start on sage-5.5 you at least need the patch from #13740, maybe more...

I of course have that patch, since I have the new cython spkg from #13832.

comment:13 Changed 7 years ago by jdemeyer

I don't know whether indented preprocessor directives like

#if ...
    #define ...
#endif

are standard C. Maybe they are (GCC accepts them and documents this), but usually this is written as

#if ...
#define ...
#endif
Last edited 7 years ago by jdemeyer (previous) (diff)

comment:14 Changed 7 years ago by vbraun

Pre-ANSI C did not allow for space, but those compilers are long dead (even by skynet standards). Its legal in all C versions that will at least start building Sage. I prefer the Cython-style indentation, then you don't have to mentally switch when reading it.

comment:15 follow-up: Changed 7 years ago by vbraun

It would be nice if we could review and merge this ticket as it is the cause for the segfault during Sage "make" with SAGE_DEBUG=yes

comment:16 in reply to: ↑ 15 Changed 7 years ago by SimonKing

Replying to vbraun:

It would be nice if we could review and merge this ticket as it is the cause for the segfault during Sage "make" with SAGE_DEBUG=yes

Is this still the case? With all the new spkgs mentioned in the ticket description of #13864 (in particular, with cython-0.17.4), sage builds and starts fine on bsd.math, with SAGE_DEBUG=yes. Or has that only been a problem on other systems?

Changed 7 years ago by jpflori

Initial patch + typo fixed.

comment:17 Changed 7 years ago by jpflori

  • Description modified (diff)
  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

Works fine and seems correct to me. I just added a missing u in some comment.

comment:18 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.