[PATCH] D86847: [Bitcode] Add BITCODE_SIZE_BLOCK_ID to encode the size of the bitcode

[Sean Bartell via Phabricator via llvm-commits]

bartell added a comment.

> In D86847#2248516 <https://reviews.llvm.org/D86847#2248516>, @dblaikie wrote:
>
>> @bartell mentioned something about a bitcode wrapper that does provide the length - how does this patch's strategy compare to that approach? Are they redundant? Should the wrapper be replaced with this patch's approach? (or should the bitcode wrapper approach be used instead of adding this patch?)
>
> I guess that is an implementation choice. This approach prefers a simple linker implementation, while the bitcode wrapper requires linker know to treat this section differently and write bitcode wrapper into section.
> @MaskRay Do you have any specific use case in mind for this? If you are tied to a linker like `lld`, it might be better just teach `lld` to treat this section differently so we don't need worry about concatenated bitcode file.

The linker wouldn't need to know about the wrapper. We would just change Clang to always emit the wrapper in .llvmbc.

The "bitcode wrapper" I'm referring to is the simple header written by emitDarwinBCHeaderAndTrailer <https://reviews.llvm.org/source/llvm-github/browse/master/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp;7be86829216698b6f4d0e083d48d7095898fc9c8$4399> and read by SkipBitcodeWrapperHeader <https://reviews.llvm.org/source/llvm-github/browse/master/llvm/include/llvm/Bitcode/BitcodeReader.h;7be86829216698b6f4d0e083d48d7095898fc9c8$247>. It's currently only written for bitcode files that use Darwin/Mach-O targets, but it should be read successfully regardless of target.

Rather than adding new fields to the bitcode itself as this patch does, I would recommend using the "bitcode wrapper" instead, just for the sake of reusing code. We could either enable it for all uses of WriteBitcodeToFile, or just use it in Clang when generating the .llvmbc section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86847