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

Set GC_DEBUG for debug builds of the interpreter #82618

Closed
pablogsal opened this issue Oct 10, 2019 · 6 comments
Closed

Set GC_DEBUG for debug builds of the interpreter #82618

pablogsal opened this issue Oct 10, 2019 · 6 comments
Labels
3.9 only security fixes

Comments

@pablogsal
Copy link
Member

BPO 38437
Nosy @tim-one, @vstinner, @pablogsal
PRs
  • bpo-38437: Activate GC_DEBUG when PY_DEBUG is set #16707
  • 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 = None
    closed_at = <Date 2019-10-10.21:49:20.915>
    created_at = <Date 2019-10-10.19:39:02.352>
    labels = ['3.9']
    title = 'Set GC_DEBUG for debug builds of the interpreter'
    updated_at = <Date 2019-10-10.21:49:32.342>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2019-10-10.21:49:32.342>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-10-10.21:49:20.915>
    closer = 'pablogsal'
    components = []
    creation = <Date 2019-10-10.19:39:02.352>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38437
    keywords = ['patch']
    message_count = 6.0
    messages = ['354401', '354403', '354405', '354406', '354408', '354409']
    nosy_count = 3.0
    nosy_names = ['tim.peters', 'vstinner', 'pablogsal']
    pr_nums = ['16707']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue38437'
    versions = ['Python 3.9']

    @pablogsal
    Copy link
    Member Author

    While working on bpo-38379 I had to manually set the GC_DEBUG macro to 1 to activate the extra checks that 'validate_list' does. These checks are super useful to make sure all the gc lists used are consistent and in the expected state with the expected masks.

    For this reason, I propose to always activate GC_DEBUG for debug builds of the interpreter. It will have a performance impact, but the debugging benefits are substantial.

    @pablogsal pablogsal added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels Oct 10, 2019
    @vstinner
    Copy link
    Member

    Under which condition can such list be corrupted?

    @pablogsal
    Copy link
    Member Author

    Under which condition can such list be corrupted?

    If someone is adding/modifiying the gc and calls any of the functions that set the gc flags like (PREV_MASK_COLLECTING).

    One example is that after calling move_unreachable(), the unreachable set has NEXT_MASK_UNREACHABLE and therefore is an invalid list (until move_legacy_finalizers removes the flag and makes it valid again). If you mess the order or violate any of the contract, the lists will be invalid. It helps a lot to know that you are not leaving the lists in invalid state by mistake or on the other hand, to check that they are valid when they are supposed to be valid.

    @tim-one
    Copy link
    Member

    tim-one commented Oct 10, 2019

    +1. This code got quite brittle when they decided to fit two pointers, a fat integer, and 3 flags into a struct with room for only the two pointers ;-) It's a mine field now. Enabling one of the few automated mine detectors is thoroughly sensible.

    @pablogsal
    Copy link
    Member Author

    New changeset 320dd50 by Pablo Galindo in branch 'master':
    bpo-38437: Activate GC_DEBUG when PY_DEBUG is set (GH-16707)
    320dd50

    @pablogsal
    Copy link
    Member Author

    Thanks Victor and Tim!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants