Page MenuHomePhabricator

[gold] Ignore bitcode from sections inside object files
ClosedPublic

Authored by tstellar on Jan 11 2022, 12:02 AM.

Details

Summary

-fembed-bitcode will put bitcode into special sections within object
files, but this is not meant to be used by LTO, so the gold plugin
should ignore it.

https://github.com/llvm/llvm-project/issues/47216

Diff Detail

Unit TestsFailed

TimeTest
60,050 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
60,030 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
2,320 msx64 debian > libFuzzer.libFuzzer::merge_two_step.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/FullCoverageSetTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/merge_two_step.test.tmp-FullCoverageSetTest

Event Timeline

tstellar requested review of this revision.Jan 11 2022, 12:02 AM
tstellar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 12:02 AM

A few comments and questions.

First, it looks like lld works because it has a similar check guarding its call of InputFile::create (which is embedded in the BitcodeFile constructor).

Note though I am seeing inconsistent info online as to what embedded bitcode is meant to be used for. It would be good to document this somewhere, like where fembed-bitcode is documented (which I am not finding). The initial submitter text when bcsection.ll was added suggests it was intended to be useful for LTO (https://reviews.llvm.org/D4371). I did find this recent discussion which indicates it was added for Apple recompilation: https://groups.google.com/g/llvm-dev/c/d1WSRvLefHQ/m/Iwm39FboAAAJ. I also found official ARM documentation online that points to the embedded .llvmbc section as being there to support LTO: https://developer.arm.com/documentation/dui0773/i/sbu1510240545830.

But in any case, as noted above, lld does not support its use in LTO links, so I think this is just in need of clear documentation somewhere.

Can you add a test?

If bcsection.ll is deleted, its input file bcsection.s should as well.

A few comments and questions.

First, it looks like lld works because it has a similar check guarding its call of InputFile::create (which is embedded in the BitcodeFile constructor).

Note though I am seeing inconsistent info online as to what embedded bitcode is meant to be used for. It would be good to document this somewhere, like where fembed-bitcode is documented (which I am not finding). The initial submitter text when bcsection.ll was added suggests it was intended to be useful for LTO (https://reviews.llvm.org/D4371). I did find this recent discussion which indicates it was added for Apple recompilation: https://groups.google.com/g/llvm-dev/c/d1WSRvLefHQ/m/Iwm39FboAAAJ. I also found official ARM documentation online that points to the embedded .llvmbc section as being there to support LTO: https://developer.arm.com/documentation/dui0773/i/sbu1510240545830.

This discussion from rust is also relevant: https://github.com/rust-lang/rust/issues/90326

But in any case, as noted above, lld does not support its use in LTO links, so I think this is just in need of clear documentation somewhere.

Can you add a test?

If bcsection.ll is deleted, its input file bcsection.s should as well.

tstellar updated this revision to Diff 399097.Jan 11 2022, 3:21 PM

Add test case.

The documentation here: https://llvm.org/docs/BitCodeFormat.html#native-object-file-wrapper-format also says that the section is useful for LTO.

As you can see from the linked issue, there are some cases where adding the .llvmbc section causes ld.bfd to fail to link, when it was able to link successfully without the section. I originally tried to fix the failures in the plugin, but after reading the comments on https://github.com/rust-lang/rust/issues/90326 it seemed like it might be better to just ignore these sections if they aren't intended to be use this way.

@jyknight You commented here that -fembed-bitcode was not intended for LTO, but the llvm documentation says the .llvmbc section is useful for LTO. Do you have any more background on the history of the .llvmbc section and do you think the gold plugin should try to use it for LTO?

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 7:30 PM

-fembed-bitcode was originally added by Apple to enable them to recompile
3rd-party-developers' software with a newer compiler, or for new CPU
targets, without the source code. It uses the normal non-LTO optimization
pipeline, and emits bitcode corresponding to the binary object also
emitted.

For LTO bitcode generation, we use a different pass manager configuration.

That said, various other people seem to have had various other ideas about
how things should work over time. So, I couldn't say conclusively that
nobody will complain if we have gold not try to parse these sections...but
it seems potentially reasonable to me.

tstellar updated this revision to Diff 430179.May 17 2022, 1:45 PM

Add documentation about the change.

MaskRay accepted this revision.EditedMay 18 2022, 12:19 AM

-fembed-bitcode was originally added by Apple to enable them to recompile
3rd-party-developers' software with a newer compiler, or for new CPU
targets, without the source code. It uses the normal non-LTO optimization
pipeline, and emits bitcode corresponding to the binary object also
emitted.

For LTO bitcode generation, we use a different pass manager configuration.

That said, various other people seem to have had various other ideas about
how things should work over time. So, I couldn't say conclusively that
nobody will complain if we have gold not try to parse these sections...but
it seems potentially reasonable to me.

I agree. I think for Mach-O the section is for LTO use. For ELF there are two usage:

llvm/docs/ReleaseNotes.rst
170
llvm/tools/gold/gold-plugin.cpp
540

special sections => .llvmbc

Be specific.

541

"not meant for LTO use " when using LLVMgold.so.

For llgo(?) it was for LTO use: https://reviews.llvm.org/D4371

This revision is now accepted and ready to land.May 18 2022, 12:19 AM
tstellar updated this revision to Diff 430734.May 19 2022, 10:16 AM
tstellar marked 3 inline comments as done.

Update comments and documentation.

@tejohnson Are you OK with this change?

tstellar updated this revision to Diff 430846.May 19 2022, 4:17 PM

Updated documentation in BitCodeFormat.rst.

@tejohnson Do the updates to the BitCodeFormat document look OK now?

@tejohnson Do the updates to the BitCodeFormat document look OK now?

Yep, thanks!

MaskRay accepted this revision.Jun 21 2022, 9:48 PM
MaskRay added inline comments.
llvm/docs/ReleaseNotes.rst
170

The LLVM gold plugin now ignores bitcode ...

tstellar updated this revision to Diff 438911.Jun 21 2022, 10:23 PM

Updated relase notes.

tstellar marked an inline comment as done.Jun 23 2022, 11:00 AM
MaskRay accepted this revision.Jun 26 2022, 11:07 AM
MaskRay added inline comments.
llvm/docs/GoldPlugin.rst
20
tstellar updated this revision to Diff 440150.Jun 27 2022, 3:18 AM

Fix documentation formatting.

tstellar marked an inline comment as done.Jun 27 2022, 3:19 AM
This revision was landed with ongoing or failed builds.Jul 14 2022, 2:46 PM
This revision was automatically updated to reflect the committed changes.