[llvm] c9cb4fc - [DebugInfo] Store optional DIFile::Source as pointer

Jonas Hahnfeld via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 8 00:59:10 PST 2022


Author: Jonas Hahnfeld
Date: 2022-12-08T09:58:33+01:00
New Revision: c9cb4fc761cd78d3a04feb211d363e91933a8e08

URL: https://github.com/llvm/llvm-project/commit/c9cb4fc761cd78d3a04feb211d363e91933a8e08
DIFF: https://github.com/llvm/llvm-project/commit/c9cb4fc761cd78d3a04feb211d363e91933a8e08.diff

LOG: [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

Added: 
    

Modified: 
    llvm/include/llvm/IR/DebugInfoMetadata.h
    llvm/lib/AsmParser/LLParser.cpp
    llvm/lib/Bitcode/Reader/MetadataLoader.cpp
    llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
    llvm/lib/IR/DebugInfoMetadata.cpp
    llvm/lib/IR/LLVMContextImpl.h
    llvm/unittests/IR/DebugInfoTest.cpp
    llvm/unittests/IR/MetadataTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index f9e11345a739c..d2ecae606b469 100644
--- 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
index 76cb5ee6944f8..f02e8cb52d1fe 100644
--- 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
index 0f3ad255f6b72..96f436a9b137d 100644
--- 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
index 9c45ea00e8a09..28b1bac8960fd 100644
--- 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
index 424c44f0055e9..2d220cc133093 100644
--- 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
index 99270daf1974c..eadb3b7fba775 100644
--- 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
index 439c0d00ee4eb..1930ecdd8cc9f 100644
--- 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
index 3ffa45663d63b..982e48ba701bf 100644
--- 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");


        


More information about the llvm-commits mailing list