[PATCH] D30377: [DebugInfo] [DWARFv5] Support for DW_AT_calling_convention for types

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks for working on this!

- There also needs to be a bitcode upgrade test (using a .bc file generated by current LLVM trunk)
- There also needs to be an IR llvm-as | llvm-dis | llvm-as | ... roundtrip test
- This needs to be added to the metadata unit tests.



================
Comment at: include/llvm/IR/DIBuilder.h:312
     /// \param TemplateParms Template type parameters.
     /// \param UniqueIdentifier A unique identifier for the class.
     DICompositeType *createClassType(
----------------
Please document the new argument.


================
Comment at: include/llvm/IR/DIBuilder.h:319
+        StringRef UniqueIdentifier = "",
+        dwarf::CallingConvention ArgABI = dwarf::DW_CC_normal);
 
----------------
Maybe call this CallingConvention?


================
Comment at: include/llvm/IR/DIBuilder.h:331
     /// \param RunTimeLang  Optional parameter, Objective-C runtime version.
     /// \param UniqueIdentifier A unique identifier for the struct.
     DICompositeType *createStructType(
----------------
comment


================
Comment at: include/llvm/IR/DIBuilder.h:349
     /// \param RunTimeLang  Optional parameter, Objective-C runtime version.
     /// \param UniqueIdentifier A unique identifier for the union.
     DICompositeType *createUnionType(DIScope *Scope, StringRef Name,
----------------
comment


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1130
   case bitc::METADATA_COMPOSITE_TYPE: {
-    if (Record.size() != 16)
+    if (Record.size() < 16)
       return error("Invalid record");
----------------
Since there are only 2 bits necessary to encode the CC, we should just use some bits in Record[0] for this and save some space.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1500
   Record.push_back(VE.getMetadataOrNullID(N->getRawIdentifier()));
+  Record.push_back(N->getArgABI());
 
----------------
See my comment in MetadataLoader.cpp


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:973
+    // Add DW_AT_calling_convention attr.
+    if (DD->getDwarfVersion() >= 5) {
+      auto ArgABI = CTy->getArgABI();
----------------
Technically true, but I think there is no harm in emitting this attribute also in DWARF 4 or earlier. We usually only do this if the new attribute cannot be safely ignored by an older consumer.


https://reviews.llvm.org/D30377