gh-101525: Skip test_gdb if the binary is BOLTed. #118572
Conversation
corona10
commented
May 4, 2024
•
corona10
commented
- Issue: Make BOLT as stable feature #101525
|
Now the all tests are fully passed with BOLTed binary. |
Lib/test/support/__init__.py
Outdated
| 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.
Can you call readelf -e <binary_path> and grep .note.bolt_info? Checking headers is efficient and robust.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Why not checking sysconfig? This check looks slow and not reliable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe test:
'--enable-bolt' in sysconfig.get_config_var('CONFIG_ARGS')
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well then there is no way to display the optimization information but okay got it.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
; 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
There was a problem hiding this comment.
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?
| @@ -336,6 +336,7 @@ def get_build_info(): | |||
| if support.check_cflags_pgo(): | |||
| # PGO (--enable-optimizations) | |||
| optimizations.append('PGO') | |||
|
|
|||
There was a problem hiding this comment.
Since you added a function in test.support, can you add a "BOLT" case here?
corona10
changed the title
corona10
changed the title
|
By the way, since |
Well, when I run a test_gdb with BOLTed binary, sometimes it fails, well let me try with other systems too. |
|
I suggest to backport the change to 3.12, and maybe also to 3.11. |
| @@ -856,6 +856,14 @@ def check_cflags_pgo(): | |||
| return any(option in cflags_nodist for option in pgo_options) | |||
|
|
|||
|
|
|||
| def check_bolt_optimized(): | |||
| # Always return false, if the platform is WASI. | |||
There was a problem hiding this comment.
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 ;)
@vstinner @erlend-aasland |
