GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-101525: Skip test_gdb if the binary is relocated by BOLT. by corona10 · Pull Request #118572 · python/cpython Conversation
|
|
Now the all tests are fully passed with BOLTed binary. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| binary_path = sys.executable | ||
| with open(binary_path, 'rb') as f: | ||
| binary = f.read() | ||
| if b'.note.bolt_info' in binary: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can you call readelf -e <binary_path> and grep .note.bolt_info? Checking headers is efficient and robust.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I also think it will be the best way to handle this issue. But I am not sure it will be okay to call external binary for this. I may need to get opinions from @vstinner and @erlend-aasland WDYT?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why not checking sysconfig? This check looks slow and not reliable.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why not checking sysconfig? This check looks slow and not reliable.
Do you have any recommendation points? CFLAG does not guarantee the build is BOLTed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe test:
'--enable-bolt' in sysconfig.get_config_var('CONFIG_ARGS')
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Only put the code in test_gdb if you're worried that the the test is not reliable. And add a comment to explain it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Well then there is no way to display the optimization information but okay got it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You cannot expect "readelf" to be available, it would be a new dependency. I dislike looking for a string in a binary without parsing the ELF binary.
We call external binaries in test.support to determine macOS features, so there is precedent for doing that sort of thing; is readelf a standalone package, or is it part of the compiler toolchain?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
; is readelf a standalone package, or is it part of the compiler toolchain?
AFAIK, it's part of binutils: https://sourceware.org/binutils/docs/binutils/readelf.html
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, it's part of binutils: https://sourceware.org/binutils/docs/binutils/readelf.html
Thanks. binutils is preinstalled on the Ubuntu runners GitHub provides, so there is no more dependencies for the CI there. Also, anyone who wants to build their own CPython probably already did apt install build-essential, which includes binutils. Looks to me like there are no more deps introduced. What about macOS?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
By the way, since |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Well, when I run a test_gdb with BOLTed binary, sometimes it fails, well let me try with other systems too. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I suggest to backport the change to 3.12, and maybe also to 3.11. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
|
|
||
| def check_bolt_optimized(): | ||
| # Always return false, if the platform is WASI. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This comment only says what the code does, but not why. Could you rewrite it to state why this is happening? I can easily see by the code that we always return false if we're on WASI; there is no need to manifest that info into a comment ;)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
@vstinner @erlend-aasland |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
d77cc23 to
ea13917
Compare
There was an error while loading. Please reload this page.
|
@vstinner @erlend-aasland I've updated the staled PR, Can you please take a look again? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…ythongh-118572) (cherry picked from commit f95fc4d) Co-authored-by: Donghee Na <[email protected]>
|
Sorry, @corona10, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-123601 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…OLT. (pythongh-118572) (cherry picked from commit f95fc4d) Co-authored-by: Donghee Na <[email protected]>
|
GH-123603 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.