[PATCH] D117244: [Bitcode] [ThinLTO] Add a new bitcode module record for THINLTO_INDEX_FLAG

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/tools/llvm-dis/llvm-dis.cpp:194-204
       BitcodeLTOInfo LTOInfo = ExitOnErr(MB.getLTOInfo());
+
+      std::unique_ptr<Module> M;
+      if ((!DumpThinLTOIndexOnly) && (!LTOInfo.OnlyHasThinLTOIndex)) {
+        M = ExitOnErr(
+            MB.getLazyModule(Context, MaterializeMetadata, SetImporting));
+        if (MaterializeMetadata)
----------------
luna wrote:
> luna wrote:
> > (Just a message before fixing the failed test)
> > 
> > In pre-merge test, `llvm-dis` parsing behavior changed for a bitcode that's invalid itself; the behavior changed from parse error to crash (test expects `llvm-dis` to return parse error while now `llvm-dis` crashes).
> > 
> > After a quick test, this is caused by the change of read order here;
> > - before this patch, `getLazyModule` will exit-on-err so `getLTOInfo` won't be executed
> > - after this patch, `getLTOInfo` is executed first, and crashes upon invalid abbrev ID around https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Bitstream/BitstreamReader.h#L528
> > 
> > I'll see how to solve this without changing test expectation from `not` to `crash` (i.e., `llvm-dis` still reports error rather than crash upon invalid bitcode).
> > 
> > The relevant checker line in `invalid.test` is `/var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-dis -disable-output /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Bitcode/Inputs/invalid-pointer-element-type.bc `
> To add more context on `getLazyModule` and `getLTOInfo` and  regarding how a bitcode is read 
>       a) `getLazyModule` will invoke `BitcodeReader::parseModule`, which parses each subblock and exit-on-err when invalid pointer is read [1]; this is also where `invalid record` is returned when non-index-reader is used to parse minimized lto code.
> 
>       b) `getLTOInfo` [2] has the equivalent loop to iterate blocks and subblocks, but skips uninteresting subblocks. As a result the invalid block `TYPE_BLOCK_ID_NEW` is read in a skipping fashion (while it's detected if parsed), so the read action right after `TYPE_BLOCK_ID_NEW` is in a sense undefined and triggered `invalid abbrev id` [3] in this case.
> 
> This means, `llvm-dis` gives more clear error log by parsing first and tell which section is not correct; if `llvm-dis` tries to read LTO info before parsing, and reading LTO info doesn't parse, the error log is less helpful than error log from parsing.
> 
> On the other hand, if a wrong reader is used during parsing, `llvm-dis` gives false alarms (e.g., `llvm-dis` cannot read minimized thinlto bitcode now)
> 
> From this perspective, it seems the right for `llvm-dis` to first parse and detect error; two options at the top of my head to solve this
> 
>   -In `getLazyModule`, use the read-and-fail approach (try common reader first, fallback to the other reader, and returns error if the fallback doesn't work); the index-flag in module block is probably not necessary in this option to solve `llvm-dis` issue.
>    
>   - Add another flavor of `getLTOInfo` that does parsing; but the problem of choosing a right reader still exists.
> 
> Maybe I should book a chat to discuss this further, but this patch is not in a hurry and not in the critical path anyway.
> 
> btw verified the loop to read module block is the same between `getLTOInfo` and `getLazyModule` implementation by printing names out (`getLazyModule` returns error on the 3rd entry when parsing it, `getLTOInfo` skips 3rd entry and fails on the 4th one).
> 
> [1] https://github.com/llvm/llvm-project/blob/d2cc6c2d0c2f8a6e272110416a3fd579ed5a3ac1/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L3657
> [2] https://github.com/llvm/llvm-project/blob/d2cc6c2d0c2f8a6e272110416a3fd579ed5a3ac1/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L6982
> [3] https://github.com/llvm/llvm-project/blob/6a1f50b84ae8f8a8087fcdbe5f27dae8c76878f1/llvm/include/llvm/Bitstream/BitstreamReader.h#L528
> 
> 
> 
Hmm, I see the issue. It is unfortunate to lose the more specific error message. Let me think about this some more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117244