[PATCH] D146776: [llvm] Preliminary fat-lto-objects support

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

In D146776#4435885 <https://reviews.llvm.org/D146776#4435885>, @MaskRay wrote:

> Thanks for the update. This fixes `-DBUILD_SHARED_LIBS=on` build of mine.
>
> Perhaps `llvm/test/Bitcode/` tests (which don't normally test `-passes=`) should be moved to elsewhere, e.g. `llvm/test/Transforms/EmbedBitcodePass/` ?

Oh, yes. I should have moved those too. I'll take care of that in the next version

In D146776#4435915 <https://reviews.llvm.org/D146776#4435915>, @MaskRay wrote:

> In D146776#4435885 <https://reviews.llvm.org/D146776#4435885>, @MaskRay wrote:
>
>> Thanks for the update. This fixes `-DBUILD_SHARED_LIBS=on` build of mine.
>>
>> Perhaps `llvm/test/Bitcode/` tests (which don't normally test `-passes=`) should be moved to elsewhere, e.g. `llvm/test/Transforms/EmbedBitcodePass/` ?
>>
>> Another strange thing to fix:
>>
>> `llc a.ll` => `.type   .Lllvm.embedded.object, at object`
>>
>> We don't normally set attributes on temporary symbols (`.L`), which do not normally have symbol table entries anyway.
>
> This is due to `private` but I think this is acceptable. Both GNU assembler and LLVM integrated assembler do not create a symbol table entry, so this is fine. This likely does not justify adding a special case for `private` lowering to the underlying ELF section.

I'm not sure I follow the conversation here. Is this an issue w/ the defaults used by `embedBufferInModule()`? or is the oddity just related to the symol being private?

> Feel free to land this one with a `readelf -S` test to check `SHF_EXCLUDE`. Maybe give some time for the Clang patch. Landing this one shall make reviewing the Clang part easier.

Will do. Thanks for all the feedback.



================
Comment at: llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp:9
+//
+// EmbedBitcodePass implementation.
+//
----------------
MaskRay wrote:
> If `EmbedBitcodePass.h` provides a descriptive comment, this comment can probably be removed. It does not give additional information to users:)
noted. I'll expand the description in the header and remove this. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146776/new/

https://reviews.llvm.org/D146776