Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BOLT] Fix LSDA section handling #71821

Merged
merged 1 commit into from
Nov 15, 2023
Merged

[BOLT] Fix LSDA section handling #71821

merged 1 commit into from
Nov 15, 2023

Conversation

yota9
Copy link
Member

@yota9 yota9 commented Nov 9, 2023

Currently BOLT finds LSDA secition by it's name .gcc_except_table.main .
But sometimes it might have suffix e.g. .gcc_except_table.main. Find
LSDA section by it's address, rather by it's name.
Fixes #71804

@yota9
Copy link
Member Author

yota9 commented Nov 14, 2023

Genle ping

Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

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

Code looks good but I would drop the test case, because it makes us depend on a compiler that generates a section named .gcc_except_table.main. If you really want to check that a binary has a specific section name, and you are using a linker script, just create the section with that name in your linker script:

.gcc_except_table.main : { *(.gcc_except_table*) }

@yota9
Copy link
Member Author

yota9 commented Nov 15, 2023

@rafaelauler Thanks! But that is why I've checked that the initial binary has this section, if this behaviour would be changed in clang somebody would have to fix it before submitting this :) From time to time we have the tests that rely on the compiler behaviour, maybe more than needed.. But I don't see a reason not to add the string to the link script, so I would do it. Thanks again!

Currently BOLT finds LSDA secition by it's name .gcc_except_table.main .
But sometimes it might have suffix e.g. .gcc_except_table.main. Find
LSDA section by it's address, rather by it's name.
Fixes llvm#71804
@yota9 yota9 merged commit c5a306f into llvm:main Nov 15, 2023
3 checks passed
Comment on lines +4 to +5
// RUN: %clang++ %cxxflags -O3 -flto=thin -no-pie -c %s -o %t.o
// RUN: %clang++ %cxxflags -flto=thin -no-pie -fuse-ld=lld %t.o -o %t.exe \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ThinLTO flag needed for this test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently definitely not since I've added @rafaelauler offer to the ld script at least (I'm not sure does the problem has any connection to lto by default, probably not). I've just re-used the test provided in #71804 . Does it raise some problem here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve had an issue on a system where lld’s version didn’t match that of the compiler. Uncommon scenario, but nevertheless it’s better to fix.

zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Currently BOLT finds LSDA secition by it's name .gcc_except_table.main .
But sometimes it might have suffix e.g. .gcc_except_table.main. Find
LSDA section by it's address, rather by it's name.
Fixes llvm#71804
@@ -0,0 +1,7 @@
SECTIONS {
.text : { *(.text*) }
Copy link
Member

Choose a reason for hiding this comment

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

lld creates a PF_R PT_LOAD. For best portability, the first section of each segment should be specified. I created #93763

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BOLT] Assertion `!HasError && "Cannot get value when an error exists!
4 participants