[DebugInfo] Alternate MD5 fix, NFC

[probinson]

Summary

Should have lower memory and time cost than D155991.

[dblaikie]

From the other review:

Could/should we do the lookup on the CU filename before it goes into the DI metadata, and store that FileID somewhere for later use?

There's a note in issue 63955 to the effect that I can't find an API to turn a filename into a FileID. If there is one that I didn't find, we could use FileIDs instead of pointers to name strings.

I was thinking more along the lines of keeping the same code we have, string pointer equality (though perhaps FileID is equivalent in cases where the pointer euqality is valid, and the FileID approach would be more self documenting/less subtle). I was thinking - for other files we get filenames correctly that are pointer identical because they're in the FileID, right? But for this one because we stash it in/get it from the DICompileUnit, that identity gets lost. But presumably the identity is true for the string filename used to build the DICompileUnit? So if, at that point, we stored the pointer, or FileID aside and used that instead of retrieving it from the DICompileUnit, perhaps that'd make this name consistent with the other names in being able to use pointer identity?

[probinson]

The CU's DIFile is conjured up in CGDebugInfo::CreateCompileUnit(), and the name is derived from -main-file-name rather than anything SourceManager provides. Although there is a comment wondering about that. CreateCompileUnit() computes a checksum for that DIFile, but apparently the DIFile never makes it into the DIFileCache.

DIFileCache is generated by CGDebugInfo::createFile(); it looks like there are some subtle differences in the path handling between what it does and what CreateCompileUnit() does, so making CreateCompileUnit() go through createFile() might not the the right thing. But it does look like CreateCompileUnit() might just be missing a one-liner to add the CU's DIFile to DIFileCache.

I'll try that instead.

[probinson]

Use the main FileID instead of expensive string compares.

Figured this out after staring at CreateCompileUnit for long enough. Seeding the DIFileCache with the DIFile created there made another test unhappy (difile_entry.cpp) but this fix doesn't.

[probinson]

Of course this means we're rerunning MD5 on the main source file; probably can capture that from TheCU and save that cost as well.

[probinson]

Reuse the main file's checksum instead of recalculating it.

[probinson]

Ping

[dblaikie]

Sounds OK to me - thanks!

bit more detail in the actual patch description/commit message (from some of the comments in this review, perhaps) would be handy.

[probinson]

Detail added in the commit message, good idea!

[nikic]

Thanks! Can confirm that this recovers the compile-time regression.

[probinson]

This patch is possibly a suspect in at least some bot failures although I'm at a loss to understand why. Perhaps I can't just blithely call getChecksum() and copy what it sends back? The ways of metadata remain mysterious to me. I'll be poking at this more as time permits, unless someone can tell me my obvious mistake.

[DavidSpickett]

Failing on one of our 2 stage AArch64 bots https://lab.llvm.org/buildbot/#/builders/176/builds/4267 after the reland (that bot has been red for various reasons today, so no emails got sent).

Maybe it helps to know that it doesn't fail on the same config with single stage https://lab.llvm.org/buildbot/#/builders/197/builds/8998. So it's at least able to build simple test cases (the test suite is failing to build for a separate reason).

If this is somehow AArch64 only we (Linaro) can help investigate, I see that it was ok on PowerPC for example https://lab.llvm.org/buildbot/#/builders/36/builds/36728.

[probinson]

Thanks @DavidSpickett the patch is currently reverted. I have a revised patch coming soon and I will keep a close eye on the bots.
I believe it's a string-lifetime issue and so whether it manifests is unpredictable, but we have enough different bots in the farm that it did get caught.

[probinson]

clang/lib/CodeGen/CGDebugInfo.cpp

423

In the final commit, Checksum is outside the if so that its lifetime persists to the end of the function (specifically, past the createFile call). CSInfo holds a StringRef to this string, not the string itself.

clang/lib/CodeGen/CGDebugInfo.h

#include "clang/Basic/WebAssemblyReferenceTypes.def"
   /// function.
   std::vector<unsigned> FnBeginRegionCount;
 
   /// This is a storage for names that are constructed on demand. For
   /// example, C++ destructors, C++ operators etc..
   llvm::BumpPtrAllocator DebugInfoNames;
   StringRef CWDName;
 
-  llvm::StringMap<llvm::TrackingMDRef> DIFileCache;
+  llvm::DenseMap<const char *, llvm::TrackingMDRef> DIFileCache;
   llvm::DenseMap<const FunctionDecl *, llvm::TrackingMDRef> SPCache;
   /// Cache declarations relevant to DW_TAG_imported_declarations (C++
   /// using declarations and global alias variables) that aren't covered
   /// by other more specific caches.
   llvm::DenseMap<const Decl *, llvm::TrackingMDRef> DeclCache;
   llvm::DenseMap<const Decl *, llvm::TrackingMDRef> ImportedDeclCache;
   llvm::DenseMap<const NamespaceDecl *, llvm::TrackingMDRef> NamespaceCache;
   llvm::DenseMap<const NamespaceAliasDecl *, llvm::TrackingMDRef>

clang/lib/CodeGen/CGDebugInfo.cpp

std::optional<StringRef> CGDebugInfo::getSource(const SourceManager &SM,
 
   return Source;
 }
 
 llvm::DIFile *CGDebugInfo::getOrCreateFile(SourceLocation Loc) {
   SourceManager &SM = CGM.getContext().getSourceManager();
   StringRef FileName;
   FileID FID;
+  std::optional<llvm::DIFile::ChecksumInfo<StringRef>> CSInfo;
 
   if (Loc.isInvalid()) {
     // The DIFile used by the CU is distinct from the main source file. Call
     // createFile() below for canonicalization if the source file was specified
     // with an absolute path.
     FileName = TheCU->getFile()->getFilename();
+    CSInfo = TheCU->getFile()->getChecksum();
   } else {
     PresumedLoc PLoc = SM.getPresumedLoc(Loc);
     FileName = PLoc.getFilename();
 
     if (FileName.empty()) {
       FileName = TheCU->getFile()->getFilename();
     } else {
       FileName = PLoc.getFilename();
     }
     FID = PLoc.getFileID();
   }
 
   // Cache the results.
   auto It = DIFileCache.find(FileName.data());
   if (It != DIFileCache.end()) {
     // Verify that the information still exists.
     if (llvm::Metadata *V = It->second)
       return cast<llvm::DIFile>(V);
   }
 
+  if (!CSInfo) {
     SmallString<64> Checksum;
-
     std::optional<llvm::DIFile::ChecksumKind> CSKind =
       computeChecksum(FID, Checksum);
-  std::optional<llvm::DIFile::ChecksumInfo<StringRef>> CSInfo;
     if (CSKind)
       CSInfo.emplace(*CSKind, Checksum);
+  }
   return createFile(FileName, CSInfo, getSource(SM, SM.getFileID(Loc)));
 }
 
 llvm::DIFile *CGDebugInfo::createFile(
     StringRef FileName,
     std::optional<llvm::DIFile::ChecksumInfo<StringRef>> CSInfo,
     std::optional<StringRef> Source) {
   StringRef Dir;

clang/test/CodeGenCXX/debug-info-function-context.cpp

 // RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -triple x86_64-pc-linux-gnu %s \
-// RUN:     -dwarf-version=5 -main-file-name %s  -o - | FileCheck %s
+// RUN:     -dwarf-version=5 -main-file-name debug-info-function-context.cpp  -o - | FileCheck %s
 
 struct C {
   void member_function();
   static int static_member_function();
   static int static_member_variable;
 };
 
 int C::static_member_variable = 0;

 int global_initialized_variable = C::static_member_function();
 
 // Check that the functions that belong to C have C as a context and the
 // functions that belong to the namespace have it as a context, and the global
 // functions (user-defined and artificial) have the file as a context.
 
 // The first DIFile is for the CU, the second is what everything else uses.
 // We're using DWARF v5 so both should have MD5 checksums.
-// CHECK: !DIFile(filename: "{{.*}}context.cpp",{{.*}} checksumkind: CSK_MD5
-// CHECK: ![[FILE:[0-9]+]] = !DIFile(filename: "{{.*}}context.cpp",{{.*}} checksumkind: CSK_MD5
+// CHECK: !DIFile(filename: "{{.*}}context.cpp",{{.*}} checksumkind: CSK_MD5, checksum: [[CKSUM:".*"]]
+// CHECK: ![[FILE:[0-9]+]] = !DIFile(filename: "{{.*}}context.cpp",{{.*}} checksumkind: CSK_MD5, checksum: [[CKSUM]]
 // CHECK: ![[C:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "C",
 // CHECK: ![[NS:.*]] = !DINamespace(name: "ns"
 // CHECK: !DISubprogram(name: "member_function",{{.*}} scope: ![[C]],{{.*}} DISPFlagDefinition
 
 // CHECK: !DISubprogram(name: "static_member_function",{{.*}} scope: ![[C]],{{.*}} DISPFlagDefinition
 
 // CHECK: !DISubprogram(name: "global_function",{{.*}} scope: ![[FILE]],{{.*}} DISPFlagDefinition
 
 // CHECK: !DISubprogram(name: "global_namespace_function",{{.*}} scope: ![[NS]],{{.*}} DISPFlagDefinition
 
 // CHECK: !DISubprogram(name: "__cxx_global_var_init",{{.*}} scope: ![[FILE]],{{.*}} DISPFlagDefinition
 // CHECK: !DISubprogram(linkageName: "_GLOBAL__sub_I_{{.*}}",{{.*}} scope: ![[FILE]],{{.*}} DISPFlagDefinition