This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Add BinaryContext::IsStripped
ClosedPublic

Authored by nhuhuan on Jul 18 2022, 12:02 PM.

Details

Summary

Determine stripped status of a binary based on .symtab

Test Plan:

ninja check-bolt

Diff Detail

Event Timeline

nhuhuan created this revision.Jul 18 2022, 12:02 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ayermolo. · View Herald Transcript
nhuhuan requested review of this revision.Jul 18 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 12:02 PM
Amir accepted this revision.Jul 18 2022, 12:08 PM

LGTM, but please drop "[NFC]" from the title.
@maksfb: can you please take a look if it makes sense to you?

This revision is now accepted and ready to land.Jul 18 2022, 12:08 PM

LGTM, also simple test would be appreciated (e.g. by adding BOLT-INFO about the presumption that the binary was stripped).

Can you add a test?

nhuhuan updated this revision to Diff 445747.Jul 19 2022, 2:28 AM

Print strip status. Add a test.

nhuhuan retitled this revision from [BOLT][NFC] Add BinaryContext::IsStripped to [BOLT] Add BinaryContext::IsStripped.Jul 19 2022, 2:30 AM
Amir requested changes to this revision.Jul 27 2022, 11:11 PM

I'm requesting changes because the user-facing message needs to be updated. Please also update the test accordingly.

bolt/lib/Rewrite/RewriteInstance.cpp
1640
bolt/test/X86/is-strip.s
2 ↗(On Diff #445747)

Please update the summary.

6 ↗(On Diff #445747)

I would prefer reusing some existing input source that doesn't require system-linux to make this test as general as possible.

This revision now requires changes to proceed.Jul 27 2022, 11:11 PM
nhuhuan updated this revision to Diff 448439.Jul 28 2022, 2:30 PM

Address feedbacks.

nhuhuan marked 3 inline comments as done.Jul 28 2022, 2:31 PM
Amir added inline comments.Jul 28 2022, 2:34 PM
bolt/lib/Rewrite/RewriteInstance.cpp
1640

clang-format? (I can't make the right format from the inline comment box)

nhuhuan updated this revision to Diff 448445.Jul 28 2022, 2:45 PM

Clang-formatted.

nhuhuan marked an inline comment as done.Jul 28 2022, 2:46 PM
Amir accepted this revision.Jul 28 2022, 11:08 PM
This revision is now accepted and ready to land.Jul 28 2022, 11:08 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 18 2022, 8:18 AM
thakis added inline comments.
bolt/test/X86/is-strip.s
2 ↗(On Diff #445747)

This doesn't pass on systems were the system linker isn't an ELF linker, e.g. macOS:

+ : 'RUN: at line 3'
+ /Users/thakis/src/llvm-build-runtimes2/bin/clang++ /Users/thakis/src/llvm-project/bolt/test/X86/Inputs/linenumber.cpp -o /Users/thakis/src/llvm-build-runtimes2/tools/bolt/test/X86/Output/is-strip.s.tmp -Wl,-q
ld: unknown option: -q
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

This seems to fix it:

-# RUN: %clang++ %p/Inputs/linenumber.cpp -o %t -Wl,-q
+# RUN: %clang++ %cflags %p/Inputs/linenumber.cpp -o %t -Wl,-q

Is that the right fix?

thakis added inline comments.Nov 18 2022, 8:37 AM
bolt/test/X86/is-strip.s
2 ↗(On Diff #445747)

Many other tests pass %cflags too at least, so that's probably correct. Uploaded D138306 for this.