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

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361296: [DebugInfo] Dont emit checksums when compiling 
a preprocessed CPP (authored by aganea, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283

Files:
  include/clang/Basic/SourceLocation.h
  lib/Basic/SourceManager.cpp
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
  test/CodeGen/debug-info-file-checksum.c

Index: include/clang/Basic/SourceLocation.h
===
--- include/clang/Basic/SourceLocation.h
+++ include/clang/Basic/SourceLocation.h
@@ -282,13 +282,15 @@
 /// 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.
   ///
@@ -305,6 +307,11 @@
 return Filename;
   }
 
+  FileID getFileID() const {
+assert(isValid());
+return ID;
+  }
+
   /// Return the presumed line number of this location.
   ///
   /// This can be affected by \#line etc.
Index: test/CodeGen/debug-info-file-checksum.c
===
--- test/CodeGen/debug-info-file-checksum.c
+++ test/CodeGen/debug-info-file-checksum.c
@@ -4,3 +4,15 @@
 // 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: !DIFile(filename: "{{.*}}code-coverage-filter1.h", directory: "{{[^"]*}}")
+// NOCHECKSUM: !DIFile(filename: "{{.*}}code-coverage-filter2.h", directory: "{{[^"]*}}")
+// NOCHECKSUM: !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")
Index: test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
@@ -0,0 +1,9 @@
+int foo(int x) {
+  return x+1;
+}
+
+#line 100
+void test1() {}
+
+#line 200
+void test2() {}
Index: test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
@@ -0,0 +1,10 @@
+#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;
+}
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1430,6 +1430,7 @@
   // 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();
@@ -1453,8 +1454,12 @@
 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);
+// The contents of files referenced by #line are not in the
+// SourceManager
+FID = FileID::get(0);
+  }
 
   // Use the line number specified by the LineEntry.  This 

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

[Paul Robinson via Phabricator via cfe-commits]

probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Ping! Any further comments?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 199659.
aganea marked 3 inline comments as done.
aganea added a comment.

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).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283

Files:
  include/clang/Basic/SourceLocation.h
  lib/Basic/SourceManager.cpp
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
  test/CodeGen/debug-info-file-checksum.c

Index: test/CodeGen/debug-info-file-checksum.c
===
--- test/CodeGen/debug-info-file-checksum.c
+++ test/CodeGen/debug-info-file-checksum.c
@@ -4,3 +4,15 @@
 // 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: !DIFile(filename: "{{.*}}code-coverage-filter1.h", directory: "{{[^"]*}}")
+// NOCHECKSUM: !DIFile(filename: "{{.*}}code-coverage-filter2.h", directory: "{{[^"]*}}")
+// NOCHECKSUM: !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")
Index: test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
@@ -0,0 +1,10 @@
+#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;
+}
Index: test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
@@ -0,0 +1,9 @@
+int foo(int x) {
+  return x+1;
+}
+
+#line 100
+void test1() {}
+
+#line 200
+void test2() {}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -422,8 +422,12 @@
   }
 
   SmallString<32> Checksum;
+
+  // 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.
   Optional CSKind =
-  computeChecksum(SM.getFileID(Loc), Checksum);
+  computeChecksum(PLoc.getFileID(), Checksum);
   Optional> CSInfo;
   if (CSKind)
 CSInfo.emplace(*CSKind, Checksum);
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1430,6 +1430,7 @@
   // 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();
@@ -1453,8 +1454,12 @@
 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);
+// The contents of files referenced by #line are not in the
+// SourceManager
+FID = FileID::get(0);
+  }
 
   // Use the line number specified by the LineEntry.  This line number may
   // be multiple lines down from the line entry.  Add the difference in
@@ -1473,7 +1478,7 @@
 }
   }
 
-  return PresumedLoc(Filename.data(), LineNo, ColNo, IncludeLoc);
+  return 

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

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.




Comment at: lib/Basic/SourceManager.cpp:1460
+FID = FileID::get(0); // contents of files referenced by #line aren't 
in
+  // the SourceManager
+  }

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



Comment at: lib/CodeGen/CGDebugInfo.cpp:429
+  // all the files referred by #line. Instead the checksum remains empty,
+  // leaving to the debugger to open the file without checksum validation.
   Optional CSKind =

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.



Comment at: test/CodeGen/debug-info-file-checksum.c:13
+// NOCHECKSUM-LABEL: !DIFile(filename: "{{.*}}code-coverage-filter2.h", 
directory: "{{[^"]*}}")
+// NOCHECKSUM-LABEL: !DIFile(filename: "{{.*}}debug-info-file-checksum.c", 
directory: "{{[^"]*}}")
+

These directives don't need the -LABEL suffix.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 199599.
aganea marked an inline comment as done.
aganea added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283

Files:
  include/clang/Basic/SourceLocation.h
  lib/Basic/SourceManager.cpp
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
  test/CodeGen/debug-info-file-checksum.c

Index: test/CodeGen/debug-info-file-checksum.c
===
--- test/CodeGen/debug-info-file-checksum.c
+++ test/CodeGen/debug-info-file-checksum.c
@@ -4,3 +4,15 @@
 // 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")
Index: test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
@@ -0,0 +1,10 @@
+#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;
+}
Index: test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-line.cpp
@@ -0,0 +1,10 @@
+int foo(int x) {
+  vprintf("test %d", x);
+  return x+1;
+}
+
+#line 100
+void test1() {}
+
+#line 200
+void test2() {}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -422,8 +422,13 @@
   }
 
   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 CSKind =
-  computeChecksum(SM.getFileID(Loc), Checksum);
+  computeChecksum(PLoc.getFileID(), Checksum);
   Optional> CSInfo;
   if (CSKind)
 CSInfo.emplace(*CSKind, Checksum);
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1430,6 +1430,7 @@
   // 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();
@@ -1453,8 +1454,11 @@
 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); 

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D60283#1480546 , @aganea wrote:

> Thanks Paul, your solution is even better. I'll apply rL11 
>  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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I had tried to do this in r11 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  = SM.getSLocEntry(FID, );
  if (Invalid || !Entry.isFile() || Entry.getFile().hasLineDirectives())
return None;


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 196321.
aganea retitled this revision from "[clang-cl] Don't emit checksums when 
compiling a preprocessed CPP" to "[DebugInfo] Don't emit checksums when 
compiling a preprocessed CPP".
aganea added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283

Files:
  include/clang/Basic/SourceLocation.h
  lib/Basic/SourceManager.cpp
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
  test/CodeGen/debug-info-file-checksum.c

Index: test/CodeGen/debug-info-file-checksum.c
===
--- test/CodeGen/debug-info-file-checksum.c
+++ test/CodeGen/debug-info-file-checksum.c
@@ -4,3 +4,11 @@
 // 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: "{{[^"]*}}")
Index: test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
===
--- test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
+++ test/CodeGen/Inputs/debug-info-file-checksum-pre.cpp
@@ -0,0 +1,10 @@
+#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;
+}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -422,11 +422,18 @@
   }
 
   SmallString<32> Checksum;
-  Optional CSKind =
-  computeChecksum(SM.getFileID(Loc), Checksum);
   Optional> CSInfo;
-  if (CSKind)
-CSInfo.emplace(*CSKind, Checksum);
+
+  // Don't compute checksums if the location is affected by a #line directive.
+  // 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.
+  if (!PLoc.isAffectedByLineDirective()) {
+Optional CSKind =
+computeChecksum(SM.getFileID(Loc), Checksum);
+if (CSKind)
+  CSInfo.emplace(*CSKind, Checksum);
+  }
   return createFile(FileName, CSInfo, getSource(SM, SM.getFileID(Loc)));
 }
 
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1444,6 +1444,7 @@
 return PresumedLoc();
 
   SourceLocation IncludeLoc = FI.getIncludeLoc();
+  bool LineDirective = false;
 
   // If we have #line directives in this file, update and overwrite the physical
   // location info if appropriate.
@@ -1470,10 +1471,11 @@
 IncludeLoc = getLocForStartOfFile(LocInfo.first);
 IncludeLoc = IncludeLoc.getLocWithOffset(Entry->IncludeOffset);
   }
+  LineDirective = true;
 }
   }
 
-  return PresumedLoc(Filename.data(), LineNo, ColNo, IncludeLoc);
+  return PresumedLoc(Filename.data(), LineNo, ColNo, IncludeLoc, LineDirective);
 }
 
 /// Returns whether the PresumedLoc for a given SourceLocation is
Index: include/clang/Basic/SourceLocation.h
===
--- include/clang/Basic/SourceLocation.h
+++ include/clang/Basic/SourceLocation.h
@@ -282,13 +282,17 @@
 /// You can get a PresumedLoc from a SourceLocation with SourceManager.
 class PresumedLoc {
   const char *Filename = nullptr;
-  unsigned Line, Col;
+  unsigned Line;
+  unsigned Col : 31;
+  unsigned InLineDirective : 1;
   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, unsigned Ln, unsigned Co, SourceLocation IL,
+  bool InLine)
+  : Filename(FN), Line(Ln), Col(Co), InLineDirective(InLine),
+IncludeLoc(IL) {}