Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the lltrace feature with the Py_Debug mode #69757

Open
matrixise opened this issue Nov 6, 2015 · 21 comments
Open

Improve the lltrace feature with the Py_Debug mode #69757

matrixise opened this issue Nov 6, 2015 · 21 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@matrixise
Copy link
Member

BPO 25571
Nosy @vstinner, @gwk, @matrixise, @Vgr255
Files
  • issue25571.patch
  • issue25571-2.diff
  • issue25571-3.diff
  • issue25571-4.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/vstinner'
    closed_at = None
    created_at = <Date 2015-11-06.21:13:45.991>
    labels = ['interpreter-core', 'type-feature']
    title = 'Improve the lltrace feature with the Py_Debug mode'
    updated_at = <Date 2017-07-23.13:02:34.149>
    user = 'https://github.com/matrixise'

    bugs.python.org fields:

    activity = <Date 2017-07-23.13:02:34.149>
    actor = 'vstinner'
    assignee = 'vstinner'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2015-11-06.21:13:45.991>
    creator = 'matrixise'
    dependencies = []
    files = ['40964', '43747', '43992', '43997']
    hgrepos = []
    issue_num = 25571
    keywords = ['patch']
    message_count = 20.0
    messages = ['254218', '254219', '255347', '270552', '270553', '270859', '270860', '270863', '270864', '271914', '271915', '271931', '271933', '278723', '294378', '294463', '298435', '298438', '298849', '298904']
    nosy_count = 6.0
    nosy_names = ['vstinner', 'akaptur', 'gwk', 'matrixise', 'xcombelle', 'abarry']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25571'
    versions = ['Python 3.6']

    @matrixise
    Copy link
    Member Author

    When we use the Py_Debug flag for the compilation, we can see the Bytecodes and the opcode arguments.

    Here is a small patch, it will show the label of the opcode in the ceval.c file.

    @matrixise
    Copy link
    Member Author

    Here is my patch with the improvement.

    @vstinner
    Copy link
    Member

    What is lltrace? I never used it :-( Is it documented? Can you give examples of output before/after?

    We may make your change optional to give the choice of the output.

    @matrixise
    Copy link
    Member Author

    Here is a small example of lltrace when you enable it.

    stephane@sg1 ~/s/h/cpython> ./python 
    Python 3.6.0a3+ (default:0d8f139a6e19+, Jul 16 2016, 11:59:46) 
    [GCC 6.1.1 20160621 (Red Hat 6.1.1-3)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> print
    <built-in function print>
    >>> print("hello")
    hello
    >>> __ltrace__ = None
    >>> print("hello")
    0: LOAD_NAME, 0
    push <built-in function print>
    2: LOAD_CONST, 0
    push 'hello'
    4: CALL_FUNCTION, 1
    ext_pop 'hello'
    hello
    ext_pop <built-in function print>
    push None
    6: PRINT_EXPR
    pop None
    8: LOAD_CONST, 1
    push None
    10: RETURN_VALUE
    pop None
    >>> del __ltrace__
    0: DELETE_NAME, 0
    2: LOAD_CONST, 0
    push None
    4: RETURN_VALUE
    pop None
    >>> print("hello")
    hello
    >>> print("hello")
    

    @matrixise
    Copy link
    Member Author

    here is the patch, if you want to test it, just use the REPL and add __ltrace__ = None in the REPL.

    @matrixise matrixise added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jul 19, 2016
    @xcombelle
    Copy link
    Mannequin

    xcombelle mannequin commented Jul 20, 2016

    I made some comment on code in review.

    A thing that worry me that there is zero automated test.

    In my opinion the minimal should be to test the expected output.

    A nice to have would be a test for the write_contents function of makeopcodetranslations.py

    @matrixise
    Copy link
    Member Author

    Totally agree with you, I am going to check that, because currently,
    there is no test with this "hidden" feature in Python.

    I am going to discuss with Victor about this point.

    Thanks,

    Stephane

    On 07/20, Xavier Combelle wrote:

    Xavier Combelle added the comment:

    I made some comment on code in review.

    A thing that worry me that there is zero automated test.

    In my opinion the minimal should be to test the expected output.

    A nice to have would be a test for the write_contents function of makeopcodetranslations.py

    ----------
    nosy: +xcombelle


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue25571\>



    Python-bugs-list mailing list
    Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/stephane%40wirtel.be

    @vstinner
    Copy link
    Member

    It would be nice to have unit tests and docs :-)

    For unit test, you can use a script like:
    ---

    def func():
        return 1
    
    __ltrace__ = True
    func()

    Run the script with test.support.assert_python_ok(), and check stdout.

    To skip the test if Python is compiled in released mode, you can use:
    ---

    # Were we compiled --with-pydebug or with #define Py_DEBUG?
    Py_DEBUG = hasattr(sys, 'gettotalrefcount')

    ...
    @unittest.skipUnless(Py_DEBUG, 'need Py_DEBUG')
    ---

    @vstinner
    Copy link
    Member

    Sorry, I don't know what is the best place to add new unit tests.

    I don't know neither what is the best place to document the __ltrace__ feature. Maybe somewhere near:

    Or maybe:

    @matrixise
    Copy link
    Member Author

    Here is a small patch for the __ltrace__ feature.

    I would like to propose a new test, __ltrace__ should be defined in the globals() dictionary and there is no check on the value associated to this key. Maybe we could defined it as a boolean, True or False.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 3, 2016

    Left some comments on Rietveld; mostly documentation-related (and pointed out some typos).

    I'm also +1 on having to specify a True value rather than just defining the variable at all (i.e. setting __ltrace__ to False is the same as not defining it). If you change this, I'd probably force the value to be an actual bool, i.e. True or False (much like __debug__; except that it's read-only and a syntax error to assign to it).

    @Vgr255 Vgr255 mannequin added the type-feature A feature request or enhancement label Aug 3, 2016
    @matrixise
    Copy link
    Member Author

    Here is a new version of my patch "bpo-25571-4.diff" with the comments of Emanuel Barry.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 4, 2016

    Maybe someone who knows this feature better can weigh in, but otherwise LGTM.

    @matrixise
    Copy link
    Member Author

    I have to update my patch to python-3.7, I will provide a patch asap.

    @matrixise
    Copy link
    Member Author

    @Haypo, you told me there is an alternative to my patch, provided by an other dev. what's the bpo issue for the alternative.

    @vstinner
    Copy link
    Member

    @gwk
    Copy link
    Mannequin

    gwk mannequin commented Jul 16, 2017

    @matrixise, I'm the author of the alternative in bpo-29400, and I'm finally finding the time to get back into it. I'm going to make a push this week to clean it up; your feedback would be much appreciated!

    @vstinner
    Copy link
    Member

    I still strongly prefer bpo-29400 over "lltrace", since it would be usable in release mode.

    @matrixise
    Copy link
    Member Author

    @gwk no problem for a review of your patch

    @Haypo as discussed before, maybe we could remove the lltrace feature because this one is never used by the developers.

    or we could ask on @python-dev ML.

    If there is a better solution and this one could replace the current lltrace, I am ok with that.

    @vstinner
    Copy link
    Member

    I suggest to wait until sys.settrace() supports bytecode level tracing,
    then rewrite lltrace on top of it, open a new issue to remove C lltrace and
    close this issue. You need to write to python-dev if you want to remove the
    current C lltrace.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @sweeneyde
    Copy link
    Member

    I opened issue #91462 and PR #91463, which are similar to some of the patches here.

    I see lltrace as a useful feature for debugging the python compiler and bytecode interpreter, or what could be useful if the output was more understandable. sys.settrace and tracing systems based on it are nice for debugging Python code, but I think debugging the interpreter itself is a different use-case with different requirements.

    Say I implement some new Python feature that relies on a new opcode, but I unknowingly have an off-by-one error that makes the code accidentally skip over a POP_TOP. I wind up with a segfault on some assert(EMPTY()). But that information isn't very helpful, so I enable lltrace (whether that's by setting __ltrace__ in the appropriate module or manually setting lltrace = 1 in the c code). Then I can re-build the interpreter and see that just before the crash, there's a RETURN_VALUE instead of a POP_TOP, and which function that occurred in. Any additional printf statements I add to ceval.c will be put in context by the surrounding lltrace output, which is nice as well. The feature is especially helpful if the error occurs during a _bootstrap_python phase of the build process, where the failure can be rather opaque, and could occur before a C debugger can be attached conveniently.

    I want something dead simple and transparent and understandable at the c level, as if I had just littered the code with my own printf statements, but better. So I would actually prefer that the feature not be pluggable. If I'm touching something involving how python functions themselves are called, I want print statements that completely sidestep all of that.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants