From c9cb4fc761cd78d3a04feb211d363e91933a8e08 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Thu, 1 Dec 2022 14:14:43 +0100 Subject: [PATCH] [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/include/llvm/IR/DebugInfoMetadata.h | 21 ++++++++++----------- llvm/lib/AsmParser/LLParser.cpp | 8 ++++---- llvm/lib/Bitcode/Reader/MetadataLoader.cpp | 9 ++++----- llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | 2 +- llvm/lib/IR/DebugInfoMetadata.cpp | 12 ++++++------ llvm/lib/IR/LLVMContextImpl.h | 7 +++---- llvm/unittests/IR/DebugInfoTest.cpp | 18 ++++++++++++++++++ llvm/unittests/IR/MetadataTest.cpp | 14 ++++++++++++++ 8 files changed, 60 insertions(+), 31 deletions(-) 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> Checksum; - std::optional Source; + /// An optional source. A nullptr means none. + MDString *Source; DIFile(LLVMContext &C, StorageType Storage, - std::optional> CS, - std::optional Src, ArrayRef Ops); + std::optional> CS, MDString *Src, + ArrayRef 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( - 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> CS, - std::optional 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> CS = std::nullopt, - std::optional 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 getSource() const { - return Source ? std::optional((*Source)->getString()) + return Source ? std::optional(Source->getString()) : std::nullopt; } @@ -663,7 +662,7 @@ class DIFile : public DIScope { std::optional> getRawChecksum() const { return Checksum; } - std::optional getRawSource() const { return Source; } + MDString *getRawSource() const { return Source; } static StringRef getChecksumKindAsString(ChecksumKind CSKind); static std::optional 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 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(Record[3]), getMDString(Record[4])); MetadataList.assignValue( - GET_OR_DISTINCT(DIFile, (Context, getMDString(Record[1]), - getMDString(Record[2]), Checksum, - Record.size() > 5 ? std::optional( - 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> CS, - std::optional Src, ArrayRef Ops) + std::optional> CS, MDString *Src, + ArrayRef 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> CS, - std::optional 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 { MDString *Filename; MDString *Directory; std::optional> Checksum; - std::optional Source; + MDString *Source; MDNodeKeyImpl(MDString *Filename, MDString *Directory, std::optional> Checksum, - std::optional Source) + MDString *Source) : Filename(Filename), Directory(Directory), Checksum(Checksum), Source(Source) {} MDNodeKeyImpl(const DIFile *N) @@ -687,8 +687,7 @@ template <> struct MDNodeKeyImpl { 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(DVIs[0]->getValue(0))); } +TEST(DIBuiler, CreateFile) { + LLVMContext Ctx; + std::unique_ptr M(new Module("MyModule", Ctx)); + DIBuilder DIB(*M); + + DIFile *F = DIB.createFile("main.c", "/"); + EXPECT_EQ(std::nullopt, F->getSource()); + + std::optional> Checksum = std::nullopt; + std::optional 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 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> Checksum = std::nullopt; + std::optional 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");