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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Generally, this sounds reasonable, thanks!

Is the line and file is necessary because in Fortran you may have more than one module declaration per file? Out of curiosity, what does the debugger use the line/file info for?

In addition to the inline comments:
Please make sure to provide a bitcode/assembler roundtrip test, maybe in test/Assembler/dimodule.ll
I'm unsure that the metadataloader implementation is correct. Can you also add a bitcode upgrade test to test/Bitcode/ that shows that the older, shorter format is handled correctly?



================
Comment at: llvm/include/llvm/IR/DIBuilder.h:745
     /// \param APINotesFile The path to an API notes file for this module.
+    /// \param File        Source file where Fortran module is declared(Not
+    /// applicable otherwise).
----------------
` /// \param File        Source file of the module declaration. Used for Fortran modules.`


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:748
+    /// \param LineNo      Line number at which Fortran module is declared(Not
+    /// applicable otherwise).
     DIModule *createModule(DIScope *Scope, StringRef Name,
----------------
same here


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:752
+                           StringRef APINotesFile = {}, DIFile *File = {},
+                           unsigned LineNo = {});
 
----------------
This makes it quite opaque what the actual default value is.
`LineNo = 0`?


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2081
 
-/// A (clang) module that has been imported by the compile unit.
-///
+/// DIModlule can represent a clang module or a Fortran module.
+/// Fortran module representation can be used by a Fortran FE
----------------
`/// Represents a module in the programming  language, for example, a Clang module, or a Fortran module.`


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2139
   StringRef getAPINotesFile() const { return getStringOperand(4); }
+  DIFile *getFile() const { return cast_or_null<DIFile>(getRawFile()); }
+  unsigned getLineNo() const { return LineNo; }
----------------
The parent class DIScope already provides a `getFile()` method. Are we storing the file operand twice?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1430
+                         getMDString(Record[4]), getMDString(Record[5]),
+                         getMDOrNull(Record[6]), Record[7])),
         NextMetadataNo);
----------------
Will this crash when we get an input with only 6 records?

Is there a bitcode upgrade test that shows that the older, shorter format is handled gracefully?


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:849
+    return hash_combine(Scope, Name, ConfigurationMacros, IncludePath, File,
+                        LineNo);
   }
----------------
How likely is it going to be that you have two fortran modules with the same name but a different, file or lineno? Is it necessary for performance to hash the file/line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79484