gh-148037: remove critical section from PyCode_Addr2Line#148103
gh-148037: remove critical section from PyCode_Addr2Line#148103kumaraditya303 wants to merge 1 commit intopython:mainfrom
PyCode_Addr2Line#148103Conversation
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
Two comments below.
An additional thought: in light of things like https://discuss.python.org/t/pure-c-structured-traceback/106872, maybe we should just use atomic operations everywhere for these fields (i.e., _Py_atomic instead of FT_ATOMIC) because PyCode_Addr2Lineis called from _Py_DumpTraceback, which may be called from a signal handler or without holding the GIL.
| } | ||
| if (co->_co_monitoring && co->_co_monitoring->lines) { | ||
| if (FT_ATOMIC_LOAD_PTR_ACQUIRE(co->_co_monitoring) && | ||
| FT_ATOMIC_LOAD_PTR_ACQUIRE(co->_co_monitoring->lines)) { |
There was a problem hiding this comment.
I think this sort of reloading of atomic variables is error prone. Structure this like:
_PyCoMonitoringData *data = FT_ATOMIC_LOAD_PTR_ACQUIRE(code->_co_monitoring);
if (data) {
_PyCoLineInstrumentationData *lines = FT_ATOMIC_LOAD_PTR_ACQUIRE(data->lines);
if (lines) {
...
}
}and pass the load line data to _Py_Instrumentation_GetLine.
| return -1; | ||
| } | ||
| initialize_lines(code, bytes_per_entry); | ||
| initialize_lines(lines, code, bytes_per_entry); |
There was a problem hiding this comment.
Minor, and up to you, but when reading this, I think it would be clearer for the atomic store to _co_monitoring->lines to be here instead of inside initialize_lines
2 participants
TODO: check that no path inside
initialize_linesexpects lines to be non-NULL.