[PATCH] D51380: Fix incorrect usage of std::error_category

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: ilya-biryukov, zturner.
Herald added a subscriber: cfe-commits.

As mentionned here , this change 
prevents a dangling pointer in `std:error_code` by using a singleton.


Repository:
  rC Clang

https://reviews.llvm.org/D51380

Files:
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -742,8 +742,10 @@
   return nullptr;
 }
 
+static llvm::ManagedStatic 
BuildPreambleErrCategory;
+
 std::error_code clang::make_error_code(BuildPreambleError Error) {
-  return std::error_code(static_cast(Error), 
BuildPreambleErrorCategory());
+  return std::error_code(static_cast(Error), *BuildPreambleErrCategory);
 }
 
 const char *BuildPreambleErrorCategory::name() const noexcept {


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -742,8 +742,10 @@
   return nullptr;
 }
 
+static llvm::ManagedStatic BuildPreambleErrCategory;
+
 std::error_code clang::make_error_code(BuildPreambleError Error) {
-  return std::error_code(static_cast(Error), BuildPreambleErrorCategory());
+  return std::error_code(static_cast(Error), *BuildPreambleErrCategory);
 }
 
 const char *BuildPreambleErrorCategory::name() const noexcept {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51380: Fix incorrect usage of std::error_category

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D51380



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

[PATCH] D51380: Fix incorrect usage of std::error_category

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340929: [Preamble] Fix incorrect usage of 
std::error_category (authored by aganea, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D51380

Files:
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -742,8 +742,10 @@
   return nullptr;
 }
 
+static llvm::ManagedStatic 
BuildPreambleErrCategory;
+
 std::error_code clang::make_error_code(BuildPreambleError Error) {
-  return std::error_code(static_cast(Error), 
BuildPreambleErrorCategory());
+  return std::error_code(static_cast(Error), *BuildPreambleErrCategory);
 }
 
 const char *BuildPreambleErrorCategory::name() const noexcept {


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -742,8 +742,10 @@
   return nullptr;
 }
 
+static llvm::ManagedStatic BuildPreambleErrCategory;
+
 std::error_code clang::make_error_code(BuildPreambleError Error) {
-  return std::error_code(static_cast(Error), BuildPreambleErrorCategory());
+  return std::error_code(static_cast(Error), *BuildPreambleErrCategory);
 }
 
 const char *BuildPreambleErrorCategory::name() const noexcept {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: thakis, ddunbar, hans.
Herald added a subscriber: cfe-commits.

Currently, `-march` does not work in the `clang-cl` driver.

We are currently migrating from MSVC. The project size makes that we have to 
stick with `clang-cl` for a little while. However, we don't want to loose 
optimisation oportunities in the runtime code, and the MSVC flag `/arch` only 
supports a limited set of options.


Repository:
  rC Clang

https://reviews.llvm.org/D51806

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, 
Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], 
"mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], 
"mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], "mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], "mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

@hans I want to target precisely a given CPU when using `clang-cl`. The options 
in the `m_x86_Features_Group` are too granular. It is easier to just add 
`-march=skylake` on the cmd-line. Is there any other way?


Repository:
  rC Clang

https://reviews.llvm.org/D51806



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

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 164666.
aganea added a comment.

Updated diff with coverage test as requested.


Repository:
  rC Clang

https://reviews.llvm.org/D51806

Files:
  include/clang/Driver/Options.td
  test/Driver/cl-options.c


Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -610,6 +610,7 @@
 // RUN: -flto \
 // RUN: -fmerge-all-constants \
 // RUN: -no-canonical-prefixes \
+// RUN: -march=skylake \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, 
Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], 
"mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], 
"mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;


Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -610,6 +610,7 @@
 // RUN: -flto \
 // RUN: -fmerge-all-constants \
 // RUN: -no-canonical-prefixes \
+// RUN: -march=skylake \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], "mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], "mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D51806



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

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

@hans Just an after thought: maybe we should prevent usage of `-march=` and 
`/arch:` at the same time. What do you think? I can add another patch for that 
purpose.


Repository:
  rC Clang

https://reviews.llvm.org/D51806



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

[PATCH] D51806: [clang-cl] Enable -march

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC341847: [clang-cl] Enable -march option (authored by aganea, 
committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D51806

Files:
  include/clang/Driver/Options.td
  test/Driver/cl-options.c


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, 
Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], 
"mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], 
"mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -610,6 +610,7 @@
 // RUN: -flto \
 // RUN: -fmerge-all-constants \
 // RUN: -no-canonical-prefixes \
+// RUN: -march=skylake \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1931,7 +1931,7 @@
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, Group;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], "mwatchos-simulator-version-min=">;
 def mwatchsimulator_version_min_EQ : Joined<["-"], "mwatchsimulator-version-min=">, Alias;
-def march_EQ : Joined<["-"], "march=">, Group;
+def march_EQ : Joined<["-"], "march=">, Group, Flags<[CoreOption]>;
 def masm_EQ : Joined<["-"], "masm=">, Group, Flags<[DriverOption]>;
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group;
 def mimplicit_it_EQ : Joined<["-"], "mimplicit-it=">, Group;
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -610,6 +610,7 @@
 // RUN: -flto \
 // RUN: -fmerge-all-constants \
 // RUN: -no-canonical-prefixes \
+// RUN: -march=skylake \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for fixing this @zturner. I simply want to back-up what @probinson says 
above - most of the games we do at Ubisoft are currently using a different 
compilation toolchains for each platform (we ship at least 4-5 platforms for 
each top game). It can be clang or it can be MSVC; and most of the time it's 
different versions of clang. Our long-term goal is to preferably have only one 
toolchain for all our platforms & all our targets - that means one set of 
common g++ like flags, including Windows.


https://reviews.llvm.org/D43700



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Updated tests again. This time I've closed all applications, all unused 
services, and tried to have a "clean" machine.

New timings without the child `clang-cl.exe` being invoked (hacked from 
https://reviews.llvm.org/D52411). The test consists in building Clang+LLVM+LLD 
at r343846 using the compiler specified on each line of the table.

**Config 1:** Intel Xeon Haswell 6 cores / 12 HW threads, 3.5 GHz, 15 MB cache, 
128 GB RAM, SSD 550 MB/s (Windows 10 Fall Creators update 1709)

__Ninja:__

| MSVC 15.8.6 (cl + link)   
  | 29 min 4 sec  |
| clang-cl + lld-link 7.0 official release  
  | 25 min 45 sec |
| clang-cl + lld-link 8.0 r343846 //built with clang-cl 7.0 official// **(no 
child)** | **23 min 27 sec** |
|

**Config 2:** Intel Xeon Skylake 18 cores / 36 HW threads, x2 (Dual CPU), 72 HW 
threads total, 2.3 GHz, 24.75 MB cache each, 128 GB RAM, NVMe SSD 4.6 GB/s 
(Windows 10 Fall Creators update 1709)

__Ninja:__

| MSVC 15.8.6 (cl + link)   
  | 6 min 40 sec |
| clang-cl + lld-link 7.0 official release  
  | 6 min 24 sec |
| clang-cl + lld-link 8.0 r343846 //built with clang-cl 7.0 official// **(no 
child)** | **5 min 10 sec** |
|

I'm wondering if the improvement is of the same order on Linux. I'd be 
interested to see your build times, out of curiosity? Can any of you try it 
with and without https://reviews.llvm.org/D52411?


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In https://reviews.llvm.org/D52193#1260862, @zturner wrote:

> I can try to get some timings from my machine.  How do we handle crash
>  recovery in the case where we don't spawn a child process?  I thought the
>  whole reason for spawning the cc1 driver as a separate process was so that
>  we could collect and report crash information in a nice way.  Not having
>  that seems like a heavy price to pay.


`clang.exe` has already a general exception handler (installed by 
`InitLLVM::InitLLVM/sys::PrintStackTraceOnErrorSignal`), which prints the 
callstack in case of a crash. So you wouldn't need the child process to do 
post-crash processing.

However, when running within a parent `clang.exe`, the parents calls 
`Driver::generateCompilationDiagnostics()` after a crash to create the repro 
(preprocessed source).

In a scenario where a single `clang.exe` is running (no child) we could modify 
`Driver::generateCompilationDiagnostics()` so it gets called from within the 
exception handler, and invoke a child process to collect the preprocessed 
source.

I wrote a fully fledged crash reporting system which does that, so I know 
that's possible. The tricky thing is to ensure 
`Driver::generateCompilationDiagnostics()` doesn't touch potentially invalid 
structures (if a crash occured) or allocate memory. The way around that is to 
pre-allocate and pre-copy the structures that you will be accessing in the case 
of a crash. But I don't see it as a hurdle in this case.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template  class CVRecord {

To add to what you said in a comment above, do you think that if we could add 
`assert(Data.size() == ((RecordPrefix *)RecordData.data())->RecordPrefix + 2)` 
at relevant places below; and then after a while we could simply switch to 
`RecordPrefix *`, once issues are ironed out?



Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+RecordPrefix *Prefix = reinterpret_cast(&PreBytes[0]);
+Prefix->RecordLen = 0;
+Prefix->RecordKind = uint16_t(Sym.Kind);

rnk wrote:
> aganea wrote:
> > Should be 2.
> I could say 2, but this is the seralizer, so really it just gets overwritten. 
> Do you think I should leave it uninitialized, 2, -1, 0?
I'd say "2" because you want as much as possible to keep data coherent in time. 
Regardless of what comes after. I think the code should be correct before being 
optimal. If someone looks for `RecordPrefix` and then tries to understand how 
it works, it'd be nice if the code displays the same, correct, behavior 
everywhere.

But then, you can argue that `RecordPrefix.RecordLen` is simply a constrait, 
tied to the amount data that comes after. And maybe the calculation logic for 
that `.RecordLen` field should be hidden inside the `RecordPrefix` class. In 
that case, to avoid screwing the init order, ideally you could simply say:
```
RecordPrefix Prefix{uint16_t(Sym.Kind)};
CVSymbol Result(&Prefix);
```
...and let `RecordPrefix` (or `CVSymbol`) handle sizing?



Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h:125
+Pre.RecordKind = uint16_t(TypeLeafKind::LF_FIELDLIST);
+CVType FieldList(&Pre, sizeof(Pre));
 consumeError(Mapping.Mapping.visitTypeEnd(FieldList));

Applying what I said above would give:
```
RecordPrefix Pre{uint16_t(TypeLeafKind::LF_FIELDLIST)};
CVType FieldList(&Pre);
```



Comment at: llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp:37
 
+  CVType CVT(ArrayRef(ScratchBuffer.data(), sizeof(RecordPrefix)));
   cantFail(Mapping.visitTypeBegin(CVT));

Here it is the same thing: `writeRecordPrefix()` writes a `.RecordLen = 0`, but 
it could very well write 2 to be coherent, then you would be able to //trust// 
the `RecordPrefix *`.



Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+  static CVSymbol Tombstone(
+  ArrayRef(DenseMapInfo::getTombstoneKey(), 1));
   return Tombstone;

I suppose you've chosen 1 because that's a invalid record size? Comment maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template  class CVRecord {

aganea wrote:
> To add to what you said in a comment above, do you think that if we could add 
> `assert(Data.size() == ((RecordPrefix *)RecordData.data())->RecordPrefix + 
> 2)` at relevant places below; and then after a while we could simply switch 
> to `RecordPrefix *`, once issues are ironed out?
I didn't mean now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via cfe-commits]

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

LGTM. With a few minor comments:




Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h:122
   ~FieldListDeserializer() override {
-CVType FieldList;
-FieldList.Type = TypeLeafKind::LF_FIELDLIST;
+RecordPrefix Pre;
+Pre.RecordLen = 2;

`RecordPrefix Pre(TypeLeafKind::LF_FIELDLIST);` like the other places? And 
above.



Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+  static CVSymbol Tombstone(
+  ArrayRef(DenseMapInfo::getTombstoneKey(), 1));
   return Tombstone;

rnk wrote:
> aganea wrote:
> > I suppose you've chosen 1 because that's a invalid record size? Comment 
> > maybe?
> Actually, the reason I did this was to avoid ambiguity between the `T* begin, 
> T* end` ctor and the `T* base, size_t count` ctor. If I use zero, it's 
> ambiguous. Implicit null strikes again. :)
Oh I see. Why not `static CVSymbol 
Tombstone(DenseMapInfo>::getTombstoneKey());` then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



___
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

[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);
   Small

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

[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
===
--- lib/Basic/Sour

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

[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

[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] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild 
should issue two different calls to clang-cl, one with /Yc and one /Yu /MP. Can 
you check with ProcMon that commands are issued in the right order?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52193



___
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) {}
 
   

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: spatel, rnk, nikic, chandlerc, MaskRay, martong, ioeric.
aganea added projects: LLVM, clang, lld.
Herald added subscribers: rnkovacs, arichardson, mgorny, nhaehnle, jvesely, 
mehdi_amini, emaste, arsenm.
Herald added a reviewer: espindola.
Herald added a reviewer: shafik.

This fixes some warnings when compiling Clang & LLD on a out-of-the-box 
WSL/Ubuntu 18.04 shell.


https://reviews.llvm.org/D61046

Files:
  clang/trunk/unittests/AST/ASTImporterTest.cpp
  clang/trunk/unittests/Tooling/LookupTest.cpp
  lld/trunk/ELF/LinkerScript.cpp
  llvm/trunk/include/llvm/BinaryFormat/ELF.h
  llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
  llvm/trunk/unittests/IR/ConstantRangeTest.cpp
  llvm/trunk/unittests/Support/TypeTraitsTest.cpp
  llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
  llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

Index: llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
===
--- llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
+++ llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
@@ -20,19 +20,9 @@
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
Index: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
===
--- llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
+++ llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
@@ -10,3 +10,8 @@
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 6.1)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
Index: llvm/trunk/unittests/Support/TypeTraitsTest.cpp
===
--- llvm/trunk/unittests/Support/TypeTraitsTest.cpp
+++ llvm/trunk/unittests/Support/TypeTraitsTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
@@ -71,6 +72,26 @@
 template void TrivialityTester();
 template void TrivialityTester();
 
+TEST(Triviality, Tester) {
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace
Index: llvm/trunk/unittests/IR/ConstantRangeTest.cpp
===
--- llvm/trunk/unittests/IR/ConstantRangeTest.cpp
+++ llvm/trunk/unittests/IR/ConstantRangeTest.cpp
@@ -395,6 +395,10 @@
   }
 }
 
+#if defined(__GNUC__) && __GNUC__ >= 7
+// Silence warning: variable 'HaveInterrupt3' set but not used
+(void)&HaveInterrupt3;
+#endif
 assert(!HaveInterrupt3 && "Should have at most three ranges");
 
 ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
Index: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
===
--- llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
+++ llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
@@ -1714,6 +1714,10 @@
 
 if (NewBldVec[i].isUndef())
   continue;
+// Fix spurious warning with gcc 7.3 -O3 for NewBldVec[i] below
+// warning: array subscript is above array bounds [-Warray-bounds]
+if (i >= 4)
+  continue;
 for (unsigned j = 0; j < i; j++) {
   if (NewBldVec[i] == NewBldVec[j]) {
 NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
Index: llvm/trunk/include/llvm/BinaryFormat/ELF.h
===
--- llvm/trunk/include/llvm/BinaryFormat/ELF.h
+++ llvm/trunk/include/llvm/BinaryFormat/ELF.h
@@ -875,6 +875,8 @@
 
 // Section flags.
 enum : unsigned {
+  SHF_NONE,
+
   // Section data should be writable during execution.
   SHF_WRITE = 0x1,
 
Index: lld/trunk/ELF/LinkerScript.cpp
===
--- lld/trunk/ELF/LinkerScript.cpp
+++ lld/trunk/ELF/LinkerScript.cpp
@@ -887,8 +887,8 @@
 // in case it is empty.
 bool IsEmpty = getInp

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 5 inline comments as done.
aganea added inline comments.



Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+}
   }
 

Fixes
```
[2097/2979] Building CXX object 
tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
/mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
/mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: suggest 
explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
 if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
^
```



Comment at: clang/trunk/unittests/Tooling/LookupTest.cpp:222
   EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+}
   };

Fixes
```
[2107/2979] Building CXX object 
tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterTest.cpp.o
/mnt/f/svn/clang/unittests/AST/ASTImporterTest.cpp: In member function ‘void 
clang::ast_matchers::RedeclChain::TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition()’:
/mnt/f/svn/clang/unittests/AST/ASTImporterTest.cpp:4051:8: warning: suggest 
explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
 if (auto *ToT = dyn_cast(ToD))
^
```



Comment at: lld/trunk/ELF/LinkerScript.cpp:892
+SHF_EXECINSTR);
 
 if (IsEmpty && isDiscardable(*Sec)) {

Fixes
```
[2238/2979] Building CXX object 
tools/lld/ELF/CMakeFiles/lldELF.dir/LinkerScript.cpp.o
/mnt/f/svn/lld/ELF/LinkerScript.cpp: In member function ‘void 
lld::elf::LinkerScript::adjustSectionsBeforeSorting()’:
/mnt/f/svn/lld/ELF/LinkerScript.cpp:891:35: warning: enumeral and non-enumeral 
type in conditional expression [-Wextra]
   Flags & ((Sec->NonAlloc ? 0 : SHF_ALLOC) | SHF_WRITE | 
SHF_EXECINSTR);
 ~~^~~
```



Comment at: llvm/trunk/unittests/Support/TypeTraitsTest.cpp:95
+
 } // namespace triviality
 

Fixes a bunch of
```
[2828/2979] Building CXX object 
unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o
/mnt/f/svn/llvm/unittests/Support/TypeTraitsTest.cpp:20:6: warning: ‘void 
{anonymous}::triviality::TrivialityTester() [with T = 
{anonymous}::triviality::B&&; bool IsTriviallyCopyConstructible = false; bool 
IsTriviallyMoveConstructible = true]’ defined but not used [-Wunused-function]
 void TrivialityTester() {
  ^~~~
```



Comment at: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt:17
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS 
-Wno-unused-function)
+endif()

Fixes
```
[2894/2979] Building CXX object 
unittests/Transforms/Scalar/CMakeFiles/ScalarTests.dir/LoopPassManagerTest.cpp.o
In file included from 
/mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43:0,
 from 
/mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
 from 
/mnt/f/svn/llvm/unittests/Transforms/Scalar/LoopPassManagerTest.cpp:29:
/mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:896:11:
 warning: ‘testing::internal::TypedExpectation::~TypedExpectation() noexcept 
[with F = 
{anonymous}::MockAnalysisHandleBase<{anonymous}::MockFunctionAnalysisHandle, 
llvm::Function>::Analysis::Result(llvm::Function&, 
llvm::AnalysisManager&)]’ declared ‘static’ but never defined 
[-Wunused-function]
   virtual ~TypedExpectation() {
   ^
```



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

https://reviews.llvm.org/D61046



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

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 196358.

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

https://reviews.llvm.org/D61046

Files:
  clang/trunk/unittests/AST/ASTImporterTest.cpp
  clang/trunk/unittests/Tooling/LookupTest.cpp
  lld/trunk/ELF/LinkerScript.cpp
  llvm/trunk/include/llvm/BinaryFormat/ELF.h
  llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
  llvm/trunk/unittests/IR/ConstantRangeTest.cpp
  llvm/trunk/unittests/Support/TypeTraitsTest.cpp
  llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
  llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

Index: llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
===
--- llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
+++ llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
@@ -20,19 +20,9 @@
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
Index: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
===
--- llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
+++ llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
@@ -10,3 +10,8 @@
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
Index: llvm/trunk/unittests/Support/TypeTraitsTest.cpp
===
--- llvm/trunk/unittests/Support/TypeTraitsTest.cpp
+++ llvm/trunk/unittests/Support/TypeTraitsTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
@@ -71,6 +72,26 @@
 template void TrivialityTester();
 template void TrivialityTester();
 
+TEST(Triviality, Tester) {
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace
Index: llvm/trunk/unittests/IR/ConstantRangeTest.cpp
===
--- llvm/trunk/unittests/IR/ConstantRangeTest.cpp
+++ llvm/trunk/unittests/IR/ConstantRangeTest.cpp
@@ -395,6 +395,10 @@
   }
 }
 
+#if defined(__GNUC__) && __GNUC__ >= 7
+// Silence warning: variable 'HaveInterrupt3' set but not used
+(void)&HaveInterrupt3;
+#endif
 assert(!HaveInterrupt3 && "Should have at most three ranges");
 
 ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
Index: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
===
--- llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
+++ llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
@@ -1714,6 +1714,12 @@
 
 if (NewBldVec[i].isUndef())
   continue;
+// Fix spurious warning with gcc 7.3 -O3 for NewBldVec[i] below
+// warning: array subscript is above array bounds [-Warray-bounds]
+#if defined(__GNUC__) && __GNUC__ >= 7 && __GNUC_MINOR__ >= 3
+if (i >= 4)
+  continue;
+#endif
 for (unsigned j = 0; j < i; j++) {
   if (NewBldVec[i] == NewBldVec[j]) {
 NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
Index: llvm/trunk/include/llvm/BinaryFormat/ELF.h
===
--- llvm/trunk/include/llvm/BinaryFormat/ELF.h
+++ llvm/trunk/include/llvm/BinaryFormat/ELF.h
@@ -875,6 +875,8 @@
 
 // Section flags.
 enum : unsigned {
+  SHF_NONE,
+
   // Section data should be writable during execution.
   SHF_WRITE = 0x1,
 
Index: lld/trunk/ELF/LinkerScript.cpp
===
--- lld/trunk/ELF/LinkerScript.cpp
+++ lld/trunk/ELF/LinkerScript.cpp
@@ -887,8 +887,8 @@
 // in case it is empty.
 bool IsEmpty = getInputSections(Sec).empty();
 if (IsEmpty)
-  Sec->Flags =
-  Flags & ((Sec->NonAlloc ? 0 : SHF_ALLOC) | SHF_WRITE | SHF_EXECINSTR);
+  Sec->Flags = Flags & ((Sec->NonAlloc ? SHF_NONE : SHF_ALLOC) | SHF_WRITE |
+  

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 196505.
aganea marked 7 inline comments as done.
aganea added a comment.

Adressed some of the comments.


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

https://reviews.llvm.org/D61046

Files:
  clang/trunk/unittests/AST/ASTImporterTest.cpp
  clang/trunk/unittests/Tooling/LookupTest.cpp
  llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
  llvm/trunk/unittests/IR/ConstantRangeTest.cpp
  llvm/trunk/unittests/Support/TypeTraitsTest.cpp
  llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
  llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

Index: llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
===
--- llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
+++ llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
@@ -20,19 +20,9 @@
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
Index: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
===
--- llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
+++ llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
@@ -10,3 +10,8 @@
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
Index: llvm/trunk/unittests/Support/TypeTraitsTest.cpp
===
--- llvm/trunk/unittests/Support/TypeTraitsTest.cpp
+++ llvm/trunk/unittests/Support/TypeTraitsTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
@@ -71,6 +72,26 @@
 template void TrivialityTester();
 template void TrivialityTester();
 
+TEST(Triviality, Tester) {
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace
Index: llvm/trunk/unittests/IR/ConstantRangeTest.cpp
===
--- llvm/trunk/unittests/IR/ConstantRangeTest.cpp
+++ llvm/trunk/unittests/IR/ConstantRangeTest.cpp
@@ -459,6 +459,8 @@
   }
 }
 
+// Silence warning: variable 'HaveInterrupt3' set but not used
+(void)HaveInterrupt3;
 assert(!HaveInterrupt3 && "Should have at most three ranges");
 
 ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
Index: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
===
--- llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
+++ llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
@@ -1714,6 +1714,12 @@
 
 if (NewBldVec[i].isUndef())
   continue;
+// Fix spurious warning with gcc 7.3 -O3
+//warning: array subscript is above array bounds [-Warray-bounds]
+//if (NewBldVec[i] == NewBldVec[j]) {
+//~~~^
+if (i >= 4)
+  continue;
 for (unsigned j = 0; j < i; j++) {
   if (NewBldVec[i] == NewBldVec[j]) {
 NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
Index: clang/trunk/unittests/Tooling/LookupTest.cpp
===
--- clang/trunk/unittests/Tooling/LookupTest.cpp
+++ clang/trunk/unittests/Tooling/LookupTest.cpp
@@ -217,8 +217,9 @@
   // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
   // it's not visible at [0].
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
-if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old") {
   EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+}
   };
   Visitor.runOver(R"(
 // a.h
Index: clang/trunk/unittests/AST/ASTImporterTest.cpp
===
--- clang/trunk/unittests/AST/ASTImporterTest.cpp
+++ clang/trunk/unittests/AST/ASTImporterTest.cpp
@@ -4048,8 +4048,9 @@
 auto *To

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp:1717-1722
+// Fix spurious warning with gcc 7.3 -O3 for NewBldVec[i] below
+// warning: array subscript is above array bounds [-Warray-bounds]
+#if defined(__GNUC__) && __GNUC__ >= 7 && __GNUC_MINOR__ >= 3
+if (i >= 4)
+  continue;
+#endif

tstellar wrote:
> We don't want to  have ifdefs for specific compilers, can this be fixed 
> another way or dropped from the patch?
Removed ifdefs, but I kept the test. This looks like an optimizer issue in GCC, 
however the regression tests are unaffected by this change. I will c-reduce and 
report it if it's still there in the latest GCC.



Comment at: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt:14-17
+# Workaround for the gcc 6.1 bug 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 
6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS 
-Wno-unused-function)
+endif()

tstellar wrote:
> Same thing here too.
Not sure I understand, there're plenty of compiler-specific examples in the 
.cmake files. Is there an alternate way to fix this?


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

https://reviews.llvm.org/D61046



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

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 3 inline comments as done.
aganea added inline comments.



Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+}
   }
 

rnk wrote:
> nikic wrote:
> > shafik wrote:
> > > aganea wrote:
> > > > Fixes
> > > > ```
> > > > [2097/2979] Building CXX object 
> > > > tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
> > > > /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
> > > > /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: 
> > > > suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
> > > >  if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
> > > > ^
> > > > ```
> > > You mixed up the error messages but I see what is going on.
> > > 
> > > So you may want to add a comment since it is not apparent that what is 
> > > going on is due the `EXPECT_TRUE` macro eventually expanding to an 
> > > `if..else` which is what is triggering the warning. Since someone may 
> > > come by in the future and just remove the braces since it is not apparent 
> > > why they are there.
> > > 
> > > Same below as well.
> > EXPECT_* inside if is quite common, I don't think we should add a comment 
> > every time it is used.
> This is a longstanding issue. gtest even includes this beautiful snippet of 
> code:
> 
> ```
> // The GNU compiler emits a warning if nested "if" statements are followed by
> // an "else" statement and braces are not used to explicitly disambiguate the
> // "else" binding.  This leads to problems with code like:
> //
> //   if (gate)
> // ASSERT_*(condition) << "Some message";
> //
> // The "switch (0) case 0:" idiom is used to suppress this.
> #ifdef __INTEL_COMPILER
> # define GTEST_AMBIGUOUS_ELSE_BLOCKER_
> #else
> # define GTEST_AMBIGUOUS_ELSE_BLOCKER_ switch (0) case 0: default:  // NOLINT
> #endif
> ```
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h#L834
> 
> The expansion looks something like this:
> ```
> if (user_cond)
>   switch (0) case 0: default:
> if (const TestResult res = ...);
> else fail(res, ...) << "User streaming can follow"
> ```
> 
> It seems new versions of GCC have gotten "smarter" and this pattern no longer 
> suppresses the warning as desired. It might be worth disabling 
> -Wdangling-else for GCC at this point, since this will be an ongoing problem 
> because LLVM typically elides braces when possible.
> 
> Oh, we even reported it back in 2017:
> https://github.com/google/googletest/issues/1119
> ... and it was closed as inactive. Woohoo. :(
So revert those braces and disable -Wdangling-else if CMAKE_COMPILER_IS_GNUCXX 
? (somewhere in HandleLLVMOptions.cmake)



Comment at: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt:14-17
+# Workaround for the gcc 6.1 bug 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 
6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS 
-Wno-unused-function)
+endif()

tstellar wrote:
> aganea wrote:
> > tstellar wrote:
> > > Same thing here too.
> > Not sure I understand, there're plenty of compiler-specific examples in the 
> > .cmake files. Is there an alternate way to fix this?
> I know there are some, but I don't think a warning like this is important 
> enough to fix with a compiler specific work-around.
You mean more like
```
# Workaround for the gcc 6.1 bug 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
if (NOT MSVC)
  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS 
-Wno-unused-function)
endif()
```
Or leave the warning? There's already an #ifdef below in 
LoopPassManagerTest.cpp but it's not at the right place, this simply corrects 
it.


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

https://reviews.llvm.org/D61046



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

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt:14-17
+# Workaround for the gcc 6.1 bug 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 
6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS 
-Wno-unused-function)
+endif()

tstellar wrote:
> aganea wrote:
> > tstellar wrote:
> > > aganea wrote:
> > > > tstellar wrote:
> > > > > Same thing here too.
> > > > Not sure I understand, there're plenty of compiler-specific examples in 
> > > > the .cmake files. Is there an alternate way to fix this?
> > > I know there are some, but I don't think a warning like this is important 
> > > enough to fix with a compiler specific work-around.
> > You mean more like
> > ```
> > # Workaround for the gcc 6.1 bug 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
> > if (NOT MSVC)
> >   set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES 
> > COMPILE_FLAGS -Wno-unused-function)
> > endif()
> > ```
> > Or leave the warning? There's already an #ifdef below in 
> > LoopPassManagerTest.cpp but it's not at the right place, this simply 
> > corrects it.
> Ok, I see in this case you are just moving the ifdef out of the code and into 
> CMake.  I really don't like either approach, but I guess this is fine.  I 
> would limit the work-around to only known broken compilers though.  It looks 
> like this might be fixed in gcc 9.
Scoping this issue under GCC version range [6.1, 9.0)


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

https://reviews.llvm.org/D61046



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

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

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

Please let me know if other changes are needed.


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

https://reviews.llvm.org/D61046

Files:
  clang/trunk/unittests/AST/ASTImporterTest.cpp
  clang/trunk/unittests/Tooling/LookupTest.cpp
  llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
  llvm/trunk/unittests/IR/ConstantRangeTest.cpp
  llvm/trunk/unittests/Support/TypeTraitsTest.cpp
  llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
  llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

Index: llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
===
--- llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
+++ llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
@@ -20,19 +20,9 @@
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
Index: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
===
--- llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
+++ llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
@@ -10,3 +10,8 @@
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
Index: llvm/trunk/unittests/Support/TypeTraitsTest.cpp
===
--- llvm/trunk/unittests/Support/TypeTraitsTest.cpp
+++ llvm/trunk/unittests/Support/TypeTraitsTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
@@ -71,6 +72,26 @@
 template void TrivialityTester();
 template void TrivialityTester();
 
+TEST(Triviality, Tester) {
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace
Index: llvm/trunk/unittests/IR/ConstantRangeTest.cpp
===
--- llvm/trunk/unittests/IR/ConstantRangeTest.cpp
+++ llvm/trunk/unittests/IR/ConstantRangeTest.cpp
@@ -459,6 +459,8 @@
   }
 }
 
+// Silence warning: variable 'HaveInterrupt3' set but not used
+(void)HaveInterrupt3;
 assert(!HaveInterrupt3 && "Should have at most three ranges");
 
 ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
Index: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
===
--- llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
+++ llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
@@ -1714,6 +1714,12 @@
 
 if (NewBldVec[i].isUndef())
   continue;
+// Fix spurious warning with gcc 7.3 -O3
+//warning: array subscript is above array bounds [-Warray-bounds]
+//if (NewBldVec[i] == NewBldVec[j]) {
+//~~~^
+if (i >= 4)
+  continue;
 for (unsigned j = 0; j < i; j++) {
   if (NewBldVec[i] == NewBldVec[j]) {
 NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
Index: clang/trunk/unittests/Tooling/LookupTest.cpp
===
--- clang/trunk/unittests/Tooling/LookupTest.cpp
+++ clang/trunk/unittests/Tooling/LookupTest.cpp
@@ -217,8 +217,9 @@
   // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
   // it's not visible at [0].
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
-if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old") {
   EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+}
   };
   Visitor.runOver(R"(
 // a.h
Index: clang/trunk/unittests/AST/ASTImporterTest.cpp
===
--- clang/trunk/unittests/AST/ASTImporterTest.cpp
+++ clang/trunk/unit

[PATCH] D42642: [CUDA] Detect installation in PATH

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.
Herald added a project: LLVM.

Just a quick heads-up - the `cuda-detect-path.cu` test fails on WSL/Ubuntu 
18.04:

  aganea@MTL-BJ842:/mnt/f/svn/buildWSL$ /usr/bin/python3.6 bin/llvm-lit -vv 
../clang/test/Driver/cuda-detect-path.cu
  llvm-lit: /mnt/f/svn/llvm/utils/lit/lit/llvm/config.py:341: note: using 
clang: /mnt/f/svn/buildWSL/bin/clang
  -- Testing: 1 tests, single process --
  FAIL: Clang :: Driver/cuda-detect-path.cu (1 of 1)
   TEST 'Clang :: Driver/cuda-detect-path.cu' FAILED 

  
  (output chopped)
  
  + env PATH=/mnt/f/svn/clang/test/Driver/Inputs/CUDA-symlinks/usr/bin 
/mnt/f/svn/buildWSL/bin/clang -v --target=i386-unknown-linux 
--sysroot=/mnt/f/svn/clang/test/Driver/no-cuda-there
  + /mnt/f/svn/buildWSL/bin/FileCheck 
/mnt/f/svn/clang/test/Driver/cuda-detect-path.cu --check-prefix SYMLINKS
  /mnt/f/svn/clang/test/Driver/cuda-detect-path.cu:82:14: error: SYMLINKS: 
expected string not found in input
  // SYMLINKS: Found CUDA installation: {{.*}}/Inputs/CUDA-symlinks/opt/cuda
   ^
  :1:1: note: scanning from here
  clang version 9.0.0 (trunk)
  ^
  :4:1: note: possible intended match here
  InstalledDir: /mnt/f/svn/buildWSL/bin
  ^
  
  aganea@MTL-BJ842:/mnt/f/svn/buildWSL$ env 
PATH=/mnt/f/svn/clang/test/Driver/Inputs/CUDA-symlinks/usr/bin 
/mnt/f/svn/buildWSL/bin/clang -v --target=i386-unknown-linux 
--sysroot=/mnt/f/svn/clang/test/Driver/no-cuda-there
  clang version 9.0.0 (trunk)
  Target: i386-unknown-linux
  Thread model: posix
  InstalledDir: /mnt/f/svn/buildWSL/bin
  aganea@MTL-BJ842:/mnt/f/svn/buildWSL$ ls -l 
/mnt/f/svn/clang/test/Driver/Inputs/CUDA-symlinks/usr/bin
  total 0
  -rwxrwxrwx 1 aganea aganea 29 Feb  1  2018 ptxas


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42642



___
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] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Ping! Is this good to go?


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

https://reviews.llvm.org/D61046



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

[PATCH] D42642: [CUDA] Detect installation in PATH

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a subscriber: rnk.
aganea added a comment.

So it turns out this is a symlink issue. The file 
`clang/trunk/test/Driver/Inputs/CUDA-symlinks/usr/bin/ptxas` has been 
synchronized on my Windows 10 PC as a regular text file, not a symlink. It 
looks like TortoiseSVN doesn't implement symlinks. As WSL inherits of my file 
system, it will not find the symbolic link. I suppose `REQUIRES: 
!system-windows` isn't enough for `cuda-detect-path.cu`, and it would need 
something like `REQUIRES: symlinks` along with support in lit. @rnk


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42642



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

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: rnk, jlebar.
aganea added a project: clang.
Herald added a subscriber: mstorsjo.
aganea edited the summary of this revision.

The files below have been synced by Tortoise as CRLF, and it looks like they 
are like that in the depot as well (I'm still using the SVN setup)
Strangely `svn diff` shows them as LF.
The issue is that `grep 'something$'` doesn't match the newline inside the 
MINGW32 bash (mintty).


Repository:
  rC Clang

https://reviews.llvm.org/D61496

Files:
  clang/trunk/test/Preprocessor/indent_macro.c
  clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
  clang/trunk/test/Preprocessor/macro_not_define.c
  clang/trunk/test/Preprocessor/macro_rparen_scan.c


Index: clang/trunk/test/Preprocessor/macro_rparen_scan.c
===
--- clang/trunk/test/Preprocessor/macro_rparen_scan.c
+++ clang/trunk/test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^3 ;$'
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: clang/trunk/test/Preprocessor/macro_not_define.c
===
--- clang/trunk/test/Preprocessor/macro_not_define.c
+++ clang/trunk/test/Preprocessor/macro_not_define.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^ # define X 3$'
 
 #define H # 
  #define D define 
Index: clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
===
--- clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
+++ clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^a: x$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^b: x y, z,h$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^c: foo(x)$'
 
 #define A(b, c...) b c
 a: A(x)
Index: clang/trunk/test/Preprocessor/indent_macro.c
===
--- clang/trunk/test/Preprocessor/indent_macro.c
+++ clang/trunk/test/Preprocessor/indent_macro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^   zzap$'
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap


Index: clang/trunk/test/Preprocessor/macro_rparen_scan.c
===
--- clang/trunk/test/Preprocessor/macro_rparen_scan.c
+++ clang/trunk/test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^3 ;$'
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: clang/trunk/test/Preprocessor/macro_not_define.c
===
--- clang/trunk/test/Preprocessor/macro_not_define.c
+++ clang/trunk/test/Preprocessor/macro_not_define.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^ # define X 3$'
 
 #define H # 
  #define D define 
Index: clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
===
--- clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
+++ clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^a: x$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^b: x y, z,h$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^c: foo(x)$'
 
 #define A(b, c...) b c
 a: A(x)
Index: clang/trunk/test/Preprocessor/indent_macro.c
===
--- clang/trunk/test/Preprocessor/indent_macro.c
+++ clang/trunk/test/Preprocessor/indent_macro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | dos2unix | grep '^   zzap$'
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D61496#1489523 , @probinson wrote:

> Checked-in files should not have CRLF endings, in general (there are a very 
> few exceptions, and these aren't among them).
>  If the checked-in files have LF endings but your tool checks them out with 
> CRLF then that is a problem with your config, or with the tool.
>  Adding dos2unix to the RUN lines is the wrong fix, either way.


Yeah I've realized that :) This is caused by svn on Windows which sets 
`svn:eol-style=native` by default, which causes local files to be saved in the 
"native" format, which is CRLF on Windows (even though in upstream they are LF) 
When files are commited, normally svn should convert them back to LF (or at 
least that's the contract 
 
for `svn:eol-style=native`).

  native
  This causes the file to contain the EOL markers that are native to the 
operating system on which Subversion was run. In other words, if a user on a 
Windows machine checks out a working copy that contains a file with an 
svn:eol-style property set to native, that file will contain CRLF EOL markers. 
A Unix user checking out a working copy that contains the same file will see LF 
EOL markers in his copy of the file.
  
  Note that Subversion will actually store the file in the repository using 
normalized LF EOL markers regardless of the operating system. This is basically 
transparent to the user, though.

I'm using the MINGW32 bash to run the tests/`check-all` because it comes with 
all the Unix tools (grep, sed, ...) that Windows doesn't have.
The bug here is that the GNU grep only matches LF, not CRLF.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61496



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

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 197990.
aganea added a comment.

Changed to use `FileCheck` instead of `grep`


Repository:
  rC Clang

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

https://reviews.llvm.org/D61496

Files:
  clang/trunk/test/Preprocessor/indent_macro.c
  clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
  clang/trunk/test/Preprocessor/macro_not_define.c
  clang/trunk/test/Preprocessor/macro_rparen_scan.c


Index: clang/trunk/test/Preprocessor/macro_rparen_scan.c
===
--- clang/trunk/test/Preprocessor/macro_rparen_scan.c
+++ clang/trunk/test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:3 ;
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: clang/trunk/test/Preprocessor/macro_not_define.c
===
--- clang/trunk/test/Preprocessor/macro_not_define.c
+++ clang/trunk/test/Preprocessor/macro_not_define.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK: # define X 3
 
 #define H # 
  #define D define 
Index: clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
===
--- clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
+++ clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix 1
+// 1:a: x
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix 2
+// 2:b: x y, z,h
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix 3
+// 3:c: foo(x)
 
 #define A(b, c...) b c
 a: A(x)
Index: clang/trunk/test/Preprocessor/indent_macro.c
===
--- clang/trunk/test/Preprocessor/indent_macro.c
+++ clang/trunk/test/Preprocessor/indent_macro.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:   zzap
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap


Index: clang/trunk/test/Preprocessor/macro_rparen_scan.c
===
--- clang/trunk/test/Preprocessor/macro_rparen_scan.c
+++ clang/trunk/test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:3 ;
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: clang/trunk/test/Preprocessor/macro_not_define.c
===
--- clang/trunk/test/Preprocessor/macro_not_define.c
+++ clang/trunk/test/Preprocessor/macro_not_define.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK: # define X 3
 
 #define H # 
  #define D define 
Index: clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
===
--- clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
+++ clang/trunk/test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix 1
+// 1:a: x
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix 2
+// 2:b: x y, z,h
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix 3
+// 3:c: foo(x)
 
 #define A(b, c...) b c
 a: A(x)
Index: clang/trunk/test/Preprocessor/indent_macro.c
===
--- clang/trunk/test/Preprocessor/indent_macro.c
+++ clang/trunk/test/Preprocessor/indent_macro.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:   zzap
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added a comment.

In D61046#1486850 , @rnk wrote:

> The only remaining change I find questionable is the R600 backend change


creduce'd and reported here: 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63477#c4 - it is still present 
with GCC 9.1


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

https://reviews.llvm.org/D61046



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

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360044: Fix compilation warnings when compiling with GCC 7.3 
(authored by aganea, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61046?vs=196655&id=198265#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61046

Files:
  llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
  llvm/trunk/unittests/IR/ConstantRangeTest.cpp
  llvm/trunk/unittests/Support/TypeTraitsTest.cpp
  llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
  llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

Index: llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
===
--- llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
+++ llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp
@@ -1714,6 +1714,12 @@
 
 if (NewBldVec[i].isUndef())
   continue;
+// Fix spurious warning with gcc 7.3 -O3
+//warning: array subscript is above array bounds [-Warray-bounds]
+//if (NewBldVec[i] == NewBldVec[j]) {
+//~~~^
+if (i >= 4)
+  continue;
 for (unsigned j = 0; j < i; j++) {
   if (NewBldVec[i] == NewBldVec[j]) {
 NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
Index: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
===
--- llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
+++ llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt
@@ -10,3 +10,8 @@
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
Index: llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
===
--- llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
+++ llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp
@@ -20,19 +20,9 @@
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
Index: llvm/trunk/unittests/Support/TypeTraitsTest.cpp
===
--- llvm/trunk/unittests/Support/TypeTraitsTest.cpp
+++ llvm/trunk/unittests/Support/TypeTraitsTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
@@ -71,6 +72,26 @@
 template void TrivialityTester();
 template void TrivialityTester();
 
+TEST(Triviality, Tester) {
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+  TrivialityTester();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace
Index: llvm/trunk/unittests/IR/ConstantRangeTest.cpp
===
--- llvm/trunk/unittests/IR/ConstantRangeTest.cpp
+++ llvm/trunk/unittests/IR/ConstantRangeTest.cpp
@@ -459,6 +459,7 @@
   }
 }
 
+(void)HaveInterrupt3;
 assert(!HaveInterrupt3 && "Should have at most three ranges");
 
 ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added inline comments.



Comment at: clang/trunk/test/Preprocessor/indent_macro.c:1-2
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:   zzap
 

lebedev.ri wrote:
> Separate with newline
What do you mean? Between RUN and CHECK? There's already a newline a L3.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61496



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: rnk, hans, zturner.
Herald added a reviewer: JDevlieghere.
Herald added a subscriber: cfe-commits.

This is very preliminary change which adds support for (`clang-cl`) `/MP` 
(Build with multiple processes). Doc for `/MP` is here 
.

Support for `-j` to the `clang` driver could also be added along the way.

I would simply like a general advice on this change. **I don't plan to commit 
this as it is**. If the general idea seems good, I'll cut it down in several 
smaller pieces.

The change about having explicit return codes (`Program.h / enum 
ProgramReturnCode`) should probably be discussed separately, although if you 
have an opinion about that, please do.

Some timings //(I ran each configuration several time to ensure the figures are 
stable)//

  Build LLVM + Clang + LLD (at r341847) inside VS2017:
  
  (Intel Xeon Haswell 6 cores / 12 HW threads, 3.5 GHz, 15M cache, 128 GB RAM, 
SSD 550 MB/s)
  
  - With VS2017 15.8.3: (56m 43 sec) 2 parallel msbuild
  - With Clang /MP + LLD (trunk r341847) [1]: (51m 8sec) 2 parallel msbuild
  
  
  (Intel Xeon Skylake 18 cores / 36 HW threads, x2 (Dual CPU), 72 HW threads 
total, 2.3 GHz, 24.75M cache, 128 GB RAM, NVMe 4.6 GB/s)
  
  - With VS2017 15.8.3: (12m 8sec) 32 parallel msbuild
  - With Clang /MP + LLD (trunk r341847) [1]: (9m 22sec) 32 parallel msbuild
  
  [1] running clang-cl.exe compiled with VS2017 15.8.3 at r341847

Please add anyone who might want to review this. Many thanks in advance!


Repository:
  rC Clang

https://reviews.llvm.org/D52193

Files:
  clang/trunk/include/clang/Driver/CLCompatOptions.td
  clang/trunk/include/clang/Driver/Compilation.h
  clang/trunk/include/clang/Driver/Driver.h
  clang/trunk/include/clang/Driver/Job.h
  clang/trunk/lib/Driver/Compilation.cpp
  clang/trunk/lib/Driver/Driver.cpp
  clang/trunk/lib/Driver/Job.cpp
  clang/trunk/tools/driver/cc1_main.cpp
  clang/trunk/tools/driver/cc1as_main.cpp
  clang/trunk/tools/driver/cc1gen_reproducer_main.cpp
  clang/trunk/tools/driver/driver.cpp
  llvm/trunk/include/llvm/Support/Program.h
  llvm/trunk/lib/Support/GraphWriter.cpp
  llvm/trunk/lib/Support/Program.cpp
  llvm/trunk/lib/Support/Windows/DynamicLibrary.inc
  llvm/trunk/lib/Support/Windows/Process.inc
  llvm/trunk/lib/Support/Windows/Program.inc
  llvm/trunk/lib/Support/Windows/WindowsSupport.h
  llvm/trunk/tools/bugpoint/ExecutionDriver.cpp
  llvm/trunk/tools/bugpoint/OptimizerDriver.cpp
  llvm/trunk/tools/bugpoint/ToolRunner.cpp
  llvm/trunk/tools/bugpoint/ToolRunner.h
  llvm/trunk/tools/dsymutil/MachOUtils.cpp
  llvm/trunk/tools/llvm-cov/CodeCoverage.cpp
  llvm/trunk/unittests/Support/ProgramTest.cpp
  llvm/trunk/utils/not/not.cpp

Index: clang/trunk/tools/driver/driver.cpp
===
--- clang/trunk/tools/driver/driver.cpp
+++ clang/trunk/tools/driver/driver.cpp
@@ -199,11 +199,11 @@
   }
 }
 
-extern int cc1_main(ArrayRef Argv, const char *Argv0,
+extern bool cc1_main(ArrayRef Argv, const char *Argv0,
 void *MainAddr);
-extern int cc1as_main(ArrayRef Argv, const char *Argv0,
+extern bool cc1as_main(ArrayRef Argv, const char *Argv0,
   void *MainAddr);
-extern int cc1gen_reproducer_main(ArrayRef Argv,
+extern bool cc1gen_reproducer_main(ArrayRef Argv,
   const char *Argv0, void *MainAddr);
 
 static void insertTargetAndModeArgs(const ParsedClangName &NameParts,
@@ -304,7 +304,7 @@
 TheDriver.setInstalledDir(InstalledPathParent);
 }
 
-static int ExecuteCC1Tool(ArrayRef argv, StringRef Tool) {
+static bool ExecuteCC1Tool(ArrayRef argv, StringRef Tool) {
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
   if (Tool == "")
 return cc1_main(argv.slice(2), argv[0], GetExecutablePathVP);
@@ -316,15 +316,15 @@
   // Reject unknown tools.
   llvm::errs() << "error: unknown integrated tool '" << Tool << "'. "
<< "Valid tools include '-cc1' and '-cc1as'.\n";
-  return 1;
+  return false;
 }
 
 int main(int argc_, const char **argv_) {
   llvm::InitLLVM X(argc_, argv_);
   SmallVector argv(argv_, argv_ + argc_);
 
   if (llvm::sys::Process::FixupStandardFileDescriptors())
-return 1;
+return (int)llvm::sys::ProgramReturnCode::Failure;
 
   llvm::InitializeAllTargets();
   auto TargetAndMode = ToolChain::getTargetAndModeFromProgramName(argv[0]);
@@ -379,7 +379,9 @@
   auto newEnd = std::remove(argv.begin(), argv.end(), nullptr);
   argv.resize(newEnd - argv.begin());
 }
-return ExecuteCC1Tool(argv, argv[1] + 4);
+bool R = ExecuteCC1Tool(argv, argv[1] + 4);
+return int(R ? llvm::sys::ProgramReturnCode::Success
+ : llvm::sys::ProgramReturnCode::Failure);
   }
 
   bool CanonicalPrefixes = true;
@@ -456,39 +458,32 @@
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);
 
   std::unique_ptr C(TheDriver.BuildCompilat

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

The goal of this change is frictionless compilation into VS2017 when using 
`clang-cl` as a compiler. We've realized that compiling Clang+LLD with Clang 
generates a faster executable that with MSVC (even latest one).
I currently can't see a good way of generating the Visual Studio solution with 
CMake, while using Ninja+clang-cl for compilation. They are two orthogonal 
configurations. Any suggestions?


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In https://reviews.llvm.org/D52193#1238944, @zturner wrote:

> In https://reviews.llvm.org/D52193#1238923, @aganea wrote:
>
> > The goal of this change is frictionless compilation into VS2017 when using 
> > `clang-cl` as a compiler. We've realized that compiling Clang+LLD with 
> > Clang generates a faster executable that with MSVC (even latest one).
> >  I currently can't see a good way of generating the Visual Studio solution 
> > with CMake, while using Ninja+clang-cl for compilation. They are two 
> > orthogonal configurations. Any suggestions?
>
>
> I don't think this is necessary.  I think your updated Matrix is pretty good.
>
> I'm surprised to see that Ninja + Clang is slower than Ninja + MSVC.  Are you 
> using lld here?


Yes, all the ‘Clang’ rows use both clang-cl and lld-link.
Like stated above, I think this is caused by a lot more processes 
(clang-cl.exe) being invoked. In contrast, cl.exe does not invoke a child 
process. There are about 3200 files to compiles, which makes 6400 calls to 
clang-cl. At about ~60ms lead time per process, that adds up to an extra ~3min 
wall clock time. It is the simplest explanation I could find.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In https://reviews.llvm.org/D52193#1239171, @hans wrote:

> Thanks for adding the Ninja numbers. It confirms that Ninja is significantly 
> faster than MSBuild + /MP.


It is true for the 6-cores Xeon, but not for the dual-18-cores. I think there 
are two issues there, somehow unrelated to /MP: 1. Invoking the child clang-cl 
-cc1 for **each** cpp file makes things much slower. I’ve clocked the 
invocation at about 60-100ms (which is caused mainly by loading dlls & 
registery accesses). Most likely Reid’s change about delay loading dlls should 
improve that. 2. The other issue is, I think, the process context switching at 
the OS-level. This link:  
https://www.phoronix.com/scan.php?page=article&item=2990wx-linux-windows&num=4 
- shows that multi-threading is significantly faster on a Linux machine, as far 
as high cores count goes (when compared with the same test ran on the same 
machine under Windows 10).

> Since that's the case, maybe we should be poking MS about making MSBuild 
> faster instead of adding /MP support to Clang? Or making it easier to use 
> Ninja in VS projects? Your patch says RFC after all :-)

Sad for Microsoft, but at this point it is a _design_ change MSBuild needs. And 
as a PM I would rather invest in bindings to better build systems (Ninja, 
Fastbuild). However I expect there are still users of MSBuild out there, and 
without /MP this means essentially that migrating to clang-cl requires also 
changing their build system and their VS2017 integration with the said build 
system.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

> clang-cl isn't supposed to do (explicit) registry accesses when you hold it 
> right (pass in -fms-compatibility-version etc). Have you seen registry access 
> costs, or is that speculation?

No speculation, I will share the FileMon logs shortly. It is indirectly 
dependent DLLs that access the registry, not clang-cl itself.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added a comment.

It seems Reid's change has done good: `cmake` is not as significant as before 
in the build process. See below the improved timings:

The tests consist in a full cleanup (delete build folder), cmake to regenerate, 
then a full rebuild of LLVM + Clang + LLD (at r342552), Release target, 
optimized tablegen.
VS2017 15.8.3, Ninja 1.8.2, CMake 3.12.2
For the `clang-cl` tests, I'm not using any official LLVM release, only the 
binaries I built myself. `lld-link` is used in that case.

//(built with MSVC)// means the LLVM toolchain used to perfom this test was 
compiled in a previous run with MSVC cl 15.8.3
//(built with Clang)// means the LLVM toolchain used to perform this test was 
compiled in a previous run with Clang at r342552

I took the best figures from several runs (ran in a random order).

---

**Config 1 :** Intel Xeon Haswell 6 cores / 12 HW threads, 3.5 GHz, 15M cache, 
128 GB RAM, SSD 550 MB/s

__MSBuild :__

| MSVC cl /MP | (50min 26sec) | 2 parallel msbuild  
|
| MSVC cl /MP | (40min 23sec) | 8 parallel msbuild  
|
| MSVC cl /MP | (40min 5sec)  | 16 parallel msbuild 
|
| clang-cl //(built with MSVC)//  | (43min 36sec) | 16 parallel msbuild 
|
| clang-cl //(built with Clang//) | (43min 42sec) | 16 parallel msbuild 
|
| clang-cl **/MP** //(built with MSVC)//  | not tested| 
|
| clang-cl **/MP** //(built with Clang)// | (36min 13sec) | 8 parallel msbuild  
|
| clang-cl **/MP** //(built with Clang)// | (34min 57sec) | 16 parallel msbuild 
|
|

__Ninja:__

| MSVC cl | (33min 29sec) |
| clang-cl //(built with MSVC)//  | (30min 2sec)  |
| clang-cl //(built with Clang)// | (28min 29sec) |
|



-

**Config 2 :** Intel Xeon Skylake 18 cores / 36 HW threads, x2 (Dual CPU), 72 
HW threads total, 2.3 GHz, 24.75M cache, 128 GB RAM, NVMe 4.6 GB/s

__MSBuild :__

| MSVC cl /MP | (10min 3sec)  | 32 parallel msbuild 
|
| clang-cl //(built with MSVC)//  | (24min 15sec) | 32 parallel msbuild 
|
| clang-cl //(built with Clang)// | (21min 21sec) | 32 parallel msbuild 
|
| clang-cl **/MP** //(built with MSVC)//  | (7min 52sec)  | 32 parallel msbuild 
|
| clang-cl **/MP** //(built with Clang)// | (7min 30sec)  | 32 parallel msbuild 
|
|

__Ninja:__

| MSVC cl | (7min 25sec) |
| clang-cl //(built with MSVC)//  | (8min 23sec) |
| clang-cl //(built with Clang)// | (8min)   |
|




Comment at: llvm/trunk/lib/Support/Windows/Program.inc:424
 
-ProcessInfo sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
-  bool WaitUntilChildTerminates, std::string *ErrMsg) {
-  assert(PI.Pid && "invalid pid to wait on, process not started?");
-  assert((PI.Process && PI.Process != INVALID_HANDLE_VALUE) &&
- "invalid process handle to wait on, process not started?");
+bool sys::WaitMany(MutableArrayRef PIArray, bool WaitOnAll,
+   unsigned SecondsToWait, bool WaitUntilProcessTerminates) {

rnk wrote:
> I guess no Posix implementation? It's kind of hard to know if we made the 
> right abstractions without doing it for Windows and *nix.
Yes, I currenly don't have an Unix box at hand. But I will implement it shortly 
as it needs to be atomically commited with the Windows part.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In https://reviews.llvm.org/D52193#1241056, @zturner wrote:

> The process stuff looks cool and useful independently of `/MP`.  Would it be 
> possible to break that into a separate patch, and add a unit test for the 
> behavior of the `WaitAll` flag?


Of course, will do.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

> clang-cl isn't supposed to do (explicit) registry accesses when you hold it 
> right (pass in -fms-compatibility-version etc). Have you seen registry access 
> costs, or is that speculation?

Please see this log: F7268226: clang-cl-log.zip 
 - the child `clang-cl -cc1` takes about 
~117ms until it gets into the global initializers. This is on my Haswell PC. On 
the Skylake, this takes "only" ~60ms.
This probably explains why Ninja is slower on the Skylake when using `clang-cl` 
as a compiler. There should be a shorter codepath maybe when only a single .cpp 
is being compiled, and avoid running the child process.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In https://reviews.llvm.org/D52193#1243456, @thakis wrote:

> ...and to reword this a bit: Clang taking a long time to start up in some 
> configurations is a bug we should profile and fix :-)


This is time spent in `ntdll.dll` loading various low-level libraries like 
`kernel32.dll`, `KernelBase.dll`, `combase.dll` and so on. So at least 20ms 
just getting to main(). Then, about ~10ms spent loading `dbghelp.dll` while 
installing the exception handler in `llvm::RegisterHandler`. So a lot of wasted 
time. The easiest would be to just not invoke a new child process!



New timings without the child `clang-cl.exe` being invoked (hacked from 
https://reviews.llvm.org/D52411). The improvement **is significant**. Tested at 
r343846.

**Config 1:** Intel Xeon Haswell 6 cores / 12 HW threads, 3.5 GHz, 15M cache, 
128 GB RAM, SSD 550 MB/s

__Ninja:__

| MSVC cl  | (37min 19sec)  
   |
| clang-cl //(built with Clang)//  | //(in progress-will update a bit 
later)// |
| clang-cl no child //(built with Clang)// | //(in progress-will update a bit 
later)// |
|

**Config 2:** Intel Xeon Skylake 18 cores / 36 HW threads, x2 (Dual CPU), 72 HW 
threads total, 2.3 GHz, 24.75M cache, 128 GB RAM, NVMe SSD 4.6 GB/s

__Ninja:__

| MSVC cl  | (7min 33sec)|
| clang-cl //(built with Clang)//  | (9min 2sec) |
| **clang-cl no child //(built with Clang)//** | **(7min 9sec)** |
|

This asks whether the improvement will be of the same order, if invoking just 
one `clang-cl.exe` for the whole compilation process. A sort of local 
compilation-as-a-service.
In that scenario, Ninja could invoke `clang-cl.exe` and pass it all the files 
to be compiled, and let clang iterate and multi-thread internally. That could 
be quite a significant change, however the improvements //could be// important.
However that would probably break the concept behind //Goma //I suppose. Unless 
you split the invocations to precisely one `clang-cl` per available (remote) 
agent.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Alexandre Ganea via Phabricator via cfe-commits]

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

Thanks Anton!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914



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

[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/regular_cdb.json:4
+  "directory": "DIR",
+  "command": "clang -c DIR/regular_cdb.cpp -IInputs -MD -MF DIR/regular_cdb.d",
+  "file": "DIR/regular_cdb.cpp"

Is `-MD -MF` required for clang-scan-deps to work? Can't we append those 
arguments automatically, so that only include paths are needed in the CDB?

Would it possible for the tool to produce a collated file? A real-world usage 
would otherwise produce tens of thousands of files. Which then would be 
opened/parsed by the calling application.



Comment at: clang/test/ClangScanDeps/regular_cdb.cpp:8
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/regular_cdb.json > %t.cdb
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1
+// RUN: cat %t.dir/regular_cdb.d | FileCheck %s

Add a second test to excercice multi-threading? (by providing a few more files 
in the CDB)



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:140
+int main(int argc, const char **argv) {
+  llvm::cl::ResetAllOptionOccurrences();
+  llvm::cl::HideUnrelatedOptions(DependencyScannerCategory);

`InitLLVM X(argc, argv);` is missing here to print callstacks in case of a 
crash, like the other tools.


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

https://reviews.llvm.org/D60233



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

[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

[Alexandre Ganea via Phabricator via cfe-commits]

aganea accepted this revision.
aganea added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/ClangScanDeps/Inputs/regular_cdb.json:4
+  "directory": "DIR",
+  "command": "clang -c DIR/regular_cdb.cpp -IInputs -MD -MF DIR/regular_cdb.d",
+  "file": "DIR/regular_cdb.cpp"

arphaman wrote:
> aganea wrote:
> > Is `-MD -MF` required for clang-scan-deps to work? Can't we append those 
> > arguments automatically, so that only include paths are needed in the CDB?
> > 
> > Would it possible for the tool to produce a collated file? A real-world 
> > usage would otherwise produce tens of thousands of files. Which then would 
> > be opened/parsed by the calling application.
> I agree, the tool shouldn't rely on those options being there. `-MD -MF` in 
> this test right now a crutch to get the dependencies printed somewhere.
> 
> I think the tool by default should print out the collated dependencies to 
> STDOUT instead of writing them to files that were specified for the build. 
> The `-MD -MF` options will not be required as well. This implementation 
> requires some changes in the dependency collector, which I am planning to do 
> as part of clang-scan-deps in the follow-up patches. Will it be ok to keep 
> this patch as it is right now, and transition to printing out the collated 
> dependencies without requiring the options as a follow-up?
> 
> 
Sounds good! Please cc me on the upcoming reviews, we have a good use-case for 
clang-scan-deps in [[ http://www.fastbuild.org/docs/home.html | Fastbuild ]]!



Comment at: clang/tools/clang-scan-deps/CMakeLists.txt:3
+Core
+Support
+)

Two space indent.


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

https://reviews.llvm.org/D60233



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

[PATCH] D63290: Unify DependencyFileGenerator class and DependencyCollector interface

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a reviewer: benlangmuir.
aganea added inline comments.



Comment at: clang/include/clang/Frontend/Utils.h:118
 
 /// Builds a depdenency file when attached to a Preprocessor (for includes) and
 /// ASTReader (for module imports), and writes it out at the end of processing

s/depdenency/dependency/



Comment at: clang/lib/Frontend/DependencyFile.cpp:78
   SrcMgr::CharacteristicKind FileType) override {
 if (!File)
   DepCollector.maybeAddDependency(FileName, /*FromModule*/false,

Given that `llvm::sys::path::remove_leading_dotslash` is not called here, but 
it is for the function above, could you end up with two entries in 
`DependencyCollector::Seen` when `DependencyCollector::sawDependency` is 
overriden?



Comment at: clang/lib/Frontend/DependencyFile.cpp:165
bool IsMissing) {
-  return !isSpecialFilename(Filename) &&
+  return !IsMissing && !isSpecialFilename(Filename) &&
  (needSystemDependencies() || !IsSystem);

Why `!IsMissing` wasn't considered before? Just wondering.


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

https://reviews.llvm.org/D63290



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

[PATCH] D63290: Unify DependencyFileGenerator class and DependencyCollector interface

[Alexandre Ganea via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D63290



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

[PATCH] D63579: [clang-scan-deps] print the dependencies to stdout and remove the need to use -MD options in the CDB

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:32
 
+class DependencyCollectorFactory {
+public:

Do you envision future uses for this factory?



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:50
+std::unique_ptr Opts) override {
+  std::unique_lock LockGuard(SharedLock);
+  return std::make_shared(std::move(Opts), SharedLock,

Do you need the blocking behavior here? You're simply passing the Lock by 
reference, but not using the stream that it protects.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:73
+  std::unique_ptr Opts;
+  std::mutex &Lock;
+  raw_ostream &OS;

I think it would be better to keep together the lock with what it protects 
(thus my question above about the factory).
What about:
```
class SharedStream {
public:
  SharedStream(raw_ostream &OS) : OS(OS) {}
  template  void applyLocked(Fn f) {
std::unique_lock LockGuard(Lock);
f(OS);
OS.flush();
  }

private:
  std::mutex Lock;
  raw_ostream &OS;
};

/// Prints out all of the gathered dependencies into one output stream instead
/// of using the output dependency file.
class DependencyPrinter : public DependencyFileGenerator {
public:
  DependencyPrinter(std::unique_ptr Opts,
SharedStream &S)
  : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), S(S) {}

  void finishedMainFile(DiagnosticsEngine &Diags) override {
S.applyLocked([](raw_ostream &OS) { outputDependencyFile(OS); });
  }

private:
  std::unique_ptr Opts;
  SharedStream &S;
};
```
WDYT?



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:115
+  Opts->Targets = {"clang-scan-deps dependency"};
+Compiler.addDependencyCollector(
+DepCollectorFactory.createDependencyCollector(std::move(Opts)));

..and in that case here we would say: 
`Compiler.addDependencyCollector(std::make_shared(DepOpts, 
SharedOS))`



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:245
+  // Print out the dependency results to STDOUT by default.
+  DependencyPrinter::Factory DepPrintFactory(llvm::outs());
   unsigned NumWorkers =

...and then here: `SharedStream S(llvm::outs);`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63579



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

[PATCH] D63579: [clang-scan-deps] print the dependencies to stdout and remove the need to use -MD options in the CDB

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:32
 
+class DependencyCollectorFactory {
+public:

arphaman wrote:
> aganea wrote:
> > Do you envision future uses for this factory?
> Most likely, yes.
> 
> I don't want to lock-in into creating the dependency printer in the 
> `PreprocessorOnlyTool`, as the eventual goal is to have it in a library, and 
> allow clients to consume modular dependencies not only using 
> `clang-scan-deps` tool itself, but using the C++ library directly 
> (potentially through a libclang C interface as well). The printer should be 
> in `clang-scan-deps` tool itself though, so it wouldn't be ideal to reference 
> it from `PreprocessorOnlyTool` now.
My initial question was, what are the future planned factories? (and thus the 
other `DependencyCollector` implementations?)

Do you think we could have the printer in the library as well?

On a related note - we're already testing `clang-scan-deps` compiled as a DLL. 
Our use-case right now is to call it from each of [[ 
https://github.com/fastbuild/fastbuild | Fastbuild ]]'s worker threads. We are 
using `main()` below almost as-is (except for the multithreading part which 
Fastbuild already does). The CDB JSON is passed by value from the Fastbuild 
thread, so it doesn't go through a file. I'd like to get back 
dependencies-as-a-string instead of `llvm::outs()`. The bottleneck currently 
is, as you can imagine, reading / writing files (seen in ETW traces), this 
patch should improve things a bit. I can send a sample of how we're using it 
after you commit.


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

https://reviews.llvm.org/D63579



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

[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Ping!


Repository:
  rC Clang

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

https://reviews.llvm.org/D63648



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D52193#1589096 , @yurybura wrote:

> Do you have any plans to make this PR compatible with trunk? Now MSVC with 
> /MP builds much faster than clang-cl (at least 2x faster for our project)...


I'll get back to this after the vacation (somewhere in August)

In D52193#1589150 , @angushewlett 
wrote:

> They're issued in the right order, but the second doesn't wait for the first 
> to complete.


Ok, I thought the wait was handled by MSBuild. If I understand correctly what 
you're saying, MSBuild issues both commands at the same time, but the second 
`cl.exe /Yu /MP` somehow waits for the first `cl.exe /Yc` to complete? (maybe 
by checking if the PCH is still open for write?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D52193



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

[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for getting back Hans!

In D63648#1589119 , @hans wrote:

> Are you saying the diagnostics were not using absolute paths in those cases 
> and this patch fixes that?


Yes.

>> , or --show-includes, or -E
> 
> Those aren't diagnostics, so that's not surprising.

What would suggest in that case? Add a new `-fpreprocessor-absolute-paths` 
option? Or change the name of `-fdiagnostics-absolute-paths` for another name 
that applies to both diagnostics and the preprocessor output?

>> or displaying notes.
> 
> What notes?

That is, the additional output starting with `note:` issued along error 
messages: `t.cc:4:5: note: candidate function not viable: no known conversion 
from 'vector>' to 'vector>' for 1st 
argument;`
Sometimes, notes are displaying an extra path, sometimes it's just the note 
alone.

>> We have a peculiar use-case on our end with Fastbuild, where all this was 
>> exposed: CPP files are being are preprocessed on one PC, then compiled on 
>> another PC (which doesn't have the source-code); then the compiler's stdout 
>> is displayed on the first PC.
> 
> And what is the final problem? That diagnostics from the compiler's stdout 
> are not absolute because they came from the preprocessed code that doesn't 
> include the absolute paths?

Exactly. When clicking on a warning/error, those relative paths prevent Visual 
Studio from jumping to the right location.

Please let me know if further clarification is needed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63648



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

[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

The problem is that it is not `cl.exe`'s behavior - it always makes paths 
absolute:

  F:\svn\test>cl /c test.cc /showIncludes
  Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  
  test.cc
  Note: including file: f:\svn\test\foo/test.h
  
  F:\svn\test>c:
  
  C:\Windows\System32>cl /c f:test.cc /showIncludes
  Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  
  test.cc
  Note: including file: f:\svn\test\foo/test.h

So maybe `Driver::IsCLMode()` should take precedence over 
`-fdiagnostics-absolute-paths` when using `/showIncludes`?

The behavior between `clang-cl` and `cl` is also very different when using `-E`:

  C:\Windows\System32>f:\svn\build\Debug\bin\clang-cl /c f:test.cc /E
  clang-cl: error: no such file or directory: 'f:test.cc'
  clang-cl: error: no input files
  
  C:\Windows\System32>f:
  
  F:\svn\test>f:\svn\build\Debug\bin\clang-cl /c test.cc /E
  # 1 "test.cc"
  # 1 "" 1
  # 1 "" 3
  # 361 "" 3
  # 1 "" 1
  # 1 "" 2
  # 1 "test.cc" 2
  # 1 "./foo/test.h" 1
  void f();
  # 1 "test.cc" 2
  
  
  F:\svn\test>cl /c test.cc /E
  Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  
  test.cc
  #line 1 "test.cc"
  #line 1 "f:\\svn\\test\\foo/test.h"
  void f();
  #line 2 "test.cc"
  
  F:\svn\test>cl /c test.cc /E /FC
  Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  
  test.cc
  #line 1 "f:\\svn\\test\\test.cc"
  #line 1 "f:\\svn\\test\\foo\\test.h"
  void f();
  #line 2 "f:\\svn\\test\\test.cc"

So maybe I should also implement `/FC` 

 along the way, and make the defaults match between `clang-cl` and `cl`? WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63648



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

[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

It totally makes sense, thanks for the explanation Nico! Let's forget about 
`cl` compatibility, that wasn't my initial intent.

We always pass //relative// paths on the cmd-line, for all the reasons you've 
mentioned. We also pass `-fdiagnostics-absolute-paths` hoping to fix the Visual 
Studio jump-to-location issue I've mentioned above. However that doesn't work, 
because we do:

  (locally, on my PC)
$ clang-cl file.cpp -Isome/relative/path/ -fdiagnostics-absolute-paths 
/showIncludes -E >file_pre.cpp
  
  (and then, on a remote PC)
$ clang-cl file_pre.cpp -fdiagnostics-absolute-paths
  (the remote stdout is then displayed on my PC)

`-fdiagnostics-absolute-paths` has no effect in the latter case, because the 
paths are extracted from the preprocessed file, and paths can't be absolutized 
anymore on the remote PC because it doesn't have the source code (it is only a 
worker server).
So we end with relative paths in diagnostics, and Visual Studio can't jump to 
the location of the diagnostic.

What would you suggest? Use absolute paths along with `-fdebug-prefix-map`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63648



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

I will take a look next week when I get back!


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

https://reviews.llvm.org/D63907



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added subscribers: sammccall, JDevlieghere.
aganea added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:1
+//===- DependencyScanningFilesystem.h - clang-scan-deps fs ===---*- C++ 
-*-===//
+//

General comment for this file and the implementation: it seems there's nothing 
specific to the dependency scanning, except for replacing the content with 
minimized content? This cached FS could be very well used to compile a project 
with parallel workers. Could this be part of the `VirtualFileSystem` 
infrastructure? ( `llvm/include/llvm/Support/VirtualFileSystem.h`) If yes, can 
you possibly create a separate patch for this? @JDevlieghere @sammccall 



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:101
+public:
+  /// A \c CachedFileSystemEntry with a lock.
+  struct SharedFileSystemEntry {

The struct is self-explanatory, not sure the comment is needed here?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:103
+  struct SharedFileSystemEntry {
+std::mutex Lock;
+CachedFileSystemEntry Value;

Would you mind renaming this to `ValueLock` so it is easier to search for? (and 
to remain consistent with `CacheLock` below)



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:129
+/// computation.
+class DependencyScanningFilesystem : public llvm::vfs::ProxyFileSystem {
+public:

Maybe worth mentioning this is NOT a global, thread-safe, class like 
`DependencyScanningFilesystemSharedCache`, but rather meant to be used as 
per-thread instances?

I am also wondering if there could be a better name to signify at first sight 
that this is a per-thread class. `DependencyScanningLocalFilesystem`? 
`DependencyScanningWorkerFilesystem`?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:84
+DependencyScanningFilesystemSharedCache() {
+  NumShards = 8;
+  CacheShards = llvm::make_unique(NumShards);

This line needs a comment. Is this value based on empirical results across 
different hardwares? (I can't recall your answer at the LLVM conf) I am 
wondering what would be the good strategy here? The more cores/HT in your PC, 
the more chances that you'll hit the same shard, thus locking. But the bigger 
we make this value, the more `StringMaps` we will have, and more cache misses 
possibly.
Should it be something like `llvm::hardware_concurrency() / some_fudge`? It'd 
be interesting to subsequently profile on high core count machines (or maybe 
you have done that).



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:210
+  bool KeepOriginalSource = IgnoredFiles.count(Filename);
+  auto &SharedCacheEntry = SharedCache.get(Filename);
+  const CachedFileSystemEntry *Result;

Don't use `auto` when the type is not obvious.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:148
+  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+  if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
+DepFS = new DependencyScanningFilesystem(Service.getSharedCache(), RealFS);

Can we not use caching all the time?


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

https://reviews.llvm.org/D63907



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

[PATCH] D65511: Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11545
+for (CXXMethodDecl *M : WorkList) {
+  DefineImplicitSpecialMember(*this, M, M->getLocation());
+  ActOnFinishInlineFunctionDef(M);

rnk wrote:
> hans wrote:
> > rnk wrote:
> > > Do we need to do this until fixpoint? Suppose a dllexported implicit 
> > > special member triggers a template instantiation, and the template has a 
> > > dllexported defaulted special member, something like:
> > > ```
> > > struct Bar { Bar(); };
> > > template  struct Foo { __declspec(dllexport) Foo() = default; 
> > > Bar obj; };
> > > struct Baz {
> > >... not sure how to trigger instantiation
> > > };
> > > ```
> > I think that should work, and that's why the function is written to be 
> > re-entrant by having a local worklist. If it triggers a template 
> > instantiation, ActOnFinishCXXNonNestedClass should get called for the newly 
> > instantiated class. But I'm also not sure how to write a test case that 
> > would trigger it.
> Right, I see. Nevermind then.
This works on latest MSVC:
```
$ cat test.cc
struct Bar { Bar(); };
template  struct Foo { __declspec(dllexport) Foo() = default; Bar 
b; };
template class Foo;

$ cl /c /Z7 test.cc   <-- crashes if using clang-cl

$ lib test.obj /def

$ cat test2.cc
struct Bar { Bar() { } };
template  struct Foo { __declspec(dllimport) Foo() = default; Bar 
b; };
int main() {
  Foo f;
  return 0;
}

$ cl /c /Z7 test2.cc

$ link test.lib test2.cc /DEBUG
```


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

https://reviews.llvm.org/D65511



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:84
+DependencyScanningFilesystemSharedCache() {
+  NumShards = 8;
+  CacheShards = llvm::make_unique(NumShards);

arphaman wrote:
> aganea wrote:
> > This line needs a comment. Is this value based on empirical results across 
> > different hardwares? (I can't recall your answer at the LLVM conf) I am 
> > wondering what would be the good strategy here? The more cores/HT in your 
> > PC, the more chances that you'll hit the same shard, thus locking. But the 
> > bigger we make this value, the more `StringMaps` we will have, and more 
> > cache misses possibly.
> > Should it be something like `llvm::hardware_concurrency() / some_fudge`? 
> > It'd be interesting to subsequently profile on high core count machines (or 
> > maybe you have done that).
> I rewrote it to use a heuristic we settled on after doing empirical testing 
> on an 18 core / 36 thread machine ( max(2, thread_count / 4) ), and added a 
> comment. This was the number `9` (36 / 4) I mentioned at the talk, so we only 
> got to it because of a heuristic. I think now after some SDK/OS updates the 
> effect from adding more shards is less pronounced, so it mostly flatlines 
> past some number between 5-10. A better heuristic would probably be OS 
> specific to take the cost of lock contention into account.
> 
> Note that the increased number of shards does not increase the number of 
> cache misses, because the shard # is determined by the filename (we don't 
> actually have global cache misses, as the cache is designed to have only one 
> miss per file when it's first accessed)! It's just that an increased number 
> of shards doesn't improve performance after hitting some specific limit, so 
> we want to find a point where we can taper it off.
> 
> It would still be definitely interesting to profile it on other high core 
> machines with different OSes to see if it's a reasonable heuristic for those 
> scenarios too.
I'll give it a try on Windows 10, one project here has a large codebase and 
some wild machines to test on.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:156
+/// this subclass.
+class MinimizedVFSFile final : public llvm::vfs::File {
+public:

This makes only a short-lived objects, is that right? Just during the call to 
`CachedFileSystemEntry::createFileEntry`?



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:94
+  // Add any filenames that were explicity passed in the build settings and
+  // that might be might be opened, as we want to ensure we don't run 
source
+  // minimization on them.

"might be" twice.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:148
+  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+  if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
+DepFS = new DependencyScanningFilesystem(Service.getSharedCache(), RealFS);

arphaman wrote:
> aganea wrote:
> > Can we not use caching all the time?
> We want to have a mode where it's as close to the regular `clang -E` 
> invocation as possible for correctness CI and testing. We also haven't 
> evaluated if the cost of keeping non-minimized sources in memory, so it might 
> be too expensive for practical use? I can add a third option that caches but 
> doesn't minimize though as a follow-up patch though 
> 
Yes that would be nice. As for the size taken in RAM, it would be only .H 
files, right? For Clang+LLVM+LLD I'm counting about 40 MB. But indeed with a 
large project, that would be about 1.5 GB of .H. Although I doubt all these 
files will be loaded at once in memory (I'll check)

As for the usage: Fastbuild works like distcc (plain mode, not pump) so we were 
also planning on extracting the fully preprocessed output, not only the 
dependencies. There's one use-case where we need to preprocess locally, then 
send the preprocessed output remotely for compilation. And another use-case 
where we only want to extract the dependency list, compute a digest, to 
retrieve the OBJ from a network cache.


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

https://reviews.llvm.org/D63907



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the update Alex! Just a few more comments and we should be good to 
go:




Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:117
+std::mutex CacheLock;
+llvm::StringMap> Cache;
+  };

Why not use a bump allocator here? (it would be per-thread) On Windows the heap 
is always thread-safe, which induce a lock in `malloc` for each new entry. You 
could also avoid the usage of `unique_ptr` by the same occasion:

`llvm::StringMap> Cache;`

//(unless you're planning on removing entries from the cache later on?)//



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76
+CachedFileSystemEntry
+CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) {
+  assert(Stat.isDirectory() && "not a directory!");

`llvm::vfs::Status &&Stat` to avoid a copy?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:103
+  auto It =
+  Shard.Cache.try_emplace(Key, std::unique_ptr());
+  auto &Ptr = It.first->getValue();

`Shard.Cache.try_emplace(Key)` will provide a default constructed value if it's 
not there.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:117
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+return Entry->getStatus();

`CachedFileSystemEntry *Entry` ?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:198
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+return createFile(Entry);

CachedFileSystemEntry *Entry ?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:148
+  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+  if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
+DepFS = new DependencyScanningFilesystem(Service.getSharedCache(), RealFS);

arphaman wrote:
> aganea wrote:
> > arphaman wrote:
> > > aganea wrote:
> > > > Can we not use caching all the time?
> > > We want to have a mode where it's as close to the regular `clang -E` 
> > > invocation as possible for correctness CI and testing. We also haven't 
> > > evaluated if the cost of keeping non-minimized sources in memory, so it 
> > > might be too expensive for practical use? I can add a third option that 
> > > caches but doesn't minimize though as a follow-up patch though 
> > > 
> > Yes that would be nice. As for the size taken in RAM, it would be only .H 
> > files, right? For Clang+LLVM+LLD I'm counting about 40 MB. But indeed with 
> > a large project, that would be about 1.5 GB of .H. Although I doubt all 
> > these files will be loaded at once in memory (I'll check)
> > 
> > As for the usage: Fastbuild works like distcc (plain mode, not pump) so we 
> > were also planning on extracting the fully preprocessed output, not only 
> > the dependencies. There's one use-case where we need to preprocess locally, 
> > then send the preprocessed output remotely for compilation. And another 
> > use-case where we only want to extract the dependency list, compute a 
> > digest, to retrieve the OBJ from a network cache.
> Right now it doesn't differentiate between .H and other files, but we could 
> teach it do have a header only filter for the cache.
No worries, I was simply wondering about the size in memory.


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

https://reviews.llvm.org/D63907



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

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

LGTM, thank you!

In D63907#1617417 , @arphaman wrote:

> Just for reference, this patch still doesn't reuse the FileManager across 
> invocations in a thread. We expect to get even better performance once we 
> reuse it, but I'm going have to improve its API first.


Can't wait! @SamChaps is already testing this patch. He found some minor 
edge-cases with the minimizer (most likely unrelated to this), I'll post a 
patch.




Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76
+CachedFileSystemEntry
+CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) {
+  assert(Stat.isDirectory() && "not a directory!");

arphaman wrote:
> aganea wrote:
> > `llvm::vfs::Status &&Stat` to avoid a copy?
> The copy should already be avoided, as I move the argument when passing in, 
> and when it's assigned to the result.
If `Stat` is not a rvalue reference, wouldn't the `std::move` in the call site 
end up as a copy? See [[ https://godbolt.org/z/Rr7cdM | this ]]. If you change 
`int test(A a)` to `int test(A &&a)` you can see the difference in the asm 
output. However if the function is inlined, the extra copy will probably be 
optimized out. Not a big deal - the OS calls like stat() will most likely 
dominate here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63907



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

[PATCH] D65906: [clang-scan-deps] Fix edge cases in the minimizer

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: arphaman, dexonsmith, Bigcheese.
aganea added a project: clang.
Herald added a subscriber: tschuett.

This patch fixes:

1. Invisible characters that come just before #include, such as #ifndef. 
( were hidden, depending on the display locale). I choose to simply skip 
over non-ASCII characters.
2. Double slashes in #include directive with angle brackets not handled 
correctly: #include 
3. #error directive with quoted, multi-line content, along with CR+LF line 
endings wasn't handled correctly.


Repository:
  rC Clang

https://reviews.llvm.org/D65906

Files:
  lib/Lex/DependencyDirectivesSourceMinimizer.cpp
  test/Lexer/minimize_source_to_dependency_directives_include.c
  test/Lexer/minimize_source_to_dependency_directives_invalid_chars.c
  test/Lexer/minimize_source_to_dependency_directives_invalid_error.c

Index: test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
===
--- test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
+++ test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
@@ -0,0 +1,16 @@
+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: cat %s | unix2dos | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include 
+#endif
+
+// CHECK:  #ifdef OTHER
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif
Index: test/Lexer/minimize_source_to_dependency_directives_invalid_chars.c
===
--- test/Lexer/minimize_source_to_dependency_directives_invalid_chars.c
+++ test/Lexer/minimize_source_to_dependency_directives_invalid_chars.c
@@ -0,0 +1,9 @@
+// Test invisible, bad characters just before #ifdef
+// RUN: echo -n -e '\xef\xbb\xbf#ifdef TEST\n' > %t.c
+// RUN: echo '#include ' >> %t.c
+// RUN: echo '#endif' >> %t.c
+// RUN: %clang_cc1 -DTEST -print-dependency-directives-minimized-source %t.c 2>&1 | FileCheck %s
+
+// CHECK:  #ifdef TEST
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif
Index: test/Lexer/minimize_source_to_dependency_directives_include.c
===
--- test/Lexer/minimize_source_to_dependency_directives_include.c
+++ test/Lexer/minimize_source_to_dependency_directives_include.c
@@ -0,0 +1,8 @@
+// Test double slashes in #include directive along with angle brackets. Previously, this was interpreted as comments.
+// RUN: %clang_cc1 -DTEST -print-dependency-directives-minimized-source %s 2>&1 | FileCheck %s
+
+#include "a//b.h"
+#include 
+
+// CHECK: #include "a//b.h"
+// CHECK: #include 
Index: lib/Lex/DependencyDirectivesSourceMinimizer.cpp
===
--- lib/Lex/DependencyDirectivesSourceMinimizer.cpp
+++ lib/Lex/DependencyDirectivesSourceMinimizer.cpp
@@ -113,7 +113,8 @@
 }
 
 static void skipOverSpaces(const char *&First, const char *const End) {
-  while (First != End && isHorizontalWhitespace(*First))
+  while (First != End &&
+ (isHorizontalWhitespace(*First) || !clang::isASCII(*First)))
 ++First;
 }
 
@@ -185,8 +186,8 @@
 }
 
 static void skipString(const char *&First, const char *const End) {
-  assert(*First == '\'' || *First == '"');
-  const char Terminator = *First;
+  assert(*First == '\'' || *First == '"' || *First == '<');
+  const char Terminator = *First == '<' ? '>' : *First;
   for (++First; First != End && *First != Terminator; ++First)
 if (*First == '\\')
   if (++First == End)
@@ -195,15 +196,27 @@
 ++First; // Finish off the string.
 }
 
-static void skipNewline(const char *&First, const char *End) {
-  assert(isVerticalWhitespace(*First));
-  ++First;
+// Returns the length of EOL, either 0 (no end-of-line), 1 (\n) or 2 (\r\n)
+static unsigned isEOL(const char *First, const char *const End) {
   if (First == End)
-return;
+return 0;
+  if (End - First > 1) {
+if (isVerticalWhitespace(First[0]) && isVerticalWhitespace(First[1]) &&
+First[0] != First[1])
+  return 2;
+  }
+  return !!isVerticalWhitespace(First[0]);
+}
 
-  // Check for "\n\r" and "\r\n".
-  if (LLVM_UNLIKELY(isVerticalWhitespace(*First) && First[-1] != First[0]))
-++First;
+static unsigned skipNewline(const char *&First, const char *End) {
+  unsigned Len = isEOL(First, End);
+  assert(Len);
+  First += Len;
+  return Len;
+}
+
+static bool wasLineContinuation(const char *First, unsigned Len) {
+  return First[-(int)Len - 1] == '\\';
 }
 
 static void skipToNewlineRaw(const char *&First, const char *const End) {
@@ -211,17 +224,22 @@
 if (First == End)
   return;
 
-if (isVerticalWhitespace(*First))
+unsigned Len = isEOL(First, End);
+if (Len)
   return;
 
-while (!isVerticalWhitespace(*First))
+do {
   if (

[PATCH] D65906: [clang-scan-deps] Fix edge cases in the minimizer

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D65906#1620034 , @arphaman wrote:

> Regarding the invisible characters before `#ifdef`, are you sure that's the 
> right behavior?


Should we then copy these invisible characters to the minimized output? Believe 
it or not, these characters are there in our codebase since 2013, and never 
clang has complained about it :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D65906



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

[PATCH] D65906: [clang-scan-deps] Fix edge cases in the minimizer

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D65906#1622209 , @arphaman wrote:

> @aganea These are not just any invisible characters that you have, this is 
> the UTF8 BOM. Clang's Lexer skips over them if they're in the beginning of 
> the file (`Lexer::InitLexer`). The minimizer should do the same thing, so 
> ideally you would factor out the BOM detection and teach the minimizer to 
> skip past it.


Thanks for the heads up. I'll do that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65906



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

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: rnk, aprantl.
aganea added a project: clang.

Previously, when clang was compiled with -DLLVM_ENABLE_ASSERTIONS=ON, the 
attached test was yielding:

  inlinable function call in a function with debug info must have a !dbg 
location
call void @"??1?$c@UBQEAA@XZ"(%struct.c* 
@"?f@?1??d@@YAPEAU?$c@UBXZ@4U2@A")
  fatal error: error in backend: Broken module found, compilation aborted!
  Stack dump:
  0.  Program arguments:  -gcodeview -debug-info-kind=limited
  1.   parser at end of file
  2.  Per-function optimization

Fixes PR43012


Repository:
  rC Clang

https://reviews.llvm.org/D66328

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGen/debug-info-no-location.cpp


Index: test/CodeGen/debug-info-no-location.cpp
===
--- test/CodeGen/debug-info-no-location.cpp
+++ test/CodeGen/debug-info-no-location.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang -cc1 -emit-llvm %s -gcodeview -debug-info-kind=limited -o - | 
FileCheck %s
+// CHECK: call void @"??1?$c@UBQEAA@XZ"(%struct.c* 
@"?f@?1??d@@YAPEAU?$c@UBXZ@4U2@A"), !dbg !51
+// CHECK-NEXT: ret void, !dbg !51
+// CHECK: !48 = distinct !DISubprogram(name: "`dynamic atexit destructor for 
'f'", scope: !3, file: !3, line: 16, type: !49, scopeLine: 16, spFlags: 
DISPFlagLocalToUnit | DISPFlagDefinition, unit: !27, retainedNodes: !29)
+// CHECK: !51 = !DILocation(line: 16, scope: !48)
+
+struct a {
+  ~a();
+};
+template  struct c : a {
+  c(void (b::*)());
+};
+struct B {
+  virtual void e();
+};
+c *d() { static c f(&B::e); return &f; }
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -246,7 +246,8 @@
   CodeGenFunction CGF(CGM);
 
   CGF.StartFunction(GlobalDecl(&VD, DynamicInitKind::AtExit),
-CGM.getContext().VoidTy, fn, FI, FunctionArgList());
+CGM.getContext().VoidTy, fn, FI, FunctionArgList(),
+VD.getLocation(), VD.getInit()->getExprLoc());
 
   llvm::CallInst *call = CGF.Builder.CreateCall(dtor, addr);
 


Index: test/CodeGen/debug-info-no-location.cpp
===
--- test/CodeGen/debug-info-no-location.cpp
+++ test/CodeGen/debug-info-no-location.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang -cc1 -emit-llvm %s -gcodeview -debug-info-kind=limited -o - | FileCheck %s
+// CHECK: call void @"??1?$c@UBQEAA@XZ"(%struct.c* @"?f@?1??d@@YAPEAU?$c@UBXZ@4U2@A"), !dbg !51
+// CHECK-NEXT: ret void, !dbg !51
+// CHECK: !48 = distinct !DISubprogram(name: "`dynamic atexit destructor for 'f'", scope: !3, file: !3, line: 16, type: !49, scopeLine: 16, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !27, retainedNodes: !29)
+// CHECK: !51 = !DILocation(line: 16, scope: !48)
+
+struct a {
+  ~a();
+};
+template  struct c : a {
+  c(void (b::*)());
+};
+struct B {
+  virtual void e();
+};
+c *d() { static c f(&B::e); return &f; }
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -246,7 +246,8 @@
   CodeGenFunction CGF(CGM);
 
   CGF.StartFunction(GlobalDecl(&VD, DynamicInitKind::AtExit),
-CGM.getContext().VoidTy, fn, FI, FunctionArgList());
+CGM.getContext().VoidTy, fn, FI, FunctionArgList(),
+VD.getLocation(), VD.getInit()->getExprLoc());
 
   llvm::CallInst *call = CGF.Builder.CreateCall(dtor, addr);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 216046.
aganea marked 4 inline comments as done.
aganea added a comment.

As requested.


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

https://reviews.llvm.org/D66328

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenCXX/debug-info-atexit-stub.cpp
  test/CodeGenCXX/debug-info-global-ctor-dtor.cpp


Index: test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
===
--- test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
+++ test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
@@ -30,24 +30,24 @@
 template A FooTpl::sdm_tpl;
 
 // CHECK-NOKEXT: !DISubprogram(name: "__cxx_global_var_init",{{.*}} line: 
15,{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-NOKEXT: !DISubprogram(name: "__dtor_glob",{{.*}} line: 15,{{.*}} 
DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-NOKEXT: !DISubprogram(name: "__dtor_glob",{{.*}}: DISPFlagLocalToUnit 
| DISPFlagDefinition
 // CHECK-NOKEXT: !DISubprogram(name: "__cxx_global_var_init.1",{{.*}} line: 
16,{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK-NOKEXT: !DISubprogram(name: "__cxx_global_array_dtor",{{.*}} line: 
16,{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-NOKEXT: !DISubprogram(name: "__dtor_array",{{.*}} line: 16,{{.*}} 
DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-NOKEXT: !DISubprogram(name: "__dtor__ZZ3foovE4stat",{{.*}} line: 
19,{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-NOKEXT: !DISubprogram(name: "__dtor_array",{{.*}}: 
DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-NOKEXT: !DISubprogram(name: "__dtor__ZZ3foovE4stat",{{.*}} 
DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK-NOKEXT: !DISubprogram({{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
 
 // CHECK-KEXT: !DISubprogram({{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
 
 // CHECK-MSVC: !DISubprogram(name: "`dynamic initializer for 'glob'",{{.*}} 
line: 15,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'glob'",{{.*}} line: 15,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'glob'",{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK-MSVC: !DISubprogram(name: "`dynamic initializer for 'array'",{{.*}} 
line: 16,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK-MSVC: !DISubprogram(name: "__cxx_global_array_dtor",{{.*}} line: 
16,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'array'",{{.*}} line: 16,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'stat'",{{.*}} line: 19,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'array'",{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
+// CHECK-MSVC: !DISubprogram(name: "`dynamic atexit destructor for 
'stat'",{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
 
 // MSVC does weird stuff when templates are involved, so we don't match 
exactly,
 // but these names are reasonable.
 // FIXME: These should not be marked DISPFlagLocalToUnit.
 // CHECK-MSVC: !DISubprogram(name: "FooTpl::`dynamic initializer for 
'sdm_tpl'",{{.*}} line: 29,{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
-// CHECK-MSVC: !DISubprogram(name: "FooTpl::`dynamic atexit destructor 
for 'sdm_tpl'",{{.*}} line: 29,{{.*}}: DISPFlagLocalToUnit | 
DISPFlagDefinition
+// CHECK-MSVC: !DISubprogram(name: "FooTpl::`dynamic atexit destructor 
for 'sdm_tpl'",{{.*}}: DISPFlagLocalToUnit | DISPFlagDefinition
Index: test/CodeGenCXX/debug-info-atexit-stub.cpp
===
--- test/CodeGenCXX/debug-info-atexit-stub.cpp
+++ test/CodeGenCXX/debug-info-atexit-stub.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -emit-llvm %s -gcodeview -debug-info-kind=limited -o - | 
FileCheck %s
+
+struct a {
+  ~a();
+};
+template  struct c : a {
+  c(void (b::*)());
+};
+struct B {
+  virtual void e();
+};
+c *d() { static c f(&B::e); return &f; }
+
+// CHECK: define internal void @"??__Ff@?1??d@@YAPEAU?$c@UBXZ@YAXXZ"()
+// CHECK-SAME: !dbg ![[SUBPROGRAM:[0-9]+]] {
+// CHECK: call void @"??1?$c@UBQEAA@XZ"(%struct.c* 
@"?f@?1??d@@YAPEAU?$c@UBXZ@4U2@A"), !dbg ![[LOCATION:[0-9]+]]
+// CHECK-NEXT: ret void, !dbg ![[LOCATION]]
+// CHECK: ![[SUBPROGRAM]] = distinct !DISubprogram(name: "`dynamic atexit 
destructor for 'f'"
+// CHECK-SAME: flags: DIFlagArtificial
+// CHECK: ![[LOCATION]] = !DILocation(line: 0, scope: ![[SUBPROGRAM]])
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -247,6 +247,8 @@
 
   CGF.StartFunction(GlobalDecl(&VD, DynamicInitKind::AtExit),
 CGM.getContext().VoidTy, fn, FI, Functi

[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: hans, thakis, rsmith.
aganea added a project: clang.

Previously, `-fdiagnostics-absolute-paths` did not have an effect in some 
cases, for example: when using relative include paths, or relative CPP paths, 
or `--show-includes`, or `-E` or displaying notes. We have a peculiar use-case 
on our end with Fastbuild, where all this was exposed: CPP files are being are 
preprocessed on one PC, then compiled on another PC (which doesn't have the 
source-code); then the compiler's stdout is displayed on the first PC.


Repository:
  rC Clang

https://reviews.llvm.org/D63648

Files:
  include/clang/Frontend/TextDiagnostic.h
  lib/Frontend/HeaderIncludeGen.cpp
  lib/Frontend/PrintPreprocessedOutput.cpp
  lib/Frontend/TextDiagnostic.cpp
  test/Frontend/absolute-paths.c
  test/Modules/build-fail-notes.m
  test/Preprocessor/pp-modules.c

Index: test/Preprocessor/pp-modules.c
===
--- test/Preprocessor/pp-modules.c
+++ test/Preprocessor/pp-modules.c
@@ -13,3 +13,30 @@
 #include "pp-modules.h" // CHECK: # 1 "{{.*}}pp-modules.h" 1
 // CHECK: #pragma clang module import Module /* clang -E: implicit import for #include  */{{$}}
 // CHECK: # 14 "{{.*}}pp-modules.c" 2
+
+// Also test path canonicalization 
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c pp-modules.c -F %S/../Modules/Inputs -E -o - | FileCheck -check-prefix=CHECK-RELATIVE %s
+
+// CHECK-RELATIVE: # 1 "pp-modules.c"
+// CHECK-RELATIVE-NEXT: # 1 "" 1
+// CHECK-RELATIVE-NEXT: # 1 "" 3
+// CHECK-RELATIVE-NEXT: # {{[0-9]+}} "" 3
+// CHECK-RELATIVE-NEXT: # 1 "" 1
+// CHECK-RELATIVE-NEXT: # 1 "" 2
+// CHECK-RELATIVE-NEXT: # 1 "pp-modules.c" 2
+// CHECK-RELATIVE: # 1 "./pp-modules.h" 1
+// CHECK-RELATIVE: # 14 "pp-modules.c" 2
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c pp-modules.c -F %S/../Modules/Inputs -E -o - -fdiagnostics-absolute-paths | FileCheck -check-prefix=CHECK-ABSOLUTE %s
+
+// CHECK-ABSOLUTE: # 1 "{{.+(/|)test(/|)Preprocessor(/|)}}pp-modules.c"
+// CHECK-ABSOLUTE-NEXT: # 1 "" 1
+// CHECK-ABSOLUTE-NEXT: # 1 "" 3
+// CHECK-ABSOLUTE-NEXT: # {{[0-9]+}} "" 3
+// CHECK-ABSOLUTE-NEXT: # 1 "" 1
+// CHECK-ABSOLUTE-NEXT: # 1 "" 2
+// CHECK-ABSOLUTE-NEXT: # 1 "{{.+(/|)test(/|)Preprocessor(/|)}}pp-modules.c" 2
+
+// CHECK-ABSOLUTE: # 1 "{{.+(/|)test(/|)Preprocessor(/|)}}pp-modules.h" 1
+// CHECK-ABSOLUTE: # 14 "{{.+(/|)test(/|)Preprocessor(/|)}}pp-modules.c" 2
Index: test/Modules/build-fail-notes.m
===
--- test/Modules/build-fail-notes.m
+++ test/Modules/build-fail-notes.m
@@ -29,3 +29,16 @@
 // CHECK-SDIAG: DependsOnModule.h:1:10: fatal: could not build module 'Module'
 // CHECK-SDIAG: build-fail-notes.m:4:9: note: while building module 'DependsOnModule' imported from
 
+// Also test path canonicalization 
+// RUN: cd %S
+// RUN: not %clang_cc1 -Wincomplete-umbrella -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F Inputs -DgetModuleVersion="epic fail" -serialize-diagnostic-file %t/tmp.diag build-fail-notes.m 2>&1 -fdiagnostics-show-note-include-stack | FileCheck -check-prefix=CHECK-RELATIVE %s
+// CHECK-RELATIVE: While building module 'DependsOnModule' imported from build-fail-notes.m:4:
+// CHECK-RELATIVE-NEXT: While building module 'Module' imported from Inputs{{/|\\}}DependsOnModule.framework{{/|\\}}Headers{{/|\\}}DependsOnModule.h:1:
+// CHECK-RELATIVE-NEXT: In file included from :1:
+// CHECK-RELATIVE-NEXT: Inputs{{/|\\}}Module.framework{{/|\\}}Headers{{/|\\}}Module.h:9:13: error: expected ';' after top level declarator
+
+// RUN: not %clang_cc1 -Wincomplete-umbrella -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F Inputs -DgetModuleVersion="epic fail" -serialize-diagnostic-file %t/tmp.diag build-fail-notes.m 2>&1 -fdiagnostics-show-note-include-stack -fdiagnostics-absolute-paths | FileCheck -check-prefix=CHECK-ABSOLUTE %s
+// CHECK-ABSOLUTE: While building module 'DependsOnModule' imported from {{.+(/|\\)test(/|\\)Modules(/|\\)}}build-fail-notes.m:4:
+// CHECK-ABSOLUTE-NEXT: While building module 'Module' imported from {{.+(/|\\)test(/|\\)Modules(/|\\)Inputs(/|\\)DependsOnModule.framework(/|\\)Headers(/|\\)}}DependsOnModule.h:1:
+// CHECK-ABSOLUTE-NEXT: In file included from {{.+(/|\\)test(/|\\)Modules(/|\\)}}:1:
+// CHECK-ABSOLUTE-NEXT: {{.+(/|\\)test(/|\\)Modules(/|\\)Inputs(/|\\)Module.framework(/|\\)Headers(/|\\)}}Module.h:9:13: error: expected ';' after top level declarator
Index: test/Frontend/absolute-paths.c
===
--- test/Frontend/absolute-paths.c
+++ test/Frontend/absolute-paths.c
@@ -15,3 +15,19 @@
 int g() {}
 // NORMAL: non-existant.c:123:10: warning: control reaches end of non-void function
 // ABSOLUTE: non-existant.c:123:10: wa

[PATCH] D63579: [clang-scan-deps] print the dependencies to stdout and remove the need to use -MD options in the CDB

[Alexandre Ganea via Phabricator via cfe-commits]

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

LGTM! Thank you!


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

https://reviews.llvm.org/D63579



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

[PATCH] D63681: [clang-scan-deps] Introduce the DependencyScanning library with the thread worker code and better error handling

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

LGTM!

Some quick stats on our end (running Windows 10 on a Intel W-2135, 6-core, 3.7 
GHz, NVMe SSD): on a large .SLN compiling approx. 16,000 .CPP files through 600 
unity .CPPs and 23,000 .H files, out of **86 secs** spent in `ClangScanDeps`, 
about **32 secs** are spent in `DirectoryLookup::LookUpFile`.




Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:29
+/// sources either using a fast mode where the source files are minimized, or
+/// using the regular processing run.
+class DependencyScanningWorker {

How do you actually select between the fast mode or the regular preprocess?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63681



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

[PATCH] D63681: [clang-scan-deps] Introduce the DependencyScanning library with the thread worker code and better error handling

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a subscriber: rnk.
aganea added a comment.

A bit more detail on what we're seeing on our end (specs in the post above). 
The 'Count' column represents the number of 1ms samples taken in that function. 
The 'Weight' column is cumulated times for all cores, for a given process, in 
ms.

Image below shows most of the time is spent in `clang-scan-deps.dll` and 
indirectly, in `ntoskrnl.exe`. The graph at the bottom also shows that 
preprocessing the ~600 unity .CPP files in the project issues 16 million IOPS. 
The graphs on the right show no bottlenecks on the disk I/O on the OS I/O-side.

F9417850: clang-scan-deps.jpg 

Top functions in `ntoskrnl.exe`, most callstacks end up in 
`clang-scan-deps.dll`:

F9417863: clang-scan-deps2.jpg 

A good chunk of all this is caused by `llvm::sys::status()`, even though is 
goes through the `FileSystemStatCache`, like I was suggesting previously 
 (ping @rnk)
//(beware, the callstacks are reversed in the image below)//

F9417870: clang-status.JPG 

I can take a look at `llvm::sys::status()` after the vacations (somewhere in 
August)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63681



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

[PATCH] D64301: Use `ln -n` to prevent forming a symlink cycle, instead of rm'ing the source

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D64301#1574304 , @rnk wrote:

> I think @zturner wanted to teach lit to completely remove the Output/ 
> directory for every test so we don't have to deal with stale files from 
> previous tests hanging around. I remember objecting that we can't do that on 
> startup since it's slow and not parallelized. However, I think we could 
> probably add it as an early step to TestRunner.executeShTest so we don't have 
> to add these extra 'rm' commands anymore.


Just a reminder, the `DeleteFile` API, and thus `rm` on Windows are async. See 
this . 
Occasionally, the tests are failing locally on my PC because of that (maybe 1 
out of 50 times).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64301



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

[PATCH] D64301: Use `ln -n` to prevent forming a symlink cycle, instead of rm'ing the source

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D64301#1576615 , @aganea wrote:

> In D64301#1574304 , @rnk wrote:
>
> > I think @zturner wanted to teach lit to completely remove the Output/ 
> > directory for every test so we don't have to deal with stale files from 
> > previous tests hanging around. I remember objecting that we can't do that 
> > on startup since it's slow and not parallelized. However, I think we could 
> > probably add it as an early step to TestRunner.executeShTest so we don't 
> > have to add these extra 'rm' commands anymore.
>
>
> Just a reminder, the `DeleteFile` API, and thus `rm` on Windows are async. 
> There's no guarantee the files were deleted once `rm` completes. See this 
> . 
> Occasionally, the tests are failing locally on my PC because of that (maybe 1 
> out of 50 times).





Repository:
  rL LLVM

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

https://reviews.llvm.org/D64301



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

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC360467: Fixed tests where grep was not matching the linefeed 
(authored by aganea, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61496?vs=197990&id=199069#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61496

Files:
  test/Preprocessor/indent_macro.c
  test/Preprocessor/macro_fn_varargs_named.c
  test/Preprocessor/macro_not_define.c
  test/Preprocessor/macro_rparen_scan.c


Index: test/Preprocessor/macro_rparen_scan.c
===
--- test/Preprocessor/macro_rparen_scan.c
+++ test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:3 ;
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: test/Preprocessor/macro_fn_varargs_named.c
===
--- test/Preprocessor/macro_fn_varargs_named.c
+++ test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix CHECK-1
+// CHECK-1:a: x
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix CHECK-2
+// CHECK-2:b: x y, z,h
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace 
--check-prefix CHECK-3
+// CHECK-3:c: foo(x)
 
 #define A(b, c...) b c
 a: A(x)
Index: test/Preprocessor/macro_not_define.c
===
--- test/Preprocessor/macro_not_define.c
+++ test/Preprocessor/macro_not_define.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK: # define X 3
 
 #define H # 
  #define D define 
Index: test/Preprocessor/indent_macro.c
===
--- test/Preprocessor/indent_macro.c
+++ test/Preprocessor/indent_macro.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:   zzap
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap


Index: test/Preprocessor/macro_rparen_scan.c
===
--- test/Preprocessor/macro_rparen_scan.c
+++ test/Preprocessor/macro_rparen_scan.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^3 ;$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:3 ;
 
 /* Right paren scanning, hard case.  Should expand to 3. */
 #define i(x) 3 
Index: test/Preprocessor/macro_fn_varargs_named.c
===
--- test/Preprocessor/macro_fn_varargs_named.c
+++ test/Preprocessor/macro_fn_varargs_named.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -E %s | grep '^a: x$'
-// RUN: %clang_cc1 -E %s | grep '^b: x y, z,h$'
-// RUN: %clang_cc1 -E %s | grep '^c: foo(x)$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix CHECK-1
+// CHECK-1:a: x
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix CHECK-2
+// CHECK-2:b: x y, z,h
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace --check-prefix CHECK-3
+// CHECK-3:c: foo(x)
 
 #define A(b, c...) b c
 a: A(x)
Index: test/Preprocessor/macro_not_define.c
===
--- test/Preprocessor/macro_not_define.c
+++ test/Preprocessor/macro_not_define.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^ # define X 3$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK: # define X 3
 
 #define H # 
  #define D define 
Index: test/Preprocessor/indent_macro.c
===
--- test/Preprocessor/indent_macro.c
+++ test/Preprocessor/indent_macro.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | grep '^   zzap$'
+// RUN: %clang_cc1 -E %s | FileCheck %s --match-full-lines --strict-whitespace
+// CHECK:   zzap
 
 // zzap is on a new line, should be indented.
 #define BLAH  zzap
___
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] D61945: ftime-trace as a CoreOption

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: thakis, anton-afanasyev.
aganea added a project: clang.

As suggested here  by @thakis , make 
so that `-ftime-trace` can be used directly in clang-cl without `-Xclang`


Repository:
  rC Clang

https://reviews.llvm.org/D61945

Files:
  include/clang/Driver/Options.td
  test/Driver/cl-options.c


Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -638,6 +638,7 @@
 // RUN: -fno-profile-instr-use \
 // RUN: -fcs-profile-generate \
 // RUN: -fcs-profile-generate=dir \
+// RUN: -ftime-trace \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1747,7 +1747,7 @@
 def : Flag<["-"], "fterminated-vtables">, Alias;
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
-def ftime_trace : Flag<["-"], "ftime-trace">, Group, 
Flags<[CC1Option]>;
+def ftime_trace : Flag<["-"], "ftime-trace">, Group, 
Flags<[CC1Option, CoreOption]>;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group, 
Flags<[CC1Option]>;
 def ftrapv : Flag<["-"], "ftrapv">, Group, Flags<[CC1Option]>,
   HelpText<"Trap on integer overflow">;


Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -638,6 +638,7 @@
 // RUN: -fno-profile-instr-use \
 // RUN: -fcs-profile-generate \
 // RUN: -fcs-profile-generate=dir \
+// RUN: -ftime-trace \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1747,7 +1747,7 @@
 def : Flag<["-"], "fterminated-vtables">, Alias;
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
-def ftime_trace : Flag<["-"], "ftime-trace">, Group, Flags<[CC1Option]>;
+def ftime_trace : Flag<["-"], "ftime-trace">, Group, Flags<[CC1Option, CoreOption]>;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group, Flags<[CC1Option]>;
 def ftrapv : Flag<["-"], "ftrapv">, Group, Flags<[CC1Option]>,
   HelpText<"Trap on integer overflow">;
___
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 Pre

[PATCH] D61945: ftime-trace as a CoreOption

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360907: ftime-trace as a CoreOption (authored by aganea, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61945?vs=199604&id=199836#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61945

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/test/Driver/cl-options.c


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1747,7 +1747,7 @@
 def : Flag<["-"], "fterminated-vtables">, Alias;
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
-def ftime_trace : Flag<["-"], "ftime-trace">, Group, 
Flags<[CC1Option]>;
+def ftime_trace : Flag<["-"], "ftime-trace">, Group, 
Flags<[CC1Option, CoreOption]>;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group, 
Flags<[CC1Option]>;
 def ftrapv : Flag<["-"], "ftrapv">, Group, Flags<[CC1Option]>,
   HelpText<"Trap on integer overflow">;
Index: cfe/trunk/test/Driver/cl-options.c
===
--- cfe/trunk/test/Driver/cl-options.c
+++ cfe/trunk/test/Driver/cl-options.c
@@ -638,6 +638,7 @@
 // RUN: -fno-profile-instr-use \
 // RUN: -fcs-profile-generate \
 // RUN: -fcs-profile-generate=dir \
+// RUN: -ftime-trace \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1747,7 +1747,7 @@
 def : Flag<["-"], "fterminated-vtables">, Alias;
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
-def ftime_trace : Flag<["-"], "ftime-trace">, Group, Flags<[CC1Option]>;
+def ftime_trace : Flag<["-"], "ftime-trace">, Group, Flags<[CC1Option, CoreOption]>;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group, Flags<[CC1Option]>;
 def ftrapv : Flag<["-"], "ftrapv">, Group, Flags<[CC1Option]>,
   HelpText<"Trap on integer overflow">;
Index: cfe/trunk/test/Driver/cl-options.c
===
--- cfe/trunk/test/Driver/cl-options.c
+++ cfe/trunk/test/Driver/cl-options.c
@@ -638,6 +638,7 @@
 // RUN: -fno-profile-instr-use \
 // RUN: -fcs-profile-generate \
 // RUN: -fcs-profile-generate=dir \
+// RUN: -ftime-trace \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 
___
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]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361296: [DebugInfo] Don't 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 li

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Could you please move the test to a more approriate location? (ie. 
clang/trunk/test/Driver/)




Comment at: clang/tools/driver/cc1_main.cpp:245
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()

This seems a bit too chatty. Suround these two lines with `if 
(Config->Verbose)` ?



Comment at: llvm/test/Support/check-time-trace.cxx:4
+
+// CHECK: "args":{"name":"clang"}
+

I don't see any other `-ftime-trace` tests, I would add a few more exhaustive 
file format checks here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914



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

[PATCH] D11850: Delay emitting members of dllexport classes until the class is fully parsed (PR23542)

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added subscribers: belkiss, rnk, aganea.
aganea added a comment.
Herald added a subscriber: llvm-commits.

Cross-referencing PR40006 . It 
seems to be a different manifestation of this same bug.

@hans Could you possibly take a look at the bug above whenever you have time? 
Many thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D11850



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

[PATCH] D69043: [RFC] Adding time-trace to LLD?

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: lld/ELF/Driver.cpp:515
+
+  if (llvm::timeTraceProfilerEnabled()) {
+SmallString<128> path(config->outputFile);

Could you possibly move this block (and the init block above) into a new file 
in `lld/Common/` so we can use it in the COFF driver as well?



Comment at: lld/ELF/Driver.cpp:527
+  }
+  return;
 }

Extraneous return.



Comment at: lld/ELF/Options.td:361
 
+def time_trace: F<"time-trace">, HelpText<"Record time trace">;
+

Given this option is a candidate for the other LLD drivers, I am wondering if 
we couldn't have a shared `lld/Common/CommonOpt.td`. @ruiu WDYT?



Comment at: llvm/lib/Support/TimeProfiler.cpp:70
   void begin(std::string Name, llvm::function_ref Detail) {
+std::lock_guard Lock(Mu);
 Stack.emplace_back(steady_clock::now(), TimePointType(), std::move(Name),

The lock is definitely not the way to go, it would skew all the timings 
especially on tens-of-cores machines. Like you're suggesting, make 
`TimeTraceProfilerInstance` a TLS, along with a new combiner upon the call to 
`timeTraceProfilerWrite()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69043



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

[PATCH] D80454: [Clang][test] fix tests when using external assembler

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Could you please attach a test to demonstrate the issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80454



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

[PATCH] D80454: [Clang][test] fix tests when using external assembler

[Alexandre Ganea via Phabricator via cfe-commits]

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

LGTM.

Are you planning on adding another patch for the change that you've sent 
initially? Ideally, I'd like to relax a bit that constraint in `Driver.cpp`, as 
in D74447 , but that requires first to disable 
`-disable-free` to ensure we exit cleanly, at least when runing the tests (a 
bit like the `LLD_IN_TEST` env var).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80454



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D69778#2035805 , @llunak wrote:

> @aganea Have you tried how this version of the patch affects PR44953? If not, 
> could you please try?


I've applied the patch, I don't see the issue raised in PR44953 anymore.

However as discussed offline with @llunak I'm just raising a concern: when 
building with this patch + D69585  
(`-fpch-instantiate-templates`) + `-fmodules-debuginfo -fmodules-codegen` as 
instructed, the PCH compilation in one of games crashes with the callstack 
below. On another game, the compilation succeeds, but I can see link errors for 
a few symbols referenced from the .PCH.OBJ.

This patch may be fine on its own, because `-fmodules-debuginfo 
-fmodules-codegen` are not enabled by default with `/Yc`, but more work is 
needed to make this feature the default. This could be done in a subsequent 
patch.

  Unknown code
  UNREACHABLE executed at 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726!
  Stack dump:
  0.Program arguments: -cc1 -triple x86_64-pc-windows-msvc19.20.0 -emit-obj 
(edited) -building-pch-with-obj -fpch-instantiate-templates -fmodules-codegen 
-fmodules-debuginfo -include-pch D:\(edited)\MyProject.pch 
-pch-through-header=(edited)\precomp.h -include (edited)\precomp.h (edited) 
-fms-compatibility-version=19.20 -std=c++17 -fdelayed-template-parsing 
-finline-functions -fobjc-runtime=gcc -fdiagnostics-show-option 
-fdiagnostics-absolute-paths -vectorize-loops -vectorize-slp -O3 -faddrsig -o 
D:\(edited)\MyProject.pch.obj -x c++ D:\(edited)\precomp.cpp
  1. parser at end of file
  2.Code generation
  3.Running pass 'Function Pass Manager' on module '(edited)\precomp.cpp'.
  4.Running pass 'X86 DAG->DAG Instruction Selection' on function 
'@"?GetDeterminant@Matrix44f@MyNamespace@@QEBAMXZ"'
  #0 0x7ff6bada1c06 HandleAbort 
D:\llvm-project\llvm\lib\Support\Windows\Signals.inc:408:0
  #1 0x7ff6beb4b0e1 raise 
D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
  #2 0x7ff6beb40e5c abort 
D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
  #3 0x7ff6bad58207 llvm::llvm_unreachable_internal(char const *, char 
const *, unsigned int) D:\llvm-project\llvm\lib\Support\ErrorHandling.cpp:210:0
  #4 0x7ff6bbb14d2e llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726:0
  #5 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #6 0x7ff6bbb140b3 llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5671:0
  #7 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #8 0x7ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
  #9 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #10 0x7ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
  #11 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #12 0x7ff6bbb14538 llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5708:0
  #13 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #14 0x7ff6bca4c6fb `anonymous namespace'::DAGCombiner::visit 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1573:0
  #15 0x7ff6bca30e4b `anonymous namespace'::DAGCombiner::combine 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1634:0
  #16 0x7ff6bca3052f llvm::SelectionDAG::Combine(enum llvm::CombineLevel, 
class llvm::AAResults *, enum llvm::CodeGenOpt::Level) 
D:\llvm-projec

[PATCH] D43002: Emit S_OBJNAME symbol in CodeView

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 266989.
aganea added a comment.
Herald added subscribers: cfe-commits, steven_wu, aheejin, arichardson, sbc100, 
emaste.
Herald added a reviewer: espindola.
Herald added a project: clang.

I'm taking over this patch, as discussed offline with Zachary.

I've re-implemented the patch to use the target/final file name of the .OBJ, as 
suggested.

The bulk of the changes is mostly to pass around the file name, down to the 
back end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Frontend/FrontendActions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/tools/driver/cc1_main.cpp
  lld/COFF/LTO.cpp
  lld/COFF/LTO.h
  lld/ELF/LTO.cpp
  lld/wasm/LTO.cpp
  llvm/include/llvm/LTO/LTO.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/Support/ToolOutputFile.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
  llvm/lib/LTO/Caching.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/test/DebugInfo/COFF/globals.ll
  llvm/test/DebugInfo/COFF/multifunction.ll
  llvm/test/DebugInfo/COFF/pr28747.ll
  llvm/test/DebugInfo/COFF/simple.ll
  llvm/test/DebugInfo/COFF/vframe-fpo.ll
  llvm/test/MC/AArch64/coff-debug.ll
  llvm/test/MC/ARM/coff-debugging-secrel.ll
  llvm/test/MC/COFF/cv-compiler-info.ll
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp

Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -352,7 +352,7 @@
 std::error_code EC;
 auto S = std::make_unique(Path, EC, sys::fs::OF_None);
 check(EC, Path);
-return std::make_unique(std::move(S));
+return std::make_unique(std::move(S), Path);
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr MB) {
Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -530,6 +530,9 @@
   GetOutputStream(TheTarget->getName(), TheTriple.getOS(), argv[0]);
   if (!Out) return 1;
 
+  // Ensure the filename is passed down to CodeViewDebug.
+  Target->Options.MCOptions.OutputPathName = Out->outputFilename();
+
   std::unique_ptr DwoOut;
   if (!SplitDwarfOutputFile.empty()) {
 std::error_code EC;
Index: llvm/test/MC/COFF/cv-compiler-info.ll
===
--- llvm/test/MC/COFF/cv-compiler-info.ll
+++ llvm/test/MC/COFF/cv-compiler-info.ll
@@ -1,4 +1,5 @@
-; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s --check-prefixes=CHECK,STDOUT
+; RUN: llc -mtriple i686-pc-windows-msvc < %s -o %t | FileCheck %s --input-file=%t --check-prefixes=CHECK,FILE
 ; ModuleID = 'D:\src\scopes\foo.cpp'
 source_filename = "D:\5Csrc\5Cscopes\5Cfoo.cpp"
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
@@ -20,19 +21,23 @@
 ; One .debug$S section should contain an S_COMPILE3 record that identifies the
 ; source language and the version of the compiler based on the DICompileUnit.
 ; CHECK: 	.section	.debug$S,"dr"
+; CHECK:.short  4353# Record kind: S_OBJNAME
+; CHECK-NEXT:   .long   0   # Signature
+; STDOUT-NEXT:  .byte   0   # Object name
+; FILE-NEXT:.asciz	"{{.*}}{{|/}}cv-compiler-info.ll.tmp" # Object name
 ; CHECK: 		.short	4412  # Record kind: S_COMPILE3
-; CHECK: 		.long	1   # Flags and language
-; CHECK: 		.short	7 # CPUType
-; CHECK: 		.short	4 # Frontend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
-; CHECK-NOT: .short	4412  # Record kind: S_COMPILE3
+; CHECK-NEXT:   .long	1   # Flags and language
+; CHECK-NEXT: 	.short	7 # CPUType
+; CHECK-NEXT: 	.short	4 # Frontend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.asciz	"clang 

[PATCH] D43002: Emit S_OBJNAME symbol in CodeView

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D43002#2061162 , @echristo wrote:

> First question:
>
> Since split dwarf has to do some similar things can we not use the same 
> support? This seems to be a lot of changes for this.
>
> Second question:
>
> and if we can't use the same support can we make split dwarf use this? Having 
> two separate methods for passing everything around is a bit painful.


To answer both questions, I tried to converge but the uses are slightly 
different. In one case (DWARF) the `-split-dwarf-file/-split-dwarf-output` 
flags are passed down almost blindly to the back-end, whereas for COFF there's 
more logic to determine where the file should be emitted (see 
`CompilerInstance::createOutputFile`). We always need full path names in 
`S_OBJNAME`, whereas (it seems) split DWARF can handle relative paths (for 
example see `llvm-mc -split-dwarf-file`). I think the purpose is also 
different: the .dwo is for immediate debugger consumption, whereas the path in 
`S_OBJNAME` seems to be used more like a global identifier.

Please let me know if you feel this could be done differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002



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

[PATCH] D43002: Emit S_OBJNAME symbol in CodeView

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 267111.
aganea added a comment.

Simplified. Now using `CodeGenOptions` to pass the object filename around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/LTO.cpp
  lld/COFF/LTO.h
  lld/ELF/LTO.cpp
  lld/wasm/LTO.cpp
  llvm/include/llvm/LTO/LTO.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/Support/ToolOutputFile.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
  llvm/lib/LTO/Caching.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/test/DebugInfo/COFF/globals.ll
  llvm/test/DebugInfo/COFF/multifunction.ll
  llvm/test/DebugInfo/COFF/pr28747.ll
  llvm/test/DebugInfo/COFF/simple.ll
  llvm/test/DebugInfo/COFF/vframe-fpo.ll
  llvm/test/MC/AArch64/coff-debug.ll
  llvm/test/MC/ARM/coff-debugging-secrel.ll
  llvm/test/MC/COFF/cv-compiler-info.ll
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp

Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -352,7 +352,7 @@
 std::error_code EC;
 auto S = std::make_unique(Path, EC, sys::fs::OF_None);
 check(EC, Path);
-return std::make_unique(std::move(S));
+return std::make_unique(std::move(S), Path);
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr MB) {
Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -530,6 +530,9 @@
   GetOutputStream(TheTarget->getName(), TheTriple.getOS(), argv[0]);
   if (!Out) return 1;
 
+  // Ensure the filename is passed down to CodeViewDebug.
+  Target->Options.MCOptions.COFFOutputFilename = Out->outputFilename();
+
   std::unique_ptr DwoOut;
   if (!SplitDwarfOutputFile.empty()) {
 std::error_code EC;
Index: llvm/test/MC/COFF/cv-compiler-info.ll
===
--- llvm/test/MC/COFF/cv-compiler-info.ll
+++ llvm/test/MC/COFF/cv-compiler-info.ll
@@ -1,4 +1,6 @@
-; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s --check-prefixes=CHECK,STDOUT
+; RUN: llc -mtriple i686-pc-windows-msvc < %s -o %t
+; RUN: FileCheck %s --input-file=%t --check-prefixes=CHECK,FILE
 ; ModuleID = 'D:\src\scopes\foo.cpp'
 source_filename = "D:\5Csrc\5Cscopes\5Cfoo.cpp"
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
@@ -20,19 +22,23 @@
 ; One .debug$S section should contain an S_COMPILE3 record that identifies the
 ; source language and the version of the compiler based on the DICompileUnit.
 ; CHECK: 	.section	.debug$S,"dr"
+; CHECK:.short  4353# Record kind: S_OBJNAME
+; CHECK-NEXT:   .long   0   # Signature
+; STDOUT-NEXT:  .byte   0   # Object name
+; FILE-NEXT:.asciz	"{{.*}}{{|/}}cv-compiler-info.ll.tmp" # Object name
 ; CHECK: 		.short	4412  # Record kind: S_COMPILE3
-; CHECK: 		.long	1   # Flags and language
-; CHECK: 		.short	7 # CPUType
-; CHECK: 		.short	4 # Frontend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
-; CHECK-NOT: .short	4412  # Record kind: S_COMPILE3
+; CHECK-NEXT:   .long	1   # Flags and language
+; CHECK-NEXT: 	.short	7 # CPUType
+; CHECK-NEXT: 	.short	4 # Frontend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
+; CHECK-NOT:.short	4412  # Record kind: S_COMPILE3
 !1 = !DIFile(filename: "D:\5Csrc\5Cscopes\5Cfoo.cpp", directory: "D:\5Csrc\5Cscopes\5Cclang")
 !2 = !{}
 !7 = !{i32 2, !"CodeView", i32 1}
Index: llvm/test/MC/ARM/coff-debugging-secrel.ll
===
--- llvm/test/MC/ARM/coff-debugging-secrel.ll
+++ llvm/test/MC/ARM/coff-debugging-secrel.ll
@@ -42,10 +42,10 @@
 
 ; CHECK-MSVC: Relocations [
 ; CHECK-MSVC:   Section {{.*}} .debug$S {
-; CHECK-MSVC: 0x64 IMAGE_REL_ARM_SECREL function
-; CHECK-MSVC: 0x68 IMAGE_REL_ARM_SECTION function
-; CHECK-MSVC: 0xA0 IMAGE_REL_ARM_SECREL funct

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: hans, amccarth, thakis, mstorsjo, akhuang.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

This patch adds some missing information to the `LF_BUILDINFO` which allows for 
rebuilding a .CPP without any external dependency but the .OBJ itself (other 
than the compiler).

Some external tools that we are using (Recode , 
Live++ ) are extracting the information to 
reproduce a build without any knowledge of the build system. The `LF_BUILDINFO` 
therefore stores a full path to the compiler, the PWD, a relative or absolute 
path to the TU, and the full CC1 command line. The command line needs to be 
freestanding (not depend on any environment variables). In the same way, MSVC 
doesn't store the provided command-line, but an expanded version (somehow their 
equivalent of CC1) which is also freestanding.

For more information see PR36198 and D43002 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80833

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/CreateInvocationFromCommandLine.cpp
  clang/test/CodeGen/debug-info-codeview-buildinfo.c
  clang/tools/driver/cc1_main.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

Index: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -831,6 +831,31 @@
   return TypeTable.writeLeafType(SIR);
 }
 
+static std::string renderCommandLine(ArrayRef CommandLineArgs,
+ StringRef MainFile) {
+  std::string FlatCmdLine;
+  SmallString<128> TempArg;
+  auto maybeQuote = [&TempArg](const char *Arg) -> StringRef {
+const bool Escape =
+StringRef(Arg).find_first_of(" \"\\$") != StringRef::npos;
+if (!Escape)
+  return Arg;
+TempArg.clear();
+(Twine("\"") + Arg + "\"").toVector(TempArg);
+return TempArg;
+  };
+  for (auto &Arg : CommandLineArgs) {
+if (Arg == nullptr)
+  break;
+// The command-line shall not contain the file to compile.
+if (Arg == MainFile)
+  continue;
+FlatCmdLine += maybeQuote(Arg);
+FlatCmdLine += " ";
+  }
+  return FlatCmdLine;
+}
+
 void CodeViewDebug::emitBuildInfo() {
   // First, make LF_BUILDINFO. It's a sequence of strings with various bits of
   // build info. The known prefix is:
@@ -851,6 +876,13 @@
   getStringIdTypeIdx(TypeTable, MainSourceFile->getDirectory());
   BuildInfoArgs[BuildInfoRecord::SourceFile] =
   getStringIdTypeIdx(TypeTable, MainSourceFile->getFilename());
+  if (!StringRef(Asm->TM.Options.MCOptions.BuildTool).empty()) {
+BuildInfoArgs[BuildInfoRecord::BuildTool] =
+getStringIdTypeIdx(TypeTable, Asm->TM.Options.MCOptions.BuildTool);
+BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+TypeTable, renderCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
+ MainSourceFile->getFilename()));
+  }
   // FIXME: Path to compiler and command line. PDB is intentionally blank unless
   // we implement /Zi type servers.
   BuildInfoRecord BIR(BuildInfoArgs);
Index: llvm/include/llvm/MC/MCTargetOptions.h
===
--- llvm/include/llvm/MC/MCTargetOptions.h
+++ llvm/include/llvm/MC/MCTargetOptions.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_MC_MCTARGETOPTIONS_H
 #define LLVM_MC_MCTARGETOPTIONS_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include 
 #include 
 
@@ -59,6 +60,9 @@
   std::string AssemblyLanguage;
   std::string SplitDwarfFile;
 
+  const char *BuildTool = nullptr;
+  ArrayRef CommandLineArgs;
+
   /// Additional paths to search for `.include` directives when using the
   /// integrated assembler.
   std::vector IASSearchPaths;
Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -203,8 +203,7 @@
   }
 }
 
-extern int cc1_main(ArrayRef Argv, const char *Argv0,
-void *MainAddr);
+extern int cc1_main(ArrayRef Argv, void *MainAddr);
 extern int cc1as_main(ArrayRef Argv, const char *Argv0,
   void *MainAddr);
 extern int cc1gen_reproducer_main(ArrayRef Argv,
@@ -327,7 +326,7 @@
   StringRef Tool = ArgV[1];
   void *GetExecutablePathVP = (void *)(intptr_t)GetExecutablePath;
   if (Tool == "-cc1")
-return cc1_main(makeArrayRef(ArgV).slice(2), ArgV[0], GetExecutablePathVP);
+return cc1_main(makeArrayRef(ArgV), GetExecutablePathVP);
   if (Tool == "-cc1as")
   

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

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

As requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/CreateInvocationFromCommandLine.cpp
  clang/test/CodeGen/debug-info-codeview-buildinfo.c
  clang/tools/driver/cc1_main.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

Index: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -831,6 +831,31 @@
   return TypeTable.writeLeafType(SIR);
 }
 
+static std::string renderCommandLine(ArrayRef CommandLineArgs,
+ StringRef MainFile) {
+  std::string FlatCmdLine;
+  SmallString<128> TempArg;
+  auto maybeQuote = [&TempArg](const char *Arg) -> StringRef {
+const bool Escape =
+StringRef(Arg).find_first_of(" \"\\$") != StringRef::npos;
+if (!Escape)
+  return Arg;
+TempArg.clear();
+(Twine("\"") + Arg + "\"").toVector(TempArg);
+return TempArg;
+  };
+  for (auto &Arg : CommandLineArgs) {
+if (Arg == nullptr)
+  break;
+// The command-line shall not contain the file to compile.
+if (Arg == MainFile)
+  continue;
+FlatCmdLine += maybeQuote(Arg);
+FlatCmdLine += " ";
+  }
+  return FlatCmdLine;
+}
+
 void CodeViewDebug::emitBuildInfo() {
   // First, make LF_BUILDINFO. It's a sequence of strings with various bits of
   // build info. The known prefix is:
@@ -851,8 +876,14 @@
   getStringIdTypeIdx(TypeTable, MainSourceFile->getDirectory());
   BuildInfoArgs[BuildInfoRecord::SourceFile] =
   getStringIdTypeIdx(TypeTable, MainSourceFile->getFilename());
-  // FIXME: Path to compiler and command line. PDB is intentionally blank unless
-  // we implement /Zi type servers.
+  if (!StringRef(Asm->TM.Options.MCOptions.BuildTool).empty()) {
+BuildInfoArgs[BuildInfoRecord::BuildTool] =
+getStringIdTypeIdx(TypeTable, Asm->TM.Options.MCOptions.BuildTool);
+BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+TypeTable, renderCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
+ MainSourceFile->getFilename()));
+  }
+  // FIXME: PDB is intentionally blank unless we implement /Zi type servers.
   BuildInfoRecord BIR(BuildInfoArgs);
   TypeIndex BuildInfoIndex = TypeTable.writeLeafType(BIR);
 
Index: llvm/include/llvm/MC/MCTargetOptions.h
===
--- llvm/include/llvm/MC/MCTargetOptions.h
+++ llvm/include/llvm/MC/MCTargetOptions.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_MC_MCTARGETOPTIONS_H
 #define LLVM_MC_MCTARGETOPTIONS_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include 
 #include 
 
@@ -59,6 +60,9 @@
   std::string AssemblyLanguage;
   std::string SplitDwarfFile;
 
+  const char *BuildTool = nullptr;
+  ArrayRef CommandLineArgs;
+
   /// Additional paths to search for `.include` directives when using the
   /// integrated assembler.
   std::vector IASSearchPaths;
Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -203,8 +203,7 @@
   }
 }
 
-extern int cc1_main(ArrayRef Argv, const char *Argv0,
-void *MainAddr);
+extern int cc1_main(ArrayRef Argv, void *MainAddr);
 extern int cc1as_main(ArrayRef Argv, const char *Argv0,
   void *MainAddr);
 extern int cc1gen_reproducer_main(ArrayRef Argv,
@@ -327,7 +326,7 @@
   StringRef Tool = ArgV[1];
   void *GetExecutablePathVP = (void *)(intptr_t)GetExecutablePath;
   if (Tool == "-cc1")
-return cc1_main(makeArrayRef(ArgV).slice(2), ArgV[0], GetExecutablePathVP);
+return cc1_main(makeArrayRef(ArgV), GetExecutablePathVP);
   if (Tool == "-cc1as")
 return cc1as_main(makeArrayRef(ArgV).slice(2), ArgV[0],
   GetExecutablePathVP);
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -181,7 +181,7 @@
   return 0;
 }
 
-int cc1_main(ArrayRef Argv, const char *Argv0, void *MainAddr) {
+int cc1_main(ArrayRef Argv, void *MainAddr) {
   ensureSufficientStack();
 
   std::unique_ptr Clang(new CompilerInstance());
@@ -203,12 +203,11 @@
   IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
   TextDiagnosticBuffer *DiagsBuffer = new TextDiagnosticBuffer;
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagsBuffer);
-

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added inline comments.



Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:3
+// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s
+
+int main() { return 42; }

Is there a way to launch an arbitrary program from lit? I started extracting 
the tool and the cmd-line from the .OBJ, to run it again. But I couldn't figure 
out how to start the extracted tool pathname? Something along the lines of
```
// RUN: eval $CMD
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833



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

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 2 inline comments as done.
aganea added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:320
+  /// Executable and command-line used to create a given CompilerInvocation.
+  const char *BuildTool = nullptr;
+  ArrayRef CommandLineArgs;

hans wrote:
> The name BuildTool makes me think of things like Make or MSBuild rather than 
> the compiler. And in this case we know the compiler is Clang :-) Also since 
> this is for capturing the cc1 arguments, it shouldn't really matter if it's 
> clang-cl, clang++ or just clang. So it seems unfortunate that it has to be 
> piped through all the way like this..
> 
> Is there some way we could just grab the path to the current clang binary 
> during the pdb writing?
There's `sys::fs::getMainExecutable(const char *argv0, void *MainExecAddr)` but 
that's not guaranteed to work on all platforms, you still need to provide 
argv[0] to fallback, see 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Unix/Path.inc#L232
I'm not well versed in Unix distros, can this only rarely occur?
Would you prefer that I used that function instead? (it is ok on Windows though)




Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:835
+static std::string renderCommandLine(ArrayRef CommandLineArgs,
+ StringRef MainFile) {
+  std::string FlatCmdLine;

hans wrote:
> Don't we already have code somewhere that can do this quoting? E.g. the code 
> that prints the cc1 args for "clang -v"?
Yes, the code below was copy-pasted from `Command::printArg`. But that's in 
Clang. Should I make that code common somewhere in `llvm/lib/Support`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833



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

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added a comment.

In D80833#2069246 , @amccarth wrote:

> Does the full path to the tool end up only in the object file, or does this 
> make it all the way to the final executable or library?


The `LF_BUILDINFO` records for each .OBJ are emitted as they are in the final 
.PDB.

> Does embedding full paths affect distributed builds or build reproducibility?

It does affect reproducibility for distributed builds, the contents of 
`LF_BUILDINFO` for a remotely-compiled .OBJ will be different in the final .PDB.
In our case, compilation jobs can be executed locally or remotely, depending on 
how they are queued.
For example,

- If a local compilation, `C:\.nuget\llvm.10.0.0.2\bin\clang-cl.exe` would be 
stamped in the `LF_BUILDINFO` record.
- If a remote compilation, 
`C:\Users\SomeUser\AppData\Local\Temp\.fbuild.tmp\worker\toolchain.130589cdf35aed3b\clang-cl.exe`
 would be stamped (the compiler executable is sent to the remote worker).

But you've got a good point. We would need an way to force the remote compiler 
to stamp the local path (which is unique for all users). We've got a manual 
override if the compiler path referred by `LF_BUILDINFO` can't be found 
locally, but this is a bit annoying, see: 
https://liveplusplus.tech/docs/documentation.html#FASTBuild.
It is better if the right information was there in the first place in the .OBJ. 
Could we do a remapping instead through VFS maybe? I'd like to avoid adding 
yet-a-new-flag to handle that.




Comment at: clang/include/clang/Basic/CodeGenOptions.h:320
+  /// Executable and command-line used to create a given CompilerInvocation.
+  const char *BuildTool = nullptr;
+  ArrayRef CommandLineArgs;

hans wrote:
> aganea wrote:
> > hans wrote:
> > > The name BuildTool makes me think of things like Make or MSBuild rather 
> > > than the compiler. And in this case we know the compiler is Clang :-) 
> > > Also since this is for capturing the cc1 arguments, it shouldn't really 
> > > matter if it's clang-cl, clang++ or just clang. So it seems unfortunate 
> > > that it has to be piped through all the way like this..
> > > 
> > > Is there some way we could just grab the path to the current clang binary 
> > > during the pdb writing?
> > There's `sys::fs::getMainExecutable(const char *argv0, void *MainExecAddr)` 
> > but that's not guaranteed to work on all platforms, you still need to 
> > provide argv[0] to fallback, see 
> > https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Unix/Path.inc#L232
> > I'm not well versed in Unix distros, can this only rarely occur?
> > Would you prefer that I used that function instead? (it is ok on Windows 
> > though)
> > 
> I think it can occur for example if the compiler is running in a chroot. And 
> probably other cases as well.
> So maybe piping the path through is better. But perhaps we should just call 
> it argv0?
> Or could we get away with putting just "clang" there? Does the tool consuming 
> this info actually need the real path to the compiler or could we rely on it 
> finding it on PATH?
Ok for argv0.
Users can have several versions of a toolchain installed locally, if they 
switch between branches or if working on different games at the same time.
We need to identify the right compiler through its full path, we can't rely on 
PATH.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833



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

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added a comment.

I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been done 
by Reid a while ago: D53179  (it already 
stores absolute paths)

However absolute paths are stored by design, if we want to reproduce the build 
locally. I can't see how this feature could be useful if we didn't store 
absolute paths. The user could have many different branches of the same game 
synced locally (we're using perforce).

We could somehow defer the full path calculation to link-time if we didn't want 
full paths in the .OBJs. But each `LF_BUILDINFO` in the final .PDB needs to 
store full local paths.
Please let me know what direction should be taken.

This is what I have now:

  > cat b.cpp
  int f() { return 42; }
  
  # With Microsoft cl.exe (VS2019):
  
  > cl b.cpp /c /Z7
  > llvm-pdbutil dump -all b.obj | grep LF_BUILDINFO -A 5 -B 6
  0x1007 | LF_STRING_ID [size = 248] ID: , String: -c -Z7 -MT 
-I"C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\ATLMFC\include" 
-I"C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\include" -I"C:\Program
  0x1008 | LF_STRING_ID [size = 268] ID: , String:  Files 
(x86)\Windows Kits\NETFXSDK\4.8\include\um" -I"C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\ucrt" -I"C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\shared" -I"C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\um"
  0x1009 | LF_SUBSTR_LIST [size = 16]
   0x1007: `-c -Z7 -MT -I"C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\ATLMFC\include" 
-I"C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\include" -I"C:\Program`
   0x1008: ` Files (x86)\Windows Kits\NETFXSDK\4.8\include\um" 
-I"C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\ucrt" 
-I"C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\shared" 
-I"C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\um"`
  0x100A | LF_STRING_ID [size = 160] ID: 0x1009, String:  -I"C:\Program Files 
(x86)\Windows Kits\10\include\10.0.18362.0\winrt" -I"C:\Program Files 
(x86)\Windows Kits\10\include\10.0.18362.0\cppwinrt" -TP -X
  0x100B | LF_BUILDINFO [size = 28]
   0x1003: `D:\llvm-project`
   0x1004: `C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\bin\HostX64\x64\cl.exe`
   0x1005: `b.cpp`
   0x1006: `D:\llvm-project\vc140.pdb`
   0x100A: ` -I"C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\winrt" -I"C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\cppwinrt" -TP -X`
  
  # With Clang:
  
  > clang-cl b.cpp /c /Z7
  > llvm-pdbutil dump -all b.obj | grep LF_BUILDINFO -A 5
  0x1007 | LF_BUILDINFO [size = 28]
   0x1003: `D:\llvm-project`
   0x1005: `D:\llvm-project\buildninjaDebMSVC\bin\clang-cl.exe`
   0x1004: `b.cpp`
   : ``
   0x1006: `-cc1 -triple x86_64-pc-windows-msvc19.26.28806 -emit-obj 
-mrelax-all -mincremental-linker-compatible -disable-free -main-file-name 
-mrelocation-model pic -pic-level 2 -mthread-model posix -mframe-pointer=none 
-relaxed-aliasing -fmath-errno -fno-rounding-math -mconstructor-aliases 
-munwind-tables -target-cpu x86-64 -mllvm -x86-asm-syntax=intel -D_MT 
-flto-visibility-public-std --dependent-lib=libcmt --dependent-lib=oldnames 
-stack-protector 2 -fms-volatile -fdiagnostics-format msvc -gcodeview 
-debug-info-kind=limited -resource-dir 
"D:\llvm-project\buildninjaDebMSVC\lib\clang\11.0.0" -internal-isystem 
"D:\llvm-project\buildninjaDebMSVC\lib\clang\11.0.0\include" -internal-isystem 
"C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\ATLMFC\include" 
-internal-isystem "C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\include" -internal-isystem 
"C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um" -internal-isystem 
"C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\ucrt" 
-internal-isystem "C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\shared" -internal-isystem "C:\Program Files 
(x86)\Windows Kits\10\include\10.0.18362.0\um" -internal-isystem "C:\Program 
Files (x86)\Windows Kits\10\include\10.0.18362.0\winrt" -internal-isystem 
"C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\cppwinrt" 
-fdeprecated-macro -fdebug-compilation-dir "D:\llvm-project" -ferror-limit 19 
-fmessage-length=120 -fno-use-cxa-atexit -fms-extensions -fms-compatibility 
-fms-compatibility-version=19.26.28806 -std=c++14 -fdelayed-template-parsing 
-fcolor-diagnostics -faddrsig -o b.obj -x c++ `




Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:8
+// CHECK: 
+// CH

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 5 inline comments as done.
aganea added a comment.

In D80833#2071863 , @hans wrote:

> In D80833#2071818 , @aganea wrote:
>
> > I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been 
> > done by Reid a while ago: D53179  (it 
> > already stores absolute paths)
>
>
> That's surprising to me. Nico is the real export on reproducible builds, but 
> we generally try very hard to make builds reproducible independent of source 
> and build dir. Maybe there should be a flag controlling this? /Brepro?


Ah, you must be using `-fdebug-compilation-dir` on Chromium I suppose, to make 
paths relative in .OBJs. Otherwise the default is the full path to PWD/CWD. 
Added a test for that.

In D80833#2071969 , @thakis wrote:

> We don't store physical absolute paths in pdbs if you hold things right. Look 
> for /pdbsourcepath: in 
> http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html . 
> rnk's change seems to store the main TU's dir as cwd, which likely either 
> takes the dir from the main TU as passed (ie if you pass an erlative path, it 
> stores that), or it honors /pdbsourcepath:. If that's fine as-is, that's ok. 
> It's different from what `pwd` returns though I think. (imagine you're in 
> c:\foo and you build bar\baz.cc. the cwd in the pdb will be c:\foo\bar but 
> the command will be relative to c:\foo if I understand things right? I might 
> not though.)


Without `-fdebug-compilation-dir`, the **full path **is stored in 
`LF_BUILDINFO` (see `CodeGenOpts::DebugCompilationDir` and 
`CGDebugInfo::getCurrentDirname()`)
With `-fdebug-compilation-dir .`, the PWD/CWD becomes relative ('.') and I've 
added code in LLD to call `pdbMakeAbsolute` on it, like for other PDB strings.
Now `/pdbsourcepath` works as well (all this is covered in 
lld/test/COFF/pdb-relative-source-lines.test).
However if you're in `c:\foo` and you do `clang-cl /c bar\baz.cc`, then 
`bar\baz.cc` will be stored as-it-is in `LF_BUILDINFO` (the PWD/CWD and the 
main filename are stored separately)

The only absolute paths that remain are: a. the compiler path 
(`D:\llvm-project\buildninjaDebMSVC\bin\clang-cl.exe` in the yaml below) and b. 
the `-internal-isystem` paths. However that is not an issue on our end, as 
we're building with `-nostdinc` + nuget packages installed in a fixed dir on 
all users' machines. Not sure how that works for you?




Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:835
+static std::string renderCommandLine(ArrayRef CommandLineArgs,
+ StringRef MainFile) {
+  std::string FlatCmdLine;

aganea wrote:
> hans wrote:
> > Don't we already have code somewhere that can do this quoting? E.g. the 
> > code that prints the cc1 args for "clang -v"?
> Yes, the code below was copy-pasted from `Command::printArg`. But that's in 
> Clang. Should I make that code common somewhere in `llvm/lib/Support`?
Replaced by `llvm::sys::flattenWindowsCommandLine`. I can't find any equivalent 
for Linux, I hope just aggregating the arguments with spaces in between is fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833



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