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-128563: Move lltrace into the frame struct #129113

Merged
merged 15 commits into from
Jan 21, 2025

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jan 21, 2025

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

The change to the size of the frame can be avoided by using a bitfield.

I'm curious why the atomic loads and stores are needed.

Include/internal/pycore_frame.h Show resolved Hide resolved
Lib/test/test_sys.py Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Jan 21, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@Fidget-Spinner
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Jan 21, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon January 21, 2025 10:43
@@ -76,7 +76,12 @@ typedef struct _PyInterpreterFrame {
_PyStackRef *stackpointer;
uint16_t return_offset; /* Only relevant during a function call */
char owner;
#ifdef Py_DEBUG
char visited:4;
char lltrace:4;
Copy link
Member

@markshannon markshannon Jan 21, 2025

Choose a reason for hiding this comment

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

Why 4?
You only need one bit (although it is best to use uint8_t as the signedness of char is implementation defined.

My mistake, lltrace is not boolean.

Since char might be signed, it is best to make this a uint8_t to avoid overflow for PYTHON_LLTRACE=9

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

One small issue, otherwise looks good.

lltrace should be unsigned (uint8_t), and maybe bigger, to avoid overflow. visited only needs one bit, so lltrace could use the rest.
For consistency, make visited a uint8_t in the non-debug build as well.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Looks good.

@Fidget-Spinner Fidget-Spinner merged commit 5809b25 into python:main Jan 21, 2025
59 of 60 checks passed
@Fidget-Spinner Fidget-Spinner deleted the move_lltrace_to_frame branch January 21, 2025 14:17
@chris-eibl
Copy link

Sorry for being late here. These lines seem to be unused, since the PR did not have to touch them?

cpython/Python/ceval.c

Lines 1066 to 1071 in 05d12ee

#ifdef Py_DEBUG
#define DPRINTF(level, ...) \
if (lltrace >= (level)) { printf(__VA_ARGS__); }
#else
#define DPRINTF(level, ...)
#endif

@Fidget-Spinner
Copy link
Member Author

@chris-eibl it seems so. If you would like to, could you please open a PR to either update to fix them or remove them? Thanks!

@chris-eibl
Copy link

Yeah, but since this is gonna be my first PR, I'll have to work myself through the devguide. Most probably not before the weekend ...

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.

3 participants