Fix macOS version identification and improve multiple-section support #745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes benfred#726, probably. This could use some testing. On some builds of Python on macOS, such as my Homebrew build of Python 3.13, the version ends up in the section `__DATA,__common` instead of `__DATA,__bss`. This difference appears to be caused by the use of ThinLTO. I'm not sure exactly why LLVM does that, but for py-spy's purposes, it suffices to scan both sections. Implement this by having `Binary` store a `Vec` of `(addr, size)` pairs instead of just one `bss_addr` and `bss_size`. While I'm at it: - Put the pyruntime section bounds into the same `Vec` rather than keeping it as a separate set of fields. This was already treated like another bss section. - Change all three executable-format parsing routines so that duplicate sections all go into the `Vec`, instead of them having to pick one BSS section. - Since we are now scanning more sections, there will be more harmless "didn't find anything" errors. Therefore, differentiate the cases by changing the return type of `Version::scan_bytes` from `Result<X, Error>` to `Result<Option<X>, Error>`. "Didn't find anything" now results in `Ok(None)`, which the callers silently ignore, but the callers `warn!` on all other errors. - Improve error handling of `check_interpreter_address` along similar lines. It now returns an iterator of `Result`s, which leaves it to the caller to decide how to expose the error from each individual address. The callers expose them either as `warn!` (if `_PyRuntime` was found and we really expect this address to be valid) or just `debug!` (if we're brute-forcing addresses). Maybe this one was overkill... In brute-force mode, it does waste time formatting errors that will never be shown most of the time, but in practice this shouldn't have noticeable impact: the number of addresses being brute-forced isn't _that_ large.
|
Solves |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #726, probably.
This could use some testing.
On some builds of Python on macOS, such as my Homebrew build of Python 3.13, the version ends up in the section
__DATA,__commoninstead of__DATA,__bss. This difference appears to be caused by the use of ThinLTO. I'm not sure exactly why LLVM does that, but for py-spy's purposes, it suffices to scan both sections.Implement this by having
Binarystore aVecof(addr, size)pairs instead of just onebss_addrandbss_size.While I'm at it:
Put the pyruntime section bounds into the same
Vecrather than keeping it as a separate set of fields. This was already treated like another bss section.Change all three executable-format parsing routines so that duplicate sections all go into the
Vec, instead of them having to pick one BSS section.Since we are now scanning more sections, there will be more harmless "didn't find anything" errors. Therefore, differentiate the cases by changing the return type of
Version::scan_bytesfromResult<X, Error>toResult<Option<X>, Error>. "Didn't find anything" now results inOk(None), which the callers silently ignore, but the callerswarn!on all other errors.Improve error handling of
check_interpreter_addressalong similar lines. It now returns an iterator ofResults, which leaves it to the caller to decide how to expose the error from each individual address. The callers expose them either aswarn!(if_PyRuntimewas found and we really expect this address to be valid) or justdebug!(if we're brute-forcing addresses).Maybe this one was overkill... In brute-force mode, it does waste time formatting errors that will never be shown most of the time, but in practice this shouldn't have noticeable impact: the number of addresses being brute-forced isn't that large.