Skip to content

bpo-43760: Document PyThreadState.use_tracing removal #28527

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 23, 2021
Merged

bpo-43760: Document PyThreadState.use_tracing removal #28527

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
774d1c7
bpo-43760: Document PyThreadState.use_tracing removal
vstinner Sep 23, 2021
9ef0fcc
only document the removal
vstinner Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/whatsnew/3.10.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2194,6 +2194,11 @@ Porting to Python 3.10
limited API. The function is mainly useful for custom builds of Python.
(Contributed by Petr Viktorin in :issue:`26241`)

* The ``PyThreadState.use_tracing`` member has been removed to optimize Python:
``tstate->use_tracing`` must be replaced with
``tstate->cframe->use_tracing``.
Copy link
Member

Choose a reason for hiding this comment

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

This may be dangerous, state->cframe is a stack-allocated structure so I am not sure if is valid in all accesses of tstate->cframe and has the same semantics. Maybe @markshannon can clarify

Copy link
Member Author

Choose a reason for hiding this comment

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

Cython and greenlet now uses tstate->cframe->use_tracing. Do you mean that it's a bug?

Copy link
Member

@pablogsal pablogsal Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It may be. That is only valid as long as the executing frame is alive. For instance a for generators that may segfault

Actually, I am wrong, if tstate->cframe->use_tracing is not NULL then it must be valid.

The other question here is that tstate->cframe->use_tracing is still private, so I don't like recommending using it because it can change yet again.

Copy link
Member

Choose a reason for hiding this comment

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

This may be dangerous for two reasons:

  1. tstate->use_tracing had global effect before, while tstate->cframe->use_tracing only affects the last executing frame. Modifiying this value only affects that particular frame. The semantics are different.
  2. tstate->cframe points to a stack allocated variable in ceval. Accessing this from heap objects may be dangerous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum. I will design an API to abstract access to use_tracing. For now, I update my PR for the bare minimum: only document that the member is removed.

Copy link
Member

Choose a reason for hiding this comment

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

Reading from, or writing to, tstate->cframe->use_tracing is OK, what is dangerous is hanging on to a pointer to tstate->cframe. use_tracing must always be accessed through tstate (and with the GIL held).

However, it may change in the future, we should make it clear that this advice only applies to 3.10.

Copy link
Member

Choose a reason for hiding this comment

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

Reading from, or writing to, tstate->cframe->use_tracing is OK,

There is also change in semantics, no? before tstate->use_tracing had global effect but this only affects the current frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong, but AFAIR, it was used as a flag to disable tracing in the current frame. That would also motivate why it was moved out of the thread state now.

Copy link
Member

@pablogsal pablogsal Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, but it would have effect on the whole call stack. Imagine you set it to false, then all callers would stopped tracing when they get to run again after the callee returns. iIUC now that won't happen because you are only modifying the one in the frame running and all its callees

Copy link

Choose a reason for hiding this comment

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

Could setting the tracefunc to NULL be the alternative? There are some differences in 3.9 between setting use_trace to False and tracefunc to NULL, but I suspect the latter can be used. Keep in mind that this is implementation & version specific functionality so it's probably ok not to support all cases that 3.9 did.

(Contributed by Mark Shannon in :issue:`43760`.)

Deprecated
----------

Expand Down