[PATCH] D79484: [DebugInfo] Fortran module DebugInfo support in LLVM

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 3 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1431
+                         Record.size() == 6 ? nullptr : getMDString(Record[6]),
+                         Record.size() == 6 ? 0 : Record[7])),
         NextMetadataNo);
----------------
aprantl wrote:
> SouraVX wrote:
> > aprantl wrote:
> > > You could still attack the loader with malformed input here:
> > > 
> > > `if (Record.size() < 5 || Record.size() > 8)`
> > > 
> > > means we accept a size of 6, 7, or 8. That means that
> > > 
> > > `                         Record.size() == 6 ? 0 : Record[7])),`
> > > 
> > > will access `Record[7]` if the size is 7.
> > > 
> > > I would use `Record.size() <= 6 ? nullptr : ...` and `Record.size() <= 7 ? 0 : ...`, respectively.
> > Thanks! for the suggestion. Code seems more robust and consistent now.
> There  is one last thing that I still don't understand:
> 
> The File is metadata operand 0 (= bitcode Record[1]), and the BitcodeWriter writes out the operands in order (as it should). How does this pass the bitcode upgrade test? Did you remember to create the .bc file with the unmodified llvm-as? Shouldn't the have to reader take into account that we inserted a field in the beginning and thus shift all records by one?
> 
> Maybe I'm missing something, but I would expect that we need something like:
> 
> ```
> unsigned Offset = Record.size() >= 7 ? 1 : 2;
> 
> Record[1+Offset], Record[2+Offset], ...
> ```
> 
> here
Thanks for pointing this out!
That was indeed the problem, test case `.bc` file `DIModule-clang-module.ll.bc` was previously generated using modified( i.e after this patch)`llvm-as`, that's why the issue doesn't surfaced.

In this revision, this file is generated using unmodified(i.e before this patch) `llvm-as`, and `llvm-dis` is able to correctly disassemble it.


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

https://reviews.llvm.org/D79484