GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@corona10

@corona10 corona10 commented May 4, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Make BOLT as stable feature #101525

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@corona10
corona10 requested review from erlend-aasland and vstinner May 4, 2024 13:57
@bedevere-app bedevere-app Bot added the awaiting core review label May 4, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 4, 2024
Make BOLT as stable feature #101525
Open
5 of 7 tasks
@corona10

corona10 commented May 4, 2024

Copy link
Copy Markdown
Member Author

Now the all tests are fully passed with BOLTed binary.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aaupov
aaupov reviewed May 5, 2024
View reviewed changes
Comment thread 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:

@aaupov aaupov May 5, 2024

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@corona10 corona10 May 5, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner May 5, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@corona10 corona10 May 5, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner May 5, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner May 5, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@corona10 corona10 May 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland May 6, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@corona10 corona10 May 6, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland May 6, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vstinner
vstinner reviewed May 6, 2024
View reviewed changes
Comment thread Lib/test/libregrtest/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
corona10 requested a review from vstinner May 6, 2024 13:33
@corona10 corona10 changed the title gh-101525: Skip test_gdb if the binary is BOLTed. [WIP] gh-101525: Skip test_gdb if the binary is BOLTed. May 6, 2024
@corona10 corona10 changed the title [WIP] gh-101525: Skip test_gdb if the binary is BOLTed. gh-101525: Skip test_gdb if the binary is BOLTed. May 6, 2024
@aaupov

aaupov commented May 6, 2024

Copy link
Copy Markdown

By the way, since -update-debug-sections is used (https://github.com/python/cpython/blob/main/configure.ac#L2185), do you really need to skip gdb tests?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented May 6, 2024

Copy link
Copy Markdown
Member Author

By the way, since -update-debug-sections is used (https://github.com/python/cpython/blob/main/configure.ac#L2185), do you really need to skip gdb tests?

Well, when I run a test_gdb with BOLTed binary, sometimes it fails, well let me try with other systems too.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner approved these changes May 7, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 7, 2024
@vstinner

vstinner commented May 7, 2024

Copy link
Copy Markdown
Member

I suggest to backport the change to 3.12, and maybe also to 3.11.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed May 7, 2024
View reviewed changes
Comment thread Lib/test/support/__init__.py Outdated


def check_bolt_optimized():
# Always return false, if the platform is WASI.

@erlend-aasland erlend-aasland May 7, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@corona10

corona10 commented May 8, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I suggest to backport the change to 3.12, and maybe also to 3.11.

@vstinner @erlend-aasland
BOLT is the experimental feature from 3.12 :)
Anyway, I will finalize this PR during the PyCon US. This week, I am too busy with all my tasks at my full-time job before the conference.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 added 9 commits September 1, 2024 01:48
@corona10
pythongh-101525: Skip test_gdb if the binary is BOLTed.
8755316
@corona10
Address code review
973abd6
@corona10
nit
83d879c
@corona10
Address code review
af9c391
@corona10
Address code review
e9ec9b9
@corona10
fix
2d6d05a
@corona10
nit
b854706
@corona10
fix
c0bd894
@corona10
Address code review
ea13917
@corona10
corona10 force-pushed the gh-101525-test branch from d77cc23 to ea13917 Compare August 31, 2024 16:55
@corona10

corona10 commented Aug 31, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@vstinner @erlend-aasland I've updated the staled PR, Can you please take a look again?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10 corona10 added the needs backport to 3.12 only security fixes label Aug 31, 2024
@corona10 corona10 added the needs backport to 3.13 only security fixes label Aug 31, 2024
vstinner
vstinner approved these changes Sep 2, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Misc/NEWS.d/next/Tests/2024-05-04-22-56-41.gh-issue-101525.LHK166.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
Address code review
0e9d019
corona10
corona10 commented Sep 2, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Tests/2024-05-04-22-56-41.gh-issue-101525.LHK166.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
corona10 enabled auto-merge (squash) September 2, 2024 13:01
@corona10 corona10 changed the title gh-101525: Skip test_gdb if the binary is BOLTed. gh-101525: Skip test_gdb if the binary is relocated by BOLT. Sep 2, 2024
@corona10
corona10 disabled auto-merge September 2, 2024 13:02
@corona10
corona10 enabled auto-merge (squash) September 2, 2024 13:02
vstinner
vstinner approved these changes Sep 2, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@corona10
corona10 merged commit f95fc4d into python:main Sep 2, 2024
@miss-islington-app

miss-islington-app Bot commented Sep 2, 2024

Copy link
Copy Markdown

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 2, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 2, 2024
@corona10 @miss-islington
pythongh-101525: Skip test_gdb if the binary is relocated by BOLT. (p…
da1b359
…ythongh-118572)

(cherry picked from commit f95fc4d)

Co-authored-by: Donghee Na <[email protected]>
@miss-islington-app

miss-islington-app Bot commented Sep 2, 2024

Copy link
Copy Markdown

Sorry, @corona10, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f95fc4de115ae03d7aa6dece678240df085cb4f6 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned corona10 Sep 2, 2024
@bedevere-app

bedevere-app Bot commented Sep 2, 2024

Copy link
Copy Markdown

GH-123601 is a backport of this pull request to the 3.13 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Sep 2, 2024
corona10 added a commit that referenced this pull request Sep 2, 2024
@miss-islington @corona10
[3.13] gh-101525: Skip test_gdb if the binary is relocated by BOLT. (g…
3b3a1a8
…h-118572) (#123601)

gh-101525: Skip test_gdb if the binary is relocated by BOLT. (gh-118572)
(cherry picked from commit f95fc4d)

Co-authored-by: Donghee Na <[email protected]>
corona10 added a commit to corona10/cpython that referenced this pull request Sep 2, 2024
@corona10
[3.12] pythongh-101525: Skip test_gdb if the binary is relocated by B…
1ba223c
…OLT. (pythongh-118572)

(cherry picked from commit f95fc4d)

Co-authored-by: Donghee Na <[email protected]>
@corona10 corona10 mentioned this pull request Sep 2, 2024
[3.12] gh-101525: Skip test_gdb if the binary is relocated by BOLT. (… #123603
Merged
@bedevere-app

bedevere-app Bot commented Sep 2, 2024

Copy link
Copy Markdown

GH-123603 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Sep 2, 2024
corona10 added a commit that referenced this pull request Sep 2, 2024
@corona10
[3.12] gh-101525: Skip test_gdb if the binary is relocated by BOLT. (… (
4514998
#123603)

[3.12] gh-101525: Skip test_gdb if the binary is relocated by BOLT. (gh-118572)
(cherry picked from commit f95fc4d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@vstinner vstinner vstinner approved these changes
+1 more reviewer
@aaupov aaupov aaupov left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@corona10 corona10

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@corona10 @aaupov @vstinner @picnixz @erlend-aasland
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.