[DebugInfo] Don't emit checksums when compiling a preprocessed CPP

[aganea]

Summary

When compiling from an already preprocessed CPP, the checksums generated in the debug info would be those of the (input) preprocessed CPP, not those of the actual #includes.
Note how all the files have the same checksum below:

** Module: "E:\RD\tests\clang_hash\main.obj"

     0 E:\RD\tests\clang_hash\lib.h (MD5: 136293700AE501A1FB76EBD273C8D288)
     1 C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\stdio.h (MD5: 136293700AE501A1FB76EBD273C8D288)
     2 E:\RD\tests\clang_hash\main.cpp (MD5: 136293700AE501A1FB76EBD273C8D288)
     3 C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\corecrt_stdio_config.h (MD5: 136293700AE501A1FB76EBD273C8D288)

When debugging in Visual Studio an EXE linked with the OBJ above, Visual Studio complains about the source files being different from when they were compiled, since the sources are compared by hash.

This patch simply clears the checksums to match MSVC behavior. Visual Studio will simply bypass the hash checking in that case, and hapily open the source file.

** Module: "E:\RD\tests\clang_hash\main.obj"

     0 c:\program files (x86)\windows kits\10\include\10.0.17763.0\ucrt\stdio.h (None)
     1 c:\program files (x86)\windows kits\10\include\10.0.17763.0\ucrt\corecrt_wstdio.h (None)
     2 c:\program files (x86)\windows kits\10\include\10.0.17763.0\ucrt\corecrt_stdio_config.h (None)
     3 c:\program files (x86)\microsoft visual studio\2017\professional\vc\tools\msvc\14.16.27023\include\vcruntime_new.h (None)
     4 e:\rd\tests\clang_hash\main.cpp (None)
     5 e:\rd\tests\clang_hash\lib.h (None)

We could write more complicated code to open the files from the corresponding #line directives, but we have no guarantee the preprocessed CPP is compiled in the same conditions as when it was pre-processed. Actually, when using Fastbuild, the files are preprocessed locally on the user's PC; then the preprocessed content is sent and compiled remotely on another PC that does not have the source code.

Thanks to Lambert Clara for the preliminary patch!

Fixes PR41215

[rnk]

lib/CodeGen/CGDebugInfo.cpp

436–437

I was unhappy with this check. It seems a bit ridiculous to compare the filenames by string when we surely have ids we could look at, but I wasn't able to figure out how.

test/Driver/cl-preprocess-md5.cpp

1 ↗

(On Diff #193764)

This should be a much smaller test in test/CodeGen. You can check in an already pre-processed file with line markers in it, run clang -cc1 -emit-llvm, and check that the DIFile has no checksum. Clang's test suite can't depend on lld, and I'm not sure if it has deps on llvm-pdbutil either.

[aganea]

I made a more elegant change, where no additional lookups or string compares are required.

Just a minor issue, I noted btw that clang /E does not generate #line directives, but simply #. In contrast, MSVC cl /E generates #line in the preprocessed output. The issue is that if compiling clang's preprocessed output with MSVC, it doesn't like # alone and throws an error. ie:

$ cat test.cpp
void test() { }

$ clang-cl /c /E test.cpp >test-pre.cpp

$ cat test-pre.cpp
# 1 "C:\\Users\\aganea\\Desktop\\test\\test.cpp"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 361 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "C:\\Users\\aganea\\Desktop\\test\\test.cpp" 2
void test() { }

$ cl /c test-pre.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27027.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test-pre.cpp
C:\Users\aganea\Desktop\test\test-pre.cpp(1): error C2019: expected preprocessor directive, found '1'
C:\Users\aganea\Desktop\test\test-pre.cpp(2): error C2019: expected preprocessor directive, found '1'
C:\Users\aganea\Desktop\test\test-pre.cpp(3): error C2019: expected preprocessor directive, found '1'
C:\Users\aganea\Desktop\test\test-pre.cpp(4): error C2019: expected preprocessor directive, found '3'
C:\Users\aganea\Desktop\test\test-pre.cpp(5): error C2019: expected preprocessor directive, found '1'
C:\Users\aganea\Desktop\test\test-pre.cpp(6): error C2019: expected preprocessor directive, found '1'
C:\Users\aganea\Desktop\test\test-pre.cpp(7): error C2019: expected preprocessor directive, found '1'

$ cl /c /E test.cpp >test-pre.cpp

$ cat test-pre.cpp
#line 1 "C:\\Users\\aganea\\Desktop\\test\\test.cpp"
void test() { }

$ cl /c test-pre.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27027.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test-pre.cpp

[thakis]

See http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp#756 for a "is same file" example. I'm not sure adding a bool to PresumedLoc is worth it for this.

[aganea]

In D60283#1456497, @thakis wrote:

See http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp#756 for a "is same file" example. I'm not sure adding a bool to PresumedLoc is worth it for this.

Yeah that works. However I'm getting two conflicting directions - I thought the worry was to avoid an extra runtime cost when comparing strings (see first version). @rnk wanted at first a compare by FileID, although it looks like it's more costly to do that, because all paths end up in SourceManager::getFileIDSlow. Just by tracing the code on a small preprocessed CPP, it looks like a costly solution. Using IsFromSameFile ends up in SourceManager::getFileIDSlow several times per iteration - whereas the solution proposed above (boolean) has zero runtime cost. I'm worried that large files with lots of #line will be much slower to compile. What do you think?

[aganea]

I'll try profiling several solutions on a large unity/jumbo file and get back with some results.

[rnk]

+@rsmith for the PresumedLoc change

From glancing on the PresumedLoc computation code, I think this bool might be the way to go. You could make it a bit more "free" by stealing a bit from the column, if we're concerned about size.

FYI, I'm off to EuroLLVM after this and return in about two weeks.

[aganea]

Steal one bit from PresumedLoc::Column as suggested by @rnk.
Ping @rsmith !

[rnk]

In D60283#1456497, @thakis wrote:

See http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp#756 for a "is same file" example. I'm not sure adding a bool to PresumedLoc is worth it for this.

@thakis, do you still object to adding info to PresumedLoc for this? I think in the absence of review from @rsmith we should go forward with this.

[probinson]

I had tried to do this in r333311 but some bots complained, so I reverted it and then didn't follow through. Thanks for doing this!
When I tried it, I took advantage of existing tracking of line directives at the file level, so there shouldn't be a need to add a Line flag to PresumedLoc?
What I did was something like this, in computeChecksum():

const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
if (Invalid || !Entry.isFile() || Entry.getFile().hasLineDirectives())
  return None;

[aganea]

Thanks Paul, your solution is even better. I'll apply rL333311 locally - if everything's fine, do you mind if I re-land it again?

[probinson]

In D60283#1480546, @aganea wrote:

Thanks Paul, your solution is even better. I'll apply rL333311 locally - if everything's fine, do you mind if I re-land it again?

I suggest you do *not* use my patch unmodified, because it stops generating any checksums as soon as it finds one preprocessed file. The functionality in your patch seems better, to skip checksums only for those files that look like they were preprocessed. If you want to take my patch and remove the extra flag from CGDebugInfo that "remembers" that we've previously seen a file with line directives, that would be fine.

[thakis]

(Just for the record, I'm happy with whatever y'all end up with.)

[aganea]

Updated again with a different solution. We can't just act on Entry.getFile().hasLineDirectives() because a directive such as #line 100 simply overrides the __LINE__, not __FILE__ (we need to retain and hash the previous __FILE__ in that case). Please see an example here in the IBM documentation.
Also we can't compare by FileID because a LineEntry simply stores a string (called filename) but the buffer for that file isn't stored in the SourceManager.

[probinson]

Minor stuff. This solution is surprisingly simple, the main question being (and I don't have an answer) whether increasing the size of PresumedLoc is okay.

lib/Basic/SourceManager.cpp

1460

The comment would work better as a proper sentence, and on the line before the statement.

lib/CodeGen/CGDebugInfo.cpp

429

While the comment describes the desirable result, it is actually not describing what is happening right here. Maybe something like this:

Compute the checksum if possible. If the location is affected by a #line directive that refers to a file, PLoc will have an invalid FileID, and we will correctly get no checksum.

test/CodeGen/debug-info-file-checksum.c

13

These directives don't need the -LABEL suffix.

[aganea]

In D60283#1503256, @probinson wrote:

Minor stuff. This solution is surprisingly simple, the main question being (and I don't have an answer) whether increasing the size of PresumedLoc is okay.

Thanks Paul! PresumedLoc seems to be used only as a temporary (on the stack).

[aganea]

Ping! Any further comments?

[probinson]

LGTM

include/clang/Basic/SourceLocation.h

 /// to the user.
 ///
 /// A 'presumed' location can be modified by \#line and GNU line marker
 /// directives and is always the expansion point of a normal location.
 ///
 /// You can get a PresumedLoc from a SourceLocation with SourceManager.
 class PresumedLoc {
   const char *Filename = nullptr;
+  FileID ID;
   unsigned Line, Col;
   SourceLocation IncludeLoc;
 
 public:
   PresumedLoc() = default;
-  PresumedLoc(const char *FN, unsigned Ln, unsigned Co, SourceLocation IL)
-      : Filename(FN), Line(Ln), Col(Co), IncludeLoc(IL) {}
+  PresumedLoc(const char *FN, FileID FID, unsigned Ln, unsigned Co,
+              SourceLocation IL)
+      : Filename(FN), ID(FID), Line(Ln), Col(Co), IncludeLoc(IL) {}
 
   /// Return true if this object is invalid or uninitialized.
   ///
   /// This occurs when created with invalid source locations or when walking
   /// off the top of a \#include stack.
   bool isInvalid() const { return Filename == nullptr; }
   bool isValid() const { return Filename != nullptr; }
 
   /// Return the presumed filename of this location.
   ///
   /// This can be affected by \#line etc.
   const char *getFilename() const {
     assert(isValid());
     return Filename;
   }
 
+  FileID getFileID() const {
+    assert(isValid());
+    return ID;
+  }
+
   /// Return the presumed line number of this location.
   ///
   /// This can be affected by \#line etc.
   unsigned getLine() const {
     assert(isValid());
     return Line;
   }
 

lib/Basic/SourceManager.cpp

if (Invalid || !Entry.isFile())
     return PresumedLoc();
 
   const SrcMgr::FileInfo &FI = Entry.getFile();
   const SrcMgr::ContentCache *C = FI.getContentCache();
 
   // To get the source name, first consult the FileEntry (if one exists)
   // before the MemBuffer as this will avoid unnecessarily paging in the
   // MemBuffer.
+  FileID FID = LocInfo.first;
   StringRef Filename;
   if (C->OrigEntry)
     Filename = C->OrigEntry->getName();
   else
     Filename = C->getBuffer(Diag, *this)->getBufferIdentifier();
 
   unsigned LineNo = getLineNumber(LocInfo.first, LocInfo.second, &Invalid);
   if (Invalid)
     return PresumedLoc();
   unsigned ColNo  = getColumnNumber(LocInfo.first, LocInfo.second, &Invalid);
   if (Invalid)
     return PresumedLoc();
 
   SourceLocation IncludeLoc = FI.getIncludeLoc();
 
   // If we have #line directives in this file, update and overwrite the physical
   // location info if appropriate.
   if (UseLineDirectives && FI.hasLineDirectives()) {
     assert(LineTable && "Can't have linetable entries without a LineTable!");
     // See if there is a #line directive before this.  If so, get it.
     if (const LineEntry *Entry =
           LineTable->FindNearestLineEntry(LocInfo.first, LocInfo.second)) {
       // If the LineEntry indicates a filename, use it.
-      if (Entry->FilenameID != -1)
+      if (Entry->FilenameID != -1) {
         Filename = LineTable->getFilename(Entry->FilenameID);
+        FID = FileID::get(0); // contents of files referenced by #line aren't in
+                              // the SourceManager
+      }
 
       // Use the line number specified by the LineEntry.  This line number may
       // be multiple lines down from the line entry.  Add the difference in
       // physical line numbers from the query point and the line marker to the
       // total.
       unsigned MarkerLineNo = getLineNumber(LocInfo.first, Entry->FileOffset);
       LineNo = Entry->LineNo + (LineNo-MarkerLineNo-1);
 
       // Note that column numbers are not molested by line markers.
 
       // Handle virtual #include manipulation.
       if (Entry->IncludeOffset) {
         IncludeLoc = getLocForStartOfFile(LocInfo.first);
         IncludeLoc = IncludeLoc.getLocWithOffset(Entry->IncludeOffset);
       }
     }
   }
 
-  return PresumedLoc(Filename.data(), LineNo, ColNo, IncludeLoc);
+  return PresumedLoc(Filename.data(), FID, LineNo, ColNo, IncludeLoc);
 }
 
 /// Returns whether the PresumedLoc for a given SourceLocation is
 /// in the main file.
 ///
 /// This computes the "presumed" location for a SourceLocation, then checks
 /// whether it came from a file other than the main file. This is different
 /// from isWrittenInMainFile() because it takes line marker directives into

lib/CodeGen/CGDebugInfo.cpp

llvm::DIFile *CGDebugInfo::getOrCreateFile(SourceLocation Loc) {
   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);
   }
 
   SmallString<32> Checksum;
+
+  // Don't compute checksums if the location is affected by a #line directive
+  // that refers to a file. We would otherwise end up with the same checksum for
+  // all the files referred by #line. Instead the checksum remains empty,
+  // leaving to the debugger to open the file without checksum validation.
   Optional<llvm::DIFile::ChecksumKind> CSKind =
-      computeChecksum(SM.getFileID(Loc), Checksum);
+      computeChecksum(PLoc.getFileID(), Checksum);
   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,
                         Optional<llvm::DIFile::ChecksumInfo<StringRef>> CSInfo,
                         Optional<StringRef> Source) {
   StringRef Dir;
   StringRef File;
   std::string RemappedFile = remapDIPath(FileName);
   std::string CurDir = remapDIPath(getCurrentDirname());

test/CodeGen/Inputs/debug-info-file-checksum-line.cpp

+int foo(int x) {
+  vprintf("test %d", x);
+  return x+1;
+}
+
+#line 100
+void test1() {}
+
+#line 200
+void test2() {}

test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp

+#line 1 "F:\\svn\\clang\\test\\CodeGen\\Inputs\\debug-info-file-checksum.c"
+#line 1 "f:\\svn\\clang\\test\\codegen\\inputs\\code-coverage-filter1.h"
+void test1() {}
+#line 2 "F:\\svn\\clang\\test\\CodeGen\\Inputs\\debug-info-file-checksum.c"
+#line 1 "f:\\svn\\clang\\test\\codegen\\inputs\\code-coverage-filter2.h"
+void test2() {}
+#line 3 "F:\\svn\\clang\\test\\CodeGen\\Inputs\\debug-info-file-checksum.c"
+int foo(int x) {
+  return x+1;
+}

test/CodeGen/debug-info-file-checksum.c

 // RUN: %clang -emit-llvm -S -g -gcodeview -x c %S/Inputs/debug-info-file-checksum.c -o - | FileCheck %s
 // RUN: %clang -emit-llvm -S -gdwarf-5 -x c %S/Inputs/debug-info-file-checksum.c -o - | FileCheck %s
 
 // Check that "checksum" is created correctly for the compiled file.
 
 // CHECK: !DIFile(filename:{{.*}}, directory:{{.*}}, checksumkind: CSK_MD5, checksum: "a3b7d27af071accdeccaa933fc603608")
+
+// Ensure #line directives (in already pre-processed files) do not emit checksums
+// RUN: %clang -emit-llvm -S -g -gcodeview -x c %S/Inputs/debug-info-file-checksum-pre.cpp -o - | FileCheck %s --check-prefix NOCHECKSUM
+
+// NOCHECKSUM-LABEL: !DIFile(filename: "{{.*}}code-coverage-filter1.h", directory: "{{[^"]*}}")
+// NOCHECKSUM-LABEL: !DIFile(filename: "{{.*}}code-coverage-filter2.h", directory: "{{[^"]*}}")
+// NOCHECKSUM-LABEL: !DIFile(filename: "{{.*}}debug-info-file-checksum.c", directory: "{{[^"]*}}")
+
+// Ensure #line directives without name do emit checksums
+// RUN: %clang -emit-llvm -S -g -gcodeview -x c %S/Inputs/debug-info-file-checksum-line.cpp -o - | FileCheck %s --check-prefix CHECKSUM
+
+// CHECKSUM: !DIFile(filename: "{{.*}}debug-info-file-checksum-line.cpp", directory:{{.*}}, checksumkind: CSK_MD5, checksum: "7b568574d0e3c56c28e5e0234d1f4a06")