[DebugInfo] Store optional DIFile::Source as pointer

getCanonicalMDString() also returns a nullptr for empty strings, which tripped over the getSource() method. Solve the ambiguity of no source versus an optional containing a nullptr by simply storing a pointer. Differential Revision: https://reviews.llvm.org/D138658
llvm · Dec 8, 2022 · c9cb4fc · c9cb4fc
1 parent 61318fa
commit c9cb4fc
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 31 deletions.
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -598,11 +598,12 @@ class DIFile : public DIScope {
 
 private:
   std::optional<ChecksumInfo<MDString *>> Checksum;
-  std::optional<MDString *> Source;
+  /// An optional source. A nullptr means none.
+  MDString *Source;
 
   DIFile(LLVMContext &C, StorageType Storage,
-         std::optional<ChecksumInfo<MDString *>> CS,
-         std::optional<MDString *> Src, ArrayRef<Metadata *> Ops);
+         std::optional<ChecksumInfo<MDString *>> CS, MDString *Src,
+         ArrayRef<Metadata *> Ops);
   ~DIFile() = default;
 
   static DIFile *getImpl(LLVMContext &Context, StringRef Filename,
@@ -615,15 +616,13 @@ class DIFile : public DIScope {
       MDChecksum.emplace(CS->Kind, getCanonicalMDString(Context, CS->Value));
     return getImpl(Context, getCanonicalMDString(Context, Filename),
                    getCanonicalMDString(Context, Directory), MDChecksum,
-                   Source ? std::optional<MDString *>(
-                                getCanonicalMDString(Context, *Source))
-                          : std::nullopt,
-                   Storage, ShouldCreate);
+                   Source ? MDString::get(Context, *Source) : nullptr, Storage,
+                   ShouldCreate);
   }
   static DIFile *getImpl(LLVMContext &Context, MDString *Filename,
                          MDString *Directory,
                          std::optional<ChecksumInfo<MDString *>> CS,
-                         std::optional<MDString *> Source, StorageType Storage,
+                         MDString *Source, StorageType Storage,
                          bool ShouldCreate = true);
 
   TempDIFile cloneImpl() const {
@@ -640,7 +639,7 @@ class DIFile : public DIScope {
   DEFINE_MDNODE_GET(DIFile,
                     (MDString * Filename, MDString *Directory,
                      std::optional<ChecksumInfo<MDString *>> CS = std::nullopt,
-                     std::optional<MDString *> Source = std::nullopt),
+                     MDString *Source = nullptr),
                     (Filename, Directory, CS, Source))
 
   TempDIFile clone() const { return cloneImpl(); }
@@ -654,7 +653,7 @@ class DIFile : public DIScope {
     return StringRefChecksum;
   }
   std::optional<StringRef> getSource() const {
-    return Source ? std::optional<StringRef>((*Source)->getString())
+    return Source ? std::optional<StringRef>(Source->getString())
                   : std::nullopt;
   }
 
@@ -663,7 +662,7 @@ class DIFile : public DIScope {
   std::optional<ChecksumInfo<MDString *>> getRawChecksum() const {
     return Checksum;
   }
-  std::optional<MDString *> getRawSource() const { return Source; }
+  MDString *getRawSource() const { return Source; }
 
   static StringRef getChecksumKindAsString(ChecksumKind CSKind);
   static std::optional<ChecksumKind> getChecksumKind(StringRef CSKindStr);

diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
@@ -4996,11 +4996,11 @@ bool LLParser::parseDIFile(MDNode *&Result, bool IsDistinct) {
   else if (checksumkind.Seen || checksum.Seen)
     return Lex.Error("'checksumkind' and 'checksum' must be provided together");
 
-  std::optional<MDString *> OptSource;
+  MDString *Source = nullptr;
   if (source.Seen)
-    OptSource = source.Val;
-  Result = GET_OR_DISTINCT(DIFile, (Context, filename.Val, directory.Val,
-                                    OptChecksum, OptSource));
+    Source = source.Val;
+  Result = GET_OR_DISTINCT(
+      DIFile, (Context, filename.Val, directory.Val, OptChecksum, Source));
   return false;
 }
 

diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -1621,11 +1621,10 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
       Checksum.emplace(static_cast<DIFile::ChecksumKind>(Record[3]),
                        getMDString(Record[4]));
     MetadataList.assignValue(
-        GET_OR_DISTINCT(DIFile, (Context, getMDString(Record[1]),
-                                 getMDString(Record[2]), Checksum,
-                                 Record.size() > 5 ? std::optional<MDString *>(
-                                                         getMDString(Record[5]))
-                                                   : std::nullopt)),
+        GET_OR_DISTINCT(DIFile,
+                        (Context, getMDString(Record[1]),
+                         getMDString(Record[2]), Checksum,
+                         Record.size() > 5 ? getMDString(Record[5]) : nullptr)),
         NextMetadataNo);
     NextMetadataNo++;
     break;

diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -1808,7 +1808,7 @@ void ModuleBitcodeWriter::writeDIFile(const DIFile *N,
   }
   auto Source = N->getRawSource();
   if (Source)
-    Record.push_back(VE.getMetadataOrNullID(*Source));
+    Record.push_back(VE.getMetadataOrNullID(Source));
 
   Stream.EmitRecord(bitc::METADATA_FILE, Record, Abbrev);
   Record.clear();

diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -801,8 +801,8 @@ DISubroutineType *DISubroutineType::getImpl(LLVMContext &Context, DIFlags Flags,
 }
 
 DIFile::DIFile(LLVMContext &C, StorageType Storage,
-               std::optional<ChecksumInfo<MDString *>> CS,
-               std::optional<MDString *> Src, ArrayRef<Metadata *> Ops)
+               std::optional<ChecksumInfo<MDString *>> CS, MDString *Src,
+               ArrayRef<Metadata *> Ops)
     : DIScope(C, DIFileKind, Storage, dwarf::DW_TAG_file_type, Ops),
       Checksum(CS), Source(Src) {}
 
@@ -834,15 +834,15 @@ DIFile::getChecksumKind(StringRef CSKindStr) {
 DIFile *DIFile::getImpl(LLVMContext &Context, MDString *Filename,
                         MDString *Directory,
                         std::optional<DIFile::ChecksumInfo<MDString *>> CS,
-                        std::optional<MDString *> Source, StorageType Storage,
+                        MDString *Source, StorageType Storage,
                         bool ShouldCreate) {
   assert(isCanonical(Filename) && "Expected canonical MDString");
   assert(isCanonical(Directory) && "Expected canonical MDString");
   assert((!CS || isCanonical(CS->Value)) && "Expected canonical MDString");
-  assert((!Source || isCanonical(*Source)) && "Expected canonical MDString");
+  // We do *NOT* expect Source to be a canonical MDString because nullptr
+  // means none, so we need something to represent the empty file.
   DEFINE_GETIMPL_LOOKUP(DIFile, (Filename, Directory, CS, Source));
-  Metadata *Ops[] = {Filename, Directory, CS ? CS->Value : nullptr,
-                     Source.value_or(nullptr)};
+  Metadata *Ops[] = {Filename, Directory, CS ? CS->Value : nullptr, Source};
   DEFINE_GETIMPL_STORE(DIFile, (CS, Source), Ops);
 }
 DICompileUnit::DICompileUnit(LLVMContext &C, StorageType Storage,

diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
@@ -668,11 +668,11 @@ template <> struct MDNodeKeyImpl<DIFile> {
   MDString *Filename;
   MDString *Directory;
   std::optional<DIFile::ChecksumInfo<MDString *>> Checksum;
-  std::optional<MDString *> Source;
+  MDString *Source;
 
   MDNodeKeyImpl(MDString *Filename, MDString *Directory,
                 std::optional<DIFile::ChecksumInfo<MDString *>> Checksum,
-                std::optional<MDString *> Source)
+                MDString *Source)
       : Filename(Filename), Directory(Directory), Checksum(Checksum),
         Source(Source) {}
   MDNodeKeyImpl(const DIFile *N)
@@ -687,8 +687,7 @@ template <> struct MDNodeKeyImpl<DIFile> {
 
   unsigned getHashValue() const {
     return hash_combine(Filename, Directory, Checksum ? Checksum->Kind : 0,
-                        Checksum ? Checksum->Value : nullptr,
-                        Source.value_or(nullptr));
+                        Checksum ? Checksum->Value : nullptr, Source);
   }
 };
 

diff --git a/llvm/unittests/IR/DebugInfoTest.cpp b/llvm/unittests/IR/DebugInfoTest.cpp
@@ -190,6 +190,24 @@ TEST(MetadataTest, DeleteInstUsedByDbgValue) {
   EXPECT_TRUE(isa<UndefValue>(DVIs[0]->getValue(0)));
 }
 
+TEST(DIBuiler, CreateFile) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> M(new Module("MyModule", Ctx));
+  DIBuilder DIB(*M);
+
+  DIFile *F = DIB.createFile("main.c", "/");
+  EXPECT_EQ(std::nullopt, F->getSource());
+
+  std::optional<DIFile::ChecksumInfo<StringRef>> Checksum = std::nullopt;
+  std::optional<StringRef> Source = std::nullopt;
+  F = DIB.createFile("main.c", "/", Checksum, Source);
+  EXPECT_EQ(Source, F->getSource());
+
+  Source = "";
+  F = DIB.createFile("main.c", "/", Checksum, Source);
+  EXPECT_EQ(Source, F->getSource());
+}
+
 TEST(DIBuilder, CreateFortranArrayTypeWithAttributes) {
   LLVMContext Ctx;
   std::unique_ptr<Module> M(new Module("MyModule", Ctx));

diff --git a/llvm/unittests/IR/MetadataTest.cpp b/llvm/unittests/IR/MetadataTest.cpp
@@ -2221,6 +2221,20 @@ TEST_F(DIFileTest, get) {
   EXPECT_EQ(N, MDNode::replaceWithUniqued(std::move(Temp)));
 }
 
+TEST_F(DIFileTest, EmptySource) {
+  DIFile *N = DIFile::get(Context, "file", "dir");
+  EXPECT_EQ(std::nullopt, N->getSource());
+
+  std::optional<DIFile::ChecksumInfo<StringRef>> Checksum = std::nullopt;
+  std::optional<StringRef> Source = std::nullopt;
+  N = DIFile::get(Context, "file", "dir", Checksum, Source);
+  EXPECT_EQ(Source, N->getSource());
+
+  Source = "";
+  N = DIFile::get(Context, "file", "dir", Checksum, Source);
+  EXPECT_EQ(Source, N->getSource());
+}
+
 TEST_F(DIFileTest, ScopeGetFile) {
   // Ensure that DIScope::getFile() returns itself.
   DIScope *N = DIFile::get(Context, "file", "dir");