[DebugInfo] Skip MD5 checksums of preprocessed files

[probinson]

Summary

A preprocessed file doesn't properly reflect the original source,
so a checksum of it is pointless.

[probinson]

+ @rnk who did the initial checksum stuff for CodeView.

I am unclear how the notion of checksums should interact with preprocessed files.

Nit: while many tests have dates in the filenames, we no longer use that convention.

[trixirt]

Rename test to dwarf5-expansion-checksum.c

[probinson]

After thinking about this a bit... The # directive will cause the filename to be identified as the source for the subsequent lines. That means it will show up as the original source file in the line table.
However, the compiler doesn't have the actual source file of the header (or the original source file, for that matter) and so cannot compute a correct checksum for it. That means, it should omit the checksum, for any file identified with a # directive.
I haven't looked at how CodeView handles this situation; for DWARF v5 I made inconsistent use of a checksum into an assertion offense.
Should we reconsider that? Or have the # directive cause Clang to omit checksums for *all* files, even the one it is actually reading?
@aprantl, @rnk, thoughts?

[aprantl]

Once we encounter a # directive we are (most likely) looking at some form of preprocessed source and that means that the checksum will inevitably be different than what we would have gotten were we reading the file directly, because of the preprocessing. At this point the value of the hash approaches zero. I think dropping all checksums is reasonable in that situation.

[probinson]

I see that the per-file info does track the presence of # [line] N but it's not immediately obvious how to query "has any file seen one" which is really what I'd want to know. The file information has various levels of indirection...

[probinson]

@trixirt do you mind if I commandeer this review? I think I have a patch.

[trixirt]

commandeer away!
Sure that is fine.

[probinson]

Upload patch to suppress checksums when we see a preprocessed file.

[aprantl]

Minor comment inline.

clang/lib/CodeGen/CGDebugInfo.cpp

378 ↗

(On Diff #148663)

Can you add a comment explaining why we are doing this here?

[probinson]

clang/lib/CodeGen/CGDebugInfo.cpp

378 ↗

(On Diff #148663)

Of course.

cfe/trunk/lib/CodeGen/CGDebugInfo.h

 /// responsible for emitting to llvm globals or pass directly to the
 /// backend.
 class CGDebugInfo {
   friend class ApplyDebugLocation;
   friend class SaveAndRestoreLocation;
   CodeGenModule &CGM;
   const codegenoptions::DebugInfoKind DebugKind;
   bool DebugTypeExtRefs;
+  mutable bool EmitFileChecksums;
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
   ExternalASTSource::ASTSourceDescriptor PCHDescriptor;
   SourceLocation CurLoc;
   llvm::MDNode *CurInlinedAt = nullptr;
   llvm::DIType *VTablePtrType = nullptr;
   llvm::DIType *ClassTy = nullptr;

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

 }
 
 CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
     : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
       DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
       DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
     DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+                      CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
 CGDebugInfo::~CGDebugInfo() {
   assert(LexicalBlockStack.empty() &&
          "Region stack mismatch, stack not empty!");
 }
 
StringRef CGDebugInfo::getClassName(const RecordDecl *RD) {
 
   return StringRef();
 }
 
 Optional<llvm::DIFile::ChecksumKind>
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-      CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
     return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
-  if (Invalid)
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
+  if (Invalid || !Entry.isFile())
     return None;
+  if (Entry.getFile().hasLineDirectives()) {
+    // This must be a preprocessed file; its content won't match the original
+    // source; therefore checksumming the text we have is pointless or wrong.
+    EmitFileChecksums = false;
+    return None;
+  }
+  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID);
 
   llvm::MD5 Hash;
   llvm::MD5::MD5Result Result;
 
   Hash.update(MemBuffer->getBuffer());
   Hash.final(Result);
 
   Hash.stringifyResult(Result, Checksum);

cfe/trunk/test/CodeGen/md5-checksum-crash.c

+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited %s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")