[DebugInfo] Refactoring DIType::setFlags to DIType::cloneWithFlags

[rtereshin]

Summary

and using the latter in DIBuilder::createArtificialType and
DIBuilder::createObjectPointerType methods as well as introducing
mirroring DISubprogram::cloneWithFlags and
DIBuilder::createArtificialSubprogram methods.

The primary goal here is to add createArtificialSubprogram to support
a pass downstream while keeping the method consistent with the
existing ones and making sure we don't encourage changing already
created DI-nodes.

[aprantl]

We generally try to discourage modifying MDNodes after creation since this is an inherently dangerous thing to do since you never know who might be holding on to a pointer to an MDNode. Do you have really good reason to alter the flags on an existing node, or could you also just set the flags correctly at creation time? Even if the answer is yes I wonder if it wouldn't be better to clone a new distinct node with the modified flag.

[rtereshin]

In D47615#1119097, @aprantl wrote:

We generally try to discourage modifying MDNodes after creation since this is an inherently dangerous thing to do since you never know who might be holding on to a pointer to an MDNode. Do you have really good reason to alter the flags on an existing node, or could you also just set the flags correctly at creation time? Even if the answer is yes I wonder if it wouldn't be better to clone a new distinct node with the modified flag.

I'm cloning an existing node to a temporary node, modifying the flags, and converting it to a distinct node.

Do you want me to put all of it in a single method instead of providing a method for modifying the flags?
If so, do we need to change DIType the same way? Currently it has public setFlags method, which is apparently only used by DIBuilder's methods that clone - set flags - convert to a distinct node.

[aprantl]

I didn't realize the DIBuilder was using the same workflow in createTypeWithFlags(). What do you think about making this method private and only having DIBuilder call it in a controlled setting (and add a DIBuilder createSubprogramWithFlags() method)?

[rtereshin]

In D47615#1119208, @aprantl wrote:

I didn't realize the DIBuilder was using the same workflow in createTypeWithFlags(). What do you think about making this method private and only having DIBuilder call it in a controlled setting (and add a DIBuilder createSubprogramWithFlags() method)?

That would require making DIBuilder a friend to DIType and DISubprogram. And looks like none of the DI* classes have DIBuilder as a friend at the moment, not sure I want to start the tradition.

Alternatively, we could:

1. toughen the assertion to isTemporary instead of just !isUniqued and mirror the methods for DISubprogram, or
2. remove setFlags methods entirely and replace them with cloneWithFlags on DIType / DISubprogram directly, having DIBuilder just call it and replaceWithDistinct later, or
3. same as 2, but have cloneWithFlags returning already distinct node and remove createTypeWithFlags from DIBuilder (which is private anyway by the virtue of being a static function) and add DIBuilder::createArtificialSubprogram (similar to DIBuilder::createArtificialType)

[aprantl]

toughen the assertion to isTemporary instead of just !isUniqued and mirror the methods for DISubprogram, or

I like that.

Options 2 and 3 also work for me. It's more work to implement but it seems even cleaner.

[rtereshin]

making sure we don't expose setters on already created DI nodes + adding a test

[aprantl]

Looks good. I have a few nitpicks about the doxygen comments inline, but otherwise this is good to go!

include/llvm/IR/DIBuilder.h

518

Create a distinct clone of SP with DIFlagArtificial set. ?

521

Create a uniqued clone of Ty with DIFlagArtificial set.

524

etc.

[aprantl]

Oh.. please be sure to update any LLVM clients of the API (such as clang and the LLVM C and Go bindings)

[rtereshin]

In D47615#1119630, @aprantl wrote:

Oh.. please be sure to update any LLVM clients of the API (such as clang and the LLVM C and Go bindings)

If ninja-all passes with clang checked out - does that mean they are all right or I need to explicitly look elsewhere?

[aprantl]

No that seems fine. The bots will tell you if it wasn't enough anyway ;-)

[aprantl]

You can do a git grep for the API just to make sure that the Go bindings don't use it, because you probably didn't build them.

include/llvm/IR/DIBuilder.h

public:
     /// \param Flags           E.g.: LValueReference.
     ///                        These flags are used to emit dwarf attributes.
     /// \param CC              Calling convention, e.g. dwarf::DW_CC_normal
     DISubroutineType *
     createSubroutineType(DITypeRefArray ParameterTypes,
                          DINode::DIFlags Flags = DINode::FlagZero,
                          unsigned CC = 0);
 
-    /// Create a new DIType* with "artificial" flag set.
-    DIType *createArtificialType(DIType *Ty);
+    /// Create a new distinct DISubprogram* with "artificial" flag set.
+    static DISubprogram *createArtificialSubprogram(DISubprogram *SP);
 
-    /// Create a new DIType* with the "object pointer"
-    /// flag set.
-    DIType *createObjectPointerType(DIType *Ty);
+    /// Create a new uniqued DIType* with "artificial" flag set.
+    static DIType *createArtificialType(DIType *Ty);
+
+    /// Create a new uniqued DIType* with the "object pointer" flag set.
+    static DIType *createObjectPointerType(DIType *Ty);
 
     /// Create a permanent forward-declared type.
     DICompositeType *createForwardDecl(unsigned Tag, StringRef Name,
                                        DIScope *Scope, DIFile *F, unsigned Line,
                                        unsigned RuntimeLang = 0,
                                        uint64_t SizeInBits = 0,
                                        uint32_t AlignInBits = 0,
                                        StringRef UniqueIdentifier = "");

include/llvm/IR/DebugInfoMetadata.h

public:
 
   DIScopeRef getScope() const { return DIScopeRef(getRawScope()); }
   StringRef getName() const { return getStringOperand(2); }
 
 
   Metadata *getRawScope() const { return getOperand(1); }
   MDString *getRawName() const { return getOperandAs<MDString>(2); }
 
-  void setFlags(DIFlags NewFlags) {
-    assert(!isUniqued() && "Cannot set flags on uniqued nodes");
-    Flags = NewFlags;
+  /// Returns a new temporary DIType with updated Flags
+  TempDIType cloneWithFlags(DIFlags NewFlags) const {
+    auto NewTy = clone();
+    NewTy->Flags = NewFlags;
+    return NewTy;
   }
 
   bool isPrivate() const {
     return (getFlags() & FlagAccessibility) == FlagPrivate;
   }
   bool isProtected() const {
     return (getFlags() & FlagAccessibility) == FlagProtected;
   }
DEFINE_MDNODE_GET(
        Metadata *ThrownTypes = nullptr),
       (Scope, Name, LinkageName, File, Line, Type, IsLocalToUnit, IsDefinition,
        ScopeLine, ContainingType, Virtuality, VirtualIndex, ThisAdjustment,
        Flags, IsOptimized, Unit, TemplateParams, Declaration, RetainedNodes,
        ThrownTypes))
 
   TempDISubprogram clone() const { return cloneImpl(); }
 
+  /// Returns a new temporary DISubprogram with updated Flags
+  TempDISubprogram cloneWithFlags(DIFlags NewFlags) const {
+    auto NewSP = clone();
+    NewSP->Flags = NewFlags;
+    return NewSP;
+  }
+
 public:
   unsigned getLine() const { return Line; }
   unsigned getVirtuality() const { return Virtuality; }
   unsigned getVirtualIndex() const { return VirtualIndex; }
   int getThisAdjustment() const { return ThisAdjustment; }
   unsigned getScopeLine() const { return ScopeLine; }
   DIFlags getFlags() const { return Flags; }
   bool isLocalToUnit() const { return IsLocalToUnit; }

lib/IR/DIBuilder.cpp

DICompositeType *DIBuilder::createVectorType(uint64_t Size,
                                              DINodeArray Subscripts) {
   auto *R = DICompositeType::get(VMContext, dwarf::DW_TAG_array_type, "",
                                  nullptr, 0, nullptr, Ty, Size, AlignInBits, 0,
                                  DINode::FlagVector, Subscripts, 0, nullptr);
   trackIfUnresolved(R);
   return R;
 }
 
-static DIType *createTypeWithFlags(LLVMContext &Context, DIType *Ty,
+DISubprogram *DIBuilder::createArtificialSubprogram(DISubprogram *SP) {
+  auto NewSP = SP->cloneWithFlags(SP->getFlags() | DINode::FlagArtificial);
+  return MDNode::replaceWithDistinct(std::move(NewSP));
+}
+
+static DIType *createTypeWithFlags(const DIType *Ty,
                                    DINode::DIFlags FlagsToSet) {
-  auto NewTy = Ty->clone();
-  NewTy->setFlags(NewTy->getFlags() | FlagsToSet);
+  auto NewTy = Ty->cloneWithFlags(Ty->getFlags() | FlagsToSet);
   return MDNode::replaceWithUniqued(std::move(NewTy));
 }
 
 DIType *DIBuilder::createArtificialType(DIType *Ty) {
   // FIXME: Restrict this to the nodes where it's valid.
   if (Ty->isArtificial())
     return Ty;
-  return createTypeWithFlags(VMContext, Ty, DINode::FlagArtificial);
+  return createTypeWithFlags(Ty, DINode::FlagArtificial);
 }
 
 DIType *DIBuilder::createObjectPointerType(DIType *Ty) {
   // FIXME: Restrict this to the nodes where it's valid.
   if (Ty->isObjectPointer())
     return Ty;
   DINode::DIFlags Flags = DINode::FlagObjectPointer | DINode::FlagArtificial;
-  return createTypeWithFlags(VMContext, Ty, Flags);
+  return createTypeWithFlags(Ty, Flags);
 }
 
 void DIBuilder::retainType(DIScope *T) {
   assert(T && "Expected non-null type");
   assert((isa<DIType>(T) || (isa<DISubprogram>(T) &&
                              cast<DISubprogram>(T)->isDefinition() == false)) &&
          "Expected type or subprogram declaration");
   AllRetainTypes.emplace_back(T);

unittests/IR/IRBuilderTest.cpp

TEST_F(IRBuilderTest, DIBuilder) {
   auto BarSP = DIB.createFunction(CU, "bar", "", File, 1, Type, false, true, 1,
                                   DINode::FlagZero, true);
   auto BadScope = DIB.createLexicalBlockFile(BarSP, File, 0);
   I->setDebugLoc(DebugLoc::get(2, 0, BadScope));
   DIB.finalize();
   EXPECT_TRUE(verifyModule(*M));
 }
 
+TEST_F(IRBuilderTest, createArtificialSubprogram) {
+  IRBuilder<> Builder(BB);
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("main.c", "/");
+  auto CU = DIB.createCompileUnit(dwarf::DW_LANG_C, File, "clang",
+                                  /*isOptimized=*/true, /*Flags=*/"",
+                                  /*Runtime Version=*/0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(CU, "foo", /*LinkageName=*/"", File,
+                               /*LineNo=*/1, Type, /*isLocalToUnit=*/false,
+                               /*isDefinition=*/true, /*ScopeLine=*/2,
+                               DINode::FlagZero, /*isOptimized=*/true);
+  EXPECT_TRUE(SP->isDistinct());
+
+  F->setSubprogram(SP);
+  AllocaInst *I = Builder.CreateAlloca(Builder.getInt8Ty());
+  ReturnInst *R = Builder.CreateRetVoid();
+  I->setDebugLoc(DebugLoc::get(3, 2, SP));
+  R->setDebugLoc(DebugLoc::get(4, 2, SP));
+  DIB.finalize();
+  EXPECT_FALSE(verifyModule(*M));
+
+  Function *G = Function::Create(F->getFunctionType(),
+                                 Function::ExternalLinkage, "", M.get());
+  BasicBlock *GBB = BasicBlock::Create(Ctx, "", G);
+  Builder.SetInsertPoint(GBB);
+  I->removeFromParent();
+  Builder.Insert(I);
+  Builder.CreateRetVoid();
+  EXPECT_FALSE(verifyModule(*M));
+
+  DISubprogram *GSP = DIBuilder::createArtificialSubprogram(F->getSubprogram());
+  EXPECT_EQ(SP->getFile(), GSP->getFile());
+  EXPECT_EQ(SP->getType(), GSP->getType());
+  EXPECT_EQ(SP->getLine(), GSP->getLine());
+  EXPECT_EQ(SP->getScopeLine(), GSP->getScopeLine());
+  EXPECT_TRUE(GSP->isDistinct());
+
+  G->setSubprogram(GSP);
+  EXPECT_TRUE(verifyModule(*M));
+
+  auto *InlinedAtNode =
+      DILocation::getDistinct(Ctx, GSP->getScopeLine(), 0, GSP);
+  DebugLoc DL = I->getDebugLoc();
+  DenseMap<const MDNode *, MDNode *> IANodes;
+  auto IA = DebugLoc::appendInlinedAt(DL, InlinedAtNode, Ctx, IANodes);
+  auto NewDL = DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(), IA);
+  I->setDebugLoc(NewDL);
+  EXPECT_FALSE(verifyModule(*M));
+
+  EXPECT_EQ("foo", SP->getName());
+  EXPECT_EQ("foo", GSP->getName());
+  EXPECT_FALSE(SP->isArtificial());
+  EXPECT_TRUE(GSP->isArtificial());
+}
+
 TEST_F(IRBuilderTest, InsertExtractElement) {
   IRBuilder<> Builder(BB);
 
   auto VecTy = VectorType::get(Builder.getInt64Ty(), 4);
   auto Elt1 = Builder.getInt64(-1);
   auto Elt2 = Builder.getInt64(-2);
   Value *Vec = UndefValue::get(VecTy);
   Vec = Builder.CreateInsertElement(Vec, Elt1, Builder.getInt8(1));

unittests/IR/MetadataTest.cpp

TEST_F(DITypeTest, clone) {
   // Check that DIType has a specialized clone that returns TempDIType.
   DIType *N = DIBasicType::get(Context, dwarf::DW_TAG_base_type, "int", 32, 32,
                                dwarf::DW_ATE_signed);
 
   TempDIType Temp = N->clone();
   EXPECT_EQ(N, MDNode::replaceWithUniqued(std::move(Temp)));
 }
 
-TEST_F(DITypeTest, setFlags) {
+TEST_F(DITypeTest, cloneWithFlags) {
   // void (void)
   Metadata *TypesOps[] = {nullptr};
   Metadata *Types = MDTuple::get(Context, TypesOps);
 
   DIType *D =
       DISubroutineType::getDistinct(Context, DINode::FlagZero, 0, Types);
   EXPECT_EQ(DINode::FlagZero, D->getFlags());
-  D->setFlags(DINode::FlagRValueReference);
-  EXPECT_EQ(DINode::FlagRValueReference, D->getFlags());
-  D->setFlags(DINode::FlagZero);
+  TempDIType D2 = D->cloneWithFlags(DINode::FlagRValueReference);
+  EXPECT_EQ(DINode::FlagRValueReference, D2->getFlags());
   EXPECT_EQ(DINode::FlagZero, D->getFlags());
 
   TempDIType T =
       DISubroutineType::getTemporary(Context, DINode::FlagZero, 0, Types);
   EXPECT_EQ(DINode::FlagZero, T->getFlags());
-  T->setFlags(DINode::FlagRValueReference);
-  EXPECT_EQ(DINode::FlagRValueReference, T->getFlags());
-  T->setFlags(DINode::FlagZero);
+  TempDIType T2 = T->cloneWithFlags(DINode::FlagRValueReference);
+  EXPECT_EQ(DINode::FlagRValueReference, T2->getFlags());
   EXPECT_EQ(DINode::FlagZero, T->getFlags());
 }
 
 typedef MetadataTest DIDerivedTypeTest;
 
 TEST_F(DIDerivedTypeTest, get) {
   DIFile *File = getFile();
   DIScope *Scope = getSubprogram();