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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:747
+    ///                    Fortran modules.
+    /// \param LineNo      Source line no. of the  module declaration.
+    ///                    Used for Fortran modules.
----------------
`    /// \param LineNo      Source line number of the module declaration.`


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1431
+                         Record.size() == 6 ? nullptr : getMDString(Record[6]),
+                         Record.size() == 6 ? 0 : Record[7])),
         NextMetadataNo);
----------------
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


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

https://reviews.llvm.org/D79484