DIBuilder support DI Macro creation

[aaboud]

Summary

Added DI macro creation API to DIBuilder.
This API will be used by Clang to create the DIMacro and DIMacroFile metadata entities.

Note: Usage of these new interface functions can be found in D16135.

[aprantl]

include/llvm/IR/DIBuilder.h

730 ↗

(On Diff #44537)

What's an example of a self-referential F?

[aaboud]

Applied changes according to Adrian comment.

include/llvm/IR/DIBuilder.h

730 ↗

(On Diff #44537)

I think there is no such example, it is a copy paste issue.
Thanks for noticing, I will fix the comment and the code.

[aprantl]

IIRC DIBuilder has a couple of unit tests (unittests/IR/IRBuilderTest.cpp). Could you add tests for the new functionality?

include/llvm/IR/DIBuilder.h

49 ↗

(On Diff #44717)

What are the allowed types of parents?

124 ↗

(On Diff #44717)

Instead of passing a bool, which leads to call sites like
/* undef */ true
would it make sense to have inline convenience wrappers like "createMacroDefine/Undef" or passing in the DW_MACINFO_undef value directly?

125 ↗

(On Diff #44717)

This is usually written as = StringRef()

[aaboud]

Added unit test that covers the macro new DIBuilder create API.
Applied more fixes according to Adrian comments.

[aaboud]

In D16077#325861, @aprantl wrote:

IIRC DIBuilder has a couple of unit tests (unittests/IR/IRBuilderTest.cpp). Could you add tests for the new functionality?

Done.

include/llvm/IR/DIBuilder.h

49 ↗

(On Diff #44717)

Added a comment explaining the types.
I still have to use "Metadata *" as "MDTuple::get()" function expect a vector of "Metadata *"!

124 ↗

(On Diff #44717)

Changed the code to pass DW_MACINFO_define and DW_MACINFO_undef.

[aprantl]

Just one final question: Why is replaceArrays() necessary? We need it for DICompositeType because composite types may be recursive. Is that also the case for DIMacroNodeArray?

[aaboud]

In D16077#332403, @aprantl wrote:

Just one final question: Why is replaceArrays() necessary? We need it for DICompositeType because composite types may be recursive. Is that also the case for DIMacroNodeArray?

The reason why DIMacroFile need the replaceArrays() function is that when we create the DIMacroFile, we still do not have the list of children nodes of that DIE.
So, we create an empty one,,and later, when we finalize the DIBuilder and all Macro DIEs are already created, only then we can fix the children list of the DIMacroFile and replace the empty list with the actual children list.

If I understand your concern well, you are suggesting that instead of the "replaceArrays()" function, we implement a "DIMacroFile::replaceMacros()" similar to the function of the DICompileUnit, correct?

[aaboud]

Updated patch to top of trunk (r276746) - Thanks to Ranjeet Singh.

Please, provide your feedback.

[aprantl]

Yes, I think putting it on DIMacroFile might be a better interface, but I'm not sure.
But then there is also another issue: I'm not sure whether it is legal to modify a uniquable (non-distinct) DINode because it will break uniquing. So I think DIMacroFiles must be created distinct to avoid this problem. Would this cause an issue during LTO with duplication or are they expected to be mostly unique across compile units anyway?

[aaboud]

In D16077#498128, @aprantl wrote:

Yes, I think putting it on DIMacroFile might be a better interface, but I'm not sure.
But then there is also another issue: I'm not sure whether it is legal to modify a uniquable (non-distinct) DINode because it will break uniquing. So I think DIMacroFiles must be created distinct to avoid this problem. Would this cause an issue during LTO with duplication or are they expected to be mostly unique across compile units anyway?

After short investigation there is an issue with both options.

1. Defining the Macro Dies as distinct will work from functional aspect, however, it will prevent sharing similar Macro Dies, which will be a real issue in dwarf5, where we can make usage of such sharing.
2. Defining the Macro Dies as unique will cause a functional issue, though not with the LTO, but with direct compilation like in the following example:

t.h

#ifdef A
#define Y X
#else
#define Y Z
#endif

t1.h

#define A
#include "t.h"
#undef A

t2.h

#define B
#include "t.h"
#undef B

t.c

#include "t1.h"
#include "t2.h"

In this case the LLVM IR for "t.h" will look like this:

!306 = !DIMacroFile(line: 2, file: !307, nodes: !308)
!307 = !DIFile(filename: "t.h", directory: "/")
!308 = !{!309, !310}
!309 = !DIMacro(type: DW_MACINFO_define, line: 2, name: "Y", value: "X")
!310 = !DIMacro(type: DW_MACINFO_define, line: 4, name: "Y", value: "Z")

Even though in this case the include of "t.h" from "t1.h" does not match the include of "t.h" from "t2.h", but because when we created the DIMacroFile we created it with empty nodes, there was a match (same macro tag, same line number, same file name, same empty node list).
The reason why LTO does not have the same problem, is that when we link the IR, the DIMacroFile already have its node list initialized, so the compare will be accurate.

So, if you ask me, I still prefer to use unique on Macro nodes, however we need to create them with the nodes and prevent changing them later.
Need to work on fixing this on the Clang part at D16135.

[aprantl]

This doesn't look too bad to me, but maybe I'm not seeing the whole picture: The bulk of the nodes will be DIMacro nodes and if I understand correctly they are leaf nodes and can safely be created non-distinct and shareable immediately. For the DIMacroFiles, we could create them as temporary distinct nodes first — similar to DIBuilder::createReplaceableCompositeType() and then replace them with non-distinct nodes once the lists are finalized.

[aprantl]

What's the status of this?

[aaboud]

In D16077#498128, @aprantl wrote:

Yes, I think putting it on DIMacroFile might be a better interface, but I'm not sure.
But then there is also another issue: I'm not sure whether it is legal to modify a uniquable (non-distinct) DINode because it will break uniquing. So I think DIMacroFiles must be created distinct to avoid this problem. Would this cause an issue during LTO with duplication or are they expected to be mostly unique across compile units anyway?

In D16077#588644, @aprantl wrote:

What's the status of this?

Sorry, for the delay, I was in an extended vacation.
Following F2F discussion with Richard Smith in the LLVM conference I have now some ideas on how to implement the Macro support in Clang.
So, expect new patches in the comming few weeks.

Again, sorry for the delay.

Regards,
Amjad

[aprantl]

No worries, I was mostly concerned that this might have been blocked on waiting for feedback from myself.

[aaboud]

Improved the code based on Adrian's comments.

[aprantl]

lib/IR/DIBuilder.cpp

94 ↗

(On Diff #82695)

if (!I.first)
Also comment what it means if it is null and why we are continuing?

101 ↗

(On Diff #82695)

clang-format?

201 ↗

(On Diff #82695)

This comment doesn't really explain anything that isn't obvious from reading the next line. Could be dropped.

210 ↗

(On Diff #82695)

Why does this need to be a temporary node? Can there be cycles?

214 ↗

(On Diff #82695)

spelling this as AllMacrosPerParent.insert({MF, nullptr}); is perhaps more obvious?

[aaboud]

Thanks Adrian for the comments, I will do the changes as needed and upload a new patch.
I have one answer below.

lib/IR/DIBuilder.cpp

210 ↗

(On Diff #82695)

The temporary node is needed because at this point we do not know the Elements of the DIMacroFile. So, we cannot create the DIMacroFile as Unique at this point (it is not final yet).

So, there is no Cycles, but temporary nodes are still needed, and it was you who suggested this solution few months ago :)

By the way, 'Elements' will always be an empty array passed to this function, but i was not sure if I should remove it or keep it. What do you think?

[aprantl]

Thanks! LGTM with outstanding comments addressed.

lib/IR/DIBuilder.cpp

210 ↗

(On Diff #82695)

I think we should remove it then and explain how it is being populated in the doxygen comment.

llvm/trunk/include/llvm/IR/DIBuilder.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_DIBUILDER_H
 #define LLVM_IR_DIBUILDER_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/None.h"
+#include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/TrackingMDRef.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
 #include <cstdint>
class DIBuilder {
     Function *ValueFn;       ///< llvm.dbg.value
 
     SmallVector<Metadata *, 4> AllEnumTypes;
     /// Track the RetainTypes, since they can be updated later on.
     SmallVector<TrackingMDNodeRef, 4> AllRetainTypes;
     SmallVector<Metadata *, 4> AllSubprograms;
     SmallVector<Metadata *, 4> AllGVs;
     SmallVector<TrackingMDNodeRef, 4> AllImportedModules;
+    /// Map Macro parent (which can be DIMacroFile or nullptr) to a list of
+    /// Metadata all of type DIMacroNode.
+    /// DIMacroNode's with nullptr parent are DICompileUnit direct children.
+    MapVector<MDNode *, SetVector<Metadata *>> AllMacrosPerParent;
 
     /// Track nodes that may be unresolved.
     SmallVector<TrackingMDNodeRef, 4> UnresolvedNodes;
     bool AllowUnresolvedNodes;
 
     /// Each subprogram's preserved local variables.
     ///
     /// Do not use a std::vector.  Some versions of libc++ apparently copy
public:
     /// \param Filename  File name.
     /// \param Directory Directory.
     /// \param CSKind    Checksum kind (e.g. CSK_None, CSK_MD5, CSK_SHA1, etc.).
     /// \param Checksum  Checksum data.
     DIFile *createFile(StringRef Filename, StringRef Directory,
                        DIFile::ChecksumKind CSKind = DIFile::CSK_None,
                        StringRef Checksum = StringRef());
 
+    /// Create debugging information entry for a macro.
+    /// \param Parent     Macro parent (could be nullptr).
+    /// \param Line       Source line number where the macro is defined.
+    /// \param MacroType  DW_MACINFO_define or DW_MACINFO_undef.
+    /// \param Name       Macro name.
+    /// \param Value      Macro value.
+    DIMacro *createMacro(DIMacroFile *Parent, unsigned Line, unsigned MacroType,
+                         StringRef Name, StringRef Value = StringRef());
+
+    /// Create debugging information temporary entry for a macro file.
+    /// List of macro node direct children will be calculated by DIBuilder,
+    /// using the \p Parent relationship.
+    /// \param Parent     Macro file parent (could be nullptr).
+    /// \param Line       Source line number where the macro file is included.
+    /// \param File       File descriptor containing the name of the macro file.
+    DIMacroFile *createTempMacroFile(DIMacroFile *Parent, unsigned Line,
+                                     DIFile *File);
+
     /// Create a single enumerator value.
     DIEnumerator *createEnumerator(StringRef Name, int64_t Val);
 
     /// Create a DWARF unspecified type.
     DIBasicType *createUnspecifiedType(StringRef Name);
 
     /// Create C++11 nullptr type.
     DIBasicType *createNullPtrType();
public:
 
     /// Create unspecified parameter type
     /// for a subroutine type.
     DIBasicType *createUnspecifiedParameter();
 
     /// Get a DINodeArray, create one if required.
     DINodeArray getOrCreateArray(ArrayRef<Metadata *> Elements);
 
+    /// Get a DIMacroNodeArray, create one if required.
+    DIMacroNodeArray getOrCreateMacroArray(ArrayRef<Metadata *> Elements);
+
     /// Get a DITypeRefArray, create one if required.
     DITypeRefArray getOrCreateTypeArray(ArrayRef<Metadata *> Elements);
 
     /// Create a descriptor for a value range.  This
     /// implicitly uniques the values returned.
     DISubrange *getOrCreateSubrange(int64_t Lo, int64_t Count);
 
     /// Create a new descriptor for the specified variable.

llvm/trunk/lib/IR/DIBuilder.cpp

void DIBuilder::finalize() {
   if (!AllGVs.empty())
     CUNode->replaceGlobalVariables(MDTuple::get(VMContext, AllGVs));
 
   if (!AllImportedModules.empty())
     CUNode->replaceImportedEntities(MDTuple::get(
         VMContext, SmallVector<Metadata *, 16>(AllImportedModules.begin(),
                                                AllImportedModules.end())));
 
+  for (const auto &I : AllMacrosPerParent) {
+    // DIMacroNode's with nullptr parent are DICompileUnit direct children.
+    if (!I.first) {
+      CUNode->replaceMacros(MDTuple::get(VMContext, I.second.getArrayRef()));
+      continue;
+    }
+    // Otherwise, it must be a temporary DIMacroFile that need to be resolved.
+    auto *TMF = cast<DIMacroFile>(I.first);
+    auto *MF = DIMacroFile::get(VMContext, dwarf::DW_MACINFO_start_file,
+                                TMF->getLine(), TMF->getFile(),
+                                getOrCreateMacroArray(I.second.getArrayRef()));
+    replaceTemporary(llvm::TempDIMacroNode(TMF), MF);
+  }
+
   // Now that all temp nodes have been replaced or deleted, resolve remaining
   // cycles.
   for (const auto &N : UnresolvedNodes)
     if (N && !N->isResolved())
       N->resolveCycles();
   UnresolvedNodes.clear();
 
   // Can't handle unresolved nodes anymore.
return ::createImportedModule(VMContext, dwarf::DW_TAG_imported_declaration,
                                 Context, Decl, Line, Name, AllImportedModules);
 }
 
 DIFile *DIBuilder::createFile(StringRef Filename, StringRef Directory,
                               DIFile::ChecksumKind CSKind, StringRef Checksum) {
   return DIFile::get(VMContext, Filename, Directory, CSKind, Checksum);
 }
 
+DIMacro *DIBuilder::createMacro(DIMacroFile *Parent, unsigned LineNumber,
+                                unsigned MacroType, StringRef Name,
+                                StringRef Value) {
+  assert(!Name.empty() && "Unable to create macro without name");
+  assert((MacroType == dwarf::DW_MACINFO_undef ||
+          MacroType == dwarf::DW_MACINFO_define) &&
+         "Unexpected macro type");
+  auto *M = DIMacro::get(VMContext, MacroType, LineNumber, Name, Value);
+  AllMacrosPerParent[Parent].insert(M);
+  return M;
+}
+
+DIMacroFile *DIBuilder::createTempMacroFile(DIMacroFile *Parent,
+                                            unsigned LineNumber, DIFile *File) {
+  auto *MF = DIMacroFile::getTemporary(VMContext, dwarf::DW_MACINFO_start_file,
+                                       LineNumber, File, DIMacroNodeArray())
+                 .release();
+  AllMacrosPerParent[Parent].insert(MF);
+  // Add the new temporary DIMacroFile to the macro per parent map as a parent.
+  // This is needed to assure DIMacroFile with no children to have an entry in
+  // the map. Otherwise, it will not be resolved in DIBuilder::finalize().
+  AllMacrosPerParent.insert({MF, {}});
+  return MF;
+}
+
 DIEnumerator *DIBuilder::createEnumerator(StringRef Name, int64_t Val) {
   assert(!Name.empty() && "Unable to create enumerator without name");
   return DIEnumerator::get(VMContext, Val, Name);
 }
 
 DIBasicType *DIBuilder::createUnspecifiedType(StringRef Name) {
   assert(!Name.empty() && "Unable to create type without name");
   return DIBasicType::get(VMContext, dwarf::DW_TAG_unspecified_type, Name);
DICompositeType *DIBuilder::createReplaceableCompositeType(
   trackIfUnresolved(RetTy);
   return RetTy;
 }
 
 DINodeArray DIBuilder::getOrCreateArray(ArrayRef<Metadata *> Elements) {
   return MDTuple::get(VMContext, Elements);
 }
 
+DIMacroNodeArray
+DIBuilder::getOrCreateMacroArray(ArrayRef<Metadata *> Elements) {
+  return MDTuple::get(VMContext, Elements);
+}
+
 DITypeRefArray DIBuilder::getOrCreateTypeArray(ArrayRef<Metadata *> Elements) {
   SmallVector<llvm::Metadata *, 16> Elts;
   for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
     if (Elements[i] && isa<MDNode>(Elements[i]))
       Elts.push_back(cast<DIType>(Elements[i]));
     else
       Elts.push_back(Elements[i]);
   }

llvm/trunk/unittests/IR/IRBuilderTest.cpp

TEST_F(IRBuilderTest, DIImportedEntity) {
   DIB.createImportedDeclaration(CU, nullptr, 1);
   DIB.createImportedDeclaration(CU, nullptr, 1);
   DIB.createImportedModule(CU, (DIImportedEntity *)nullptr, 2);
   DIB.createImportedModule(CU, (DIImportedEntity *)nullptr, 2);
   DIB.finalize();
   EXPECT_TRUE(verifyModule(*M));
   EXPECT_TRUE(CU->getImportedEntities().size() == 2);
 }
+
+//  0: #define M0 V0          <-- command line definition
+//  0: main.c                 <-- main file
+//     3:   #define M1 V1     <-- M1 definition in main.c
+//     5:   #include "file.h" <-- inclusion of file.h from main.c
+//          1: #define M2     <-- M2 definition in file.h with no value
+//     7:   #undef M1 V1      <-- M1 un-definition in main.c
+TEST_F(IRBuilderTest, DIBuilderMacro) {
+  IRBuilder<> Builder(BB);
+  DIBuilder DIB(*M);
+  auto File1 = DIB.createFile("main.c", "/");
+  auto File2 = DIB.createFile("file.h", "/");
+  auto CU = DIB.createCompileUnit(
+      dwarf::DW_LANG_C, DIB.createFile("main.c", "/"), "llvm-c", true, "", 0);
+  auto MDef0 =
+      DIB.createMacro(nullptr, 0, dwarf::DW_MACINFO_define, "M0", "V0");
+  auto TMF1 = DIB.createTempMacroFile(nullptr, 0, File1);
+  auto MDef1 = DIB.createMacro(TMF1, 3, dwarf::DW_MACINFO_define, "M1", "V1");
+  auto TMF2 = DIB.createTempMacroFile(TMF1, 5, File2);
+  auto MDef2 = DIB.createMacro(TMF2, 1, dwarf::DW_MACINFO_define, "M2");
+  auto MUndef1 = DIB.createMacro(TMF1, 7, dwarf::DW_MACINFO_undef, "M1");
+
+  EXPECT_EQ(dwarf::DW_MACINFO_define, MDef1->getMacinfoType());
+  EXPECT_EQ(3, MDef1->getLine());
+  EXPECT_EQ("M1", MDef1->getName());
+  EXPECT_EQ("V1", MDef1->getValue());
+
+  EXPECT_EQ(dwarf::DW_MACINFO_undef, MUndef1->getMacinfoType());
+  EXPECT_EQ(7, MUndef1->getLine());
+  EXPECT_EQ("M1", MUndef1->getName());
+  EXPECT_EQ("", MUndef1->getValue());
+
+  EXPECT_EQ(dwarf::DW_MACINFO_start_file, TMF2->getMacinfoType());
+  EXPECT_EQ(5, TMF2->getLine());
+  EXPECT_EQ(File2, TMF2->getFile());
+
+  DIB.finalize();
+
+  SmallVector<Metadata *, 4> Elements;
+  Elements.push_back(MDef2);
+  auto MF2 = DIMacroFile::get(Ctx, dwarf::DW_MACINFO_start_file, 5, File2,
+                              DIB.getOrCreateMacroArray(Elements));
+
+  Elements.clear();
+  Elements.push_back(MDef1);
+  Elements.push_back(MF2);
+  Elements.push_back(MUndef1);
+  auto MF1 = DIMacroFile::get(Ctx, dwarf::DW_MACINFO_start_file, 0, File1,
+                              DIB.getOrCreateMacroArray(Elements));
+
+  Elements.clear();
+  Elements.push_back(MDef0);
+  Elements.push_back(MF1);
+  auto MN0 = MDTuple::get(Ctx, Elements);
+  EXPECT_EQ(MN0, CU->getRawMacros());
+
+  Elements.clear();
+  Elements.push_back(MDef1);
+  Elements.push_back(MF2);
+  Elements.push_back(MUndef1);
+  auto MN1 = MDTuple::get(Ctx, Elements);
+  EXPECT_EQ(MN1, MF1->getRawElements());
+
+  Elements.clear();
+  Elements.push_back(MDef2);
+  auto MN2 = MDTuple::get(Ctx, Elements);
+  EXPECT_EQ(MN2, MF2->getRawElements());
+  EXPECT_TRUE(verifyModule(*M));
+}
 }