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

gh-111178: fix UBSan failures in Python/traceback.c #128259

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Dec 25, 2024

{NULL} /* Sentinel */
{"tb_next", tb_next_get, tb_next_set, NULL, NULL},
{"tb_lineno", tb_lineno_get, NULL, NULL, NULL},
{NULL, NULL, NULL, NULL, NULL} /* Sentinel */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I was under the impression that a NULL name indicated the sentinel--nothing should be necessary after it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking no, but this would leave fields non-initialized. Considering that the UBSan PRs are meant to avoid compiler issues (also, I think it's always better to initialize fields when possible). But I can revert those changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've thought about it. Since we're moving towards removing un-necessary fields in static structures, I think it's better to just have a single NULL for a sentinel (ideally, a macro would be better so that we don't need to indicate that XXX is a sentinel but let's keep the diff smaller). I'll also update my other PRs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking no, but this would leave fields non-initialized.

I'm pretty sure partial initialization in C will leave the other fields as zero (or NULL here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants