[PATCH] D60283: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-05 Thread Reid Kleckner via Phabricator via cfe-commits 
rnk added a comment.

+@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.


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: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-05 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea 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.


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?


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: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-05 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea added a comment.

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


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: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-05 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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.


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: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-05 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea updated this revision to Diff 193886.
aganea marked 2 inline comments as done.
aganea edited the summary of this revision.
aganea added a comment.

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

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 MSVC 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 "" 1
  # 1 "" 3
  # 361 "" 3
  # 1 "" 1
  # 1 "" 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


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-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 we simply leave the checksum empty, and the debugger will
+  // open the file without checksums.
+  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
===
---

[PATCH] D60283: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-04 Thread Reid Kleckner via Phabricator via cfe-commits 
rnk added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:429-430
+  const FileEntry *fileEntry = SM.getFileEntryForID(foundIdFromLoc);
+  if (!fileEntry || fileEntry->getName().empty() ||
+  fileEntry->getName().equals(FileName)) {
+CSKind = computeChecksum(foundIdFromLoc, Checksum);

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.



Comment at: test/Driver/cl-preprocess-md5.cpp:1
+// RUN: %clang_cl /c %s /Z7 /Fo%t.obj
+// RUN: llvm-pdbutil dump -l %t.obj | grep -E " \(MD5: ([0-9A-F]+)\)" | sed -n 
-e 's/^.*MD5: //p' | sort | uniq | wc -l | FileCheck %s --check-prefix=GOOD-SIZE

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.


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: [clang-cl] Don't emit checksums when compiling a preprocessed CPP

2019-04-04 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea created this revision.
aganea added reviewers: rnk, scott.linder, uabelho, aprantl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.

Fixes PR41215


Repository:
  rC Clang

https://reviews.llvm.org/D60283

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/Driver/cl-preprocess-md5.cpp


Index: test/Driver/cl-preprocess-md5.cpp
===
--- test/Driver/cl-preprocess-md5.cpp
+++ test/Driver/cl-preprocess-md5.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cl /c %s /Z7 /Fo%t.obj
+// RUN: llvm-pdbutil dump -l %t.obj | grep -E " \(MD5: ([0-9A-F]+)\)" | sed -n 
-e 's/^.*MD5: //p' | sort | uniq | wc -l | FileCheck %s --check-prefix=GOOD-SIZE
+// GOOD-SIZE-NOT: 1
+
+// RUN: %clang_cl /c %s /Z7 /E >%t.cpp
+// RUN: %clang_cl /c %t.cpp /Z7 /Fo%t.obj
+// RUN: llvm-pdbutil dump -l %t.obj | not grep -E " \(MD5: ([0-9A-F]+)\)"
+// RUN: llvm-pdbutil dump -l %t.obj | grep " (no checksum)"
+// 
+
+#include 
+int main() {
+  std::string A{"a"};
+  return 0;
+}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -421,13 +421,18 @@
   return cast(V);
   }
 
+  const FileID foundIdFromLoc = SM.getFileID(Loc);
   SmallString<32> Checksum;
-  Optional CSKind =
-  computeChecksum(SM.getFileID(Loc), Checksum);
+  Optional CSKind;
   Optional> CSInfo;
-  if (CSKind)
-CSInfo.emplace(*CSKind, Checksum);
-  return createFile(FileName, CSInfo, getSource(SM, SM.getFileID(Loc)));
+  const FileEntry *fileEntry = SM.getFileEntryForID(foundIdFromLoc);
+  if (!fileEntry || fileEntry->getName().empty() ||
+  fileEntry->getName().equals(FileName)) {
+CSKind = computeChecksum(foundIdFromLoc, Checksum);
+if (CSKind)
+  CSInfo.emplace(*CSKind, Checksum);
+  }
+  return createFile(FileName, CSInfo, getSource(SM, foundIdFromLoc));
 }
 
 llvm::DIFile *


Index: test/Driver/cl-preprocess-md5.cpp
===
--- test/Driver/cl-preprocess-md5.cpp
+++ test/Driver/cl-preprocess-md5.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cl /c %s /Z7 /Fo%t.obj
+// RUN: llvm-pdbutil dump -l %t.obj | grep -E " \(MD5: ([0-9A-F]+)\)" | sed -n -e 's/^.*MD5: //p' | sort | uniq | wc -l | FileCheck %s --check-prefix=GOOD-SIZE
+// GOOD-SIZE-NOT: 1
+
+// RUN: %clang_cl /c %s /Z7 /E >%t.cpp
+// RUN: %clang_cl /c %t.cpp /Z7 /Fo%t.obj
+// RUN: llvm-pdbutil dump -l %t.obj | not grep -E " \(MD5: ([0-9A-F]+)\)"
+// RUN: llvm-pdbutil dump -l %t.obj | grep " (no checksum)"
+// 
+
+#include 
+int main() {
+  std::string A{"a"};
+  return 0;
+}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -421,13 +421,18 @@
   return cast(V);
   }
 
+  const FileID foundIdFromLoc = SM.getFileID(Loc);