[PATCH] D72390: [www] Remove stale text about default c++ standard from cxx_status.html

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop abandoned this revision.
russell.gallop added a comment.
Herald added a project: All.

Latest page looks fine: https://clang.llvm.org/cxx_status.html#cxx11


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72390

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

[PATCH] D131665: [CMake] Support passing arguments to build tool (bootstrap).

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop resigned from this revision.
russell.gallop added a comment.

Resign as reviewer as I work with Carlos (and am not familiar enough with the 
details of this area).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131665

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

[PATCH] D131665: [CMake] Support passing arguments to build tool (bootstrap).

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Noting related review: https://reviews.llvm.org/D115815 which added this 
variable to support this for other "external projects".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131665

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop abandoned this revision.
russell.gallop added a comment.

In D96120#2597877 , @kcc wrote:

> I am afraid we will have to delete the older (non-standalone) variant 
> entirely. 
> (And the sooner the better)

Okay, think we found a few things out by doing this before porting standalone.

If I find time I’ll have a go at removing the sanitizer version.

The scudo documentation (https://llvm.org/docs/ScudoHardenedAllocator.html) 
still seems to refer to the sanitizer version so will need updating.


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

https://reviews.llvm.org/D96120

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2591368 , @mstorsjo wrote:

> Yes, it would need something similar - I tried whipping something together, 
> which after some tweaks seems to work:
> Feel free to squash that into your patch (which saves me a bit of effort) :-)

Thanks for doing that. I've added this in.

> ...I did test building the double-free.cpp test with `-fsanitize=scudo`, and 
> it seems to work as it should for all three cases it tests...

Great, thanks.


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

https://reviews.llvm.org/D96120

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 327123.
russell.gallop added a comment.

Update with suggested changes to MinGW.cpp


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

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size = sysconf(_SC_PAGESIZE);
-  size_t size = (1U << 17) + page_size;
-  struct sigaction a;
+  long page_size = getsystempagesize();
+  size_t size = (1U << 19) + page_size;
 
   assert(argc == 2);
-  memset(, 0, sizeof(a));
-  a.sa_sigaction = handler;
-  a.sa_flags = SA_SIGINFO;
 
   char *p = (char *)malloc(size);
   assert(p);
   memset(p, 'A', size); // This should not trigger anything.
   // Set up the SIGSEGV handler now, as the rest should trigger an AV.
+#ifdef _WIN32
+  SetUnhandledExceptionFilter(handler);
+#else
+  struct sigaction a = {0};
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, , NULL);
+#endif
+
   if (!strcmp(argv[1], 

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2591418 , @vitalybuka wrote:

>>> This is intended as a step to porting scudo standalone.
>
> Why this is needed for scudo stadalone?

It’s not strictly needed. It seemed like it was easier, as some of the work had 
already been done, and some of the issues worked through on this would probably 
also come up when porting standalone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop marked 2 inline comments as done.
russell.gallop added a comment.

In D96120#2550941 , @mstorsjo wrote:

> In D96120#2550876 , @russell.gallop 
> wrote:
>
>> In D96120#2546077 , @mstorsjo wrote:
>>
>>> As is, this breaks compilation for mingw. With the three modifications I 
>>> suggest here, it no longer breaks compilation for me - I have no idea if it 
>>> actually works in mingw configurations though, but not breaking compilation 
>>> is at least the first step.
>>
>> Thanks for the information I'll try to run up a mingw environment and check 
>> it works.
>
> In case that turns out to be tricky, I might be able to help with that, at 
> least for building a simple test program with it and running it, if you say 
> how it's supposed to be used  (building/linking with `-fsanitize=scudo`?) and 
> how to inspect whether it actually works.

Hi @mstorsjo. Thanks for the suggestions. I tried running up an mingw 
environment with msys but had trouble getting it working (running into cmake 
issues). Would you be able to help?

Yes, building and linking a program with -fsanitize=scudo is the simple way to 
build a simple program. The support for this is in 
clang/lib/Driver/ToolChains/MSVC.cpp. Would this require support in 
clang/lib/Driver/ToolChains/MinGW.cpp?

I think the best way to test is to run the scudo LIT tests with (e.g.) "ninja 
check-scudo". These build and run some simple test programs and check that 
problems are detected.

Beyond this I built LLVM with scudo and ran check-all. I don't know whether you 
want to go this far. Note that -fsanitize=scudo doesn't work for the MSVC LLVM 
build which uses link/lld-link for linking rather than going via the compiler 
driver so I hooked it into LLVM_INTEGRATED_CRT_ALLOC (see 
https://reviews.llvm.org/D96133). If mingw uses the compiler driver to link 
(like Linux) then you may be able to use -DLLVM_USE_SANITIZER=Scudo on a stage2 
build. This would require some changes to 
llvm/cmake/modules/HandleLLVMOptions.cmake to permit it (I had similar changes 
in https://reviews.llvm.org/D86694). It's harder to tell whether this has 
worked correctly. With MSVC faster ThinLTO links on multi-core machines are a 
good indicator but I guess mingw wouldn't be using the MSVC allocator.

Thanks
Russ




Comment at: compiler-rt/lib/scudo/scudo_new_delete.cpp:30
+#ifdef _WIN64
+COMMENT_EXPORT("??2@YAPEAX_K@Z")// operator new
+COMMENT_EXPORT("??2@YAPEAX_KAEBUnothrow_t@std@@@Z") // operator new nothrow

mstorsjo wrote:
> These don't work in this form for mingw targets (which use the itanium c++ 
> abi).
> 
> By changing `#if SANTIZER_WINDOWS` into `#if SANITIZER_WINDOWS && 
> !defined(__MINGW32__)`, I fixed the linker errors at least.
I've applied this suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 326716.
russell.gallop added a comment.

Added comment on AllocatorSize.

Applied Mingw changes suggested by @mstorsjo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size = sysconf(_SC_PAGESIZE);
-  size_t size = (1U << 17) + page_size;
-  struct sigaction a;
+  long page_size = getsystempagesize();
+  size_t size = (1U << 19) + page_size;
 
   assert(argc == 2);
-  memset(, 0, sizeof(a));
-  a.sa_sigaction = handler;
-  a.sa_flags = SA_SIGINFO;
 
   char *p = (char *)malloc(size);
   assert(p);
   memset(p, 'A', size); // This should not trigger anything.
   // Set up the SIGSEGV handler now, as the rest should trigger an AV.
+#ifdef _WIN32
+  SetUnhandledExceptionFilter(handler);
+#else
+  struct sigaction a = {0};
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, , NULL);

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2545151 , @aganea wrote:

> A few questions: Does this work on x86 targets?

I haven't tried. I'll test this out.

> Are the scudo tests below being built with /MD or with /MT?

The lit tests? They don't specify either. Does that imply /MT?

> Would this change compile/work on MinGW as well?

I'll check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2546077 , @mstorsjo wrote:

> As is, this breaks compilation for mingw. With the three modifications I 
> suggest here, it no longer breaks compilation for me - I have no idea if it 
> actually works in mingw configurations though, but not breaking compilation 
> is at least the first step.

Thanks for the information I'll try to run up a mingw environment and check it 
works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

I believe that the MCJIT failures are due to bug 
https:/bugs.llvm.org/show_bug.cgi?id=24978 rather than a problem in the Scudo 
port so I have added details of how I hit it to that bugzilla and opened two 
reviews to get this landed:
https://reviews.llvm.org/D96120 - [scudo] Port scudo sanitizer to Windows
https://reviews.llvm.org/D96133 - Allow building with scudo memory allocator on 
Windows

Please can you take a look?

I have not added the LLVM_USE_SANITIZER support for scudo from this review. 
-fsanitize adds scudo libraries when the linker is driven from clang, and the 
LLVM cmake build on Windows uses link.exe directly so this doesn't help. And 
scudo sanitizer is planned to go away so I didn't think it made sense to add 
that support on Linux either.


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

https://reviews.llvm.org/D86694

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

[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: cryptoad, aganea.
Herald added subscribers: phosek, mgorny.
russell.gallop requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

Based on https://reviews.llvm.org/D42519, this ports the sanitizer version of 
scudo to Windows.

Passes lit tests and when used as the allocator for LLVM, that passes 
check-all. Have noticed that on LLVM with Scudo, 
https://bugs.llvm.org/show_bug.cgi?id=24978 does intermittently occur when 
running lli tests.

For more details of evaluation see https://reviews.llvm.org/D86694

A separate review will allow hooking this in as the memory allocator for LLVM.

I'm aware that scudo sanitizer version is not under active development. This is 
intended as a step to porting scudo standalone.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size = 

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

I've focussed on the test test-global-init-nonzero-sm-pic.ll which fails 
writing to an address which (I believe) should be in the .data section but 
isn't.

With some breakpoints in SectionMemoryManager.cpp it appears that this fails 
when the top 32bits of the .text allocated address and the .data allocated 
address are different:

  Allocating Data section ".data"
  Returning aligned address 0x01ed 30f6 of size 0x0004
  Allocating code section ".text"
  Returning aligned address 0x022d 3105 of size 0x003a

And work when they happen to be the same. When this fails, the address causing 
the access violation is the top 32bits of the .text address and the bottom 
32bits of the .data address, e.g. 0x022d 30f6. This doesn't fail 
without scudo as the top 32bits of the addresses seem to always be the same.

With assertions enabled this is causing an assert:

  
f:\git\llvm-project\stage1_scudo>"f:\git\llvm-project\stage1_scudo\bin\lli.exe" 
"-mtriple=x86_64-pc-windows-msvc-elf" "-relocation-model=pic" 
"-code-model=small" 
"f:\git\llvm-project\llvm\test\ExecutionEngine\MCJIT\test-global-init-nonzero-sm-pic.ll"
  Assertion failed: isInt<32>(RealOffset), file 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyldELF.cpp, 
line 300
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
  Stack dump:
  0.  Program arguments: f:\\git\\llvm-project\\stage1_scudo\\bin\\lli.exe 
-mtriple=x86_64-pc-windows-msvc-elf -relocation-model=pic -code-model=small 
f:\\git\\llvm-project\\llvm\\test\\ExecutionEngine\\MCJIT\\test-global-init-nonzero-sm-pic.ll
   #0 0x7ff63eb423c5 HandleAbort 
F:\git\llvm-project\llvm\lib\Support\Windows\Signals.inc:408:0
   #1 0x7ff63f89d951 raise 
minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
   #2 0x7ff63f8617cc abort 
minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
   #3 0x7ff63f8825b8 common_assert_to_stderr 
minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:175:0
   #4 0x7ff63f882d42 _wassert 
minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:443:0
   #5 0x7ff63e749e96 llvm::RuntimeDyldELF::resolveX86_64Relocation(class 
llvm::SectionEntry const &, unsigned __int64, unsigned __int64, unsigned int, 
__int64, unsigned __int64) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyldELF.cpp:302:0
   #6 0x7ff63e7496be llvm::RuntimeDyldELF::resolveRelocation(class 
llvm::RelocationEntry const &, unsigned __int64) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyldELF.cpp:932:0
   #7 0x7ff63e72e656 llvm::RuntimeDyldImpl::resolveRelocationList(class 
llvm::SmallVector const &, unsigned __int64) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyld.cpp:1077:0
   #8 0x7ff63e72e52c ::operator++ C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\list:166:0
   #9 0x7ff63e72e52c ::operator++ C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\list:247:0
  #10 0x7ff63e72e52c llvm::RuntimeDyldImpl::resolveLocalRelocations(void) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyld.cpp:145:0
  #11 0x7ff63e72e872 llvm::RuntimeDyldImpl::resolveRelocations(void) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\RuntimeDyld\RuntimeDyld.cpp:139:0
  #12 0x7ff63e532601 llvm::MCJIT::finalizeLoadedModules(void) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\MCJIT\MCJIT.cpp:245:0
  #13 0x7ff63e532bfd ::{dtor} 
F:\git\llvm-project\llvm\include\llvm\ADT\SmallVector.h:1045:0
  #14 0x7ff63e532bfd llvm::MCJIT::finalizeObject(void) 
F:\git\llvm-project\llvm\lib\ExecutionEngine\MCJIT\MCJIT.cpp:271:0
  #15 0x7ff63dcfa06c main F:\git\llvm-project\llvm\tools\lli\lli.cpp:633:0
  #16 0x7ff63f81527c invoke_main 
d:\agent\_work\63\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78:0
  #17 0x7ff63f81527c __scrt_common_main_seh 
d:\agent\_work\63\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
  #18 0x7ffd145a7034 (C:\WINDOWS\System32\KERNEL32.DLL+0x17034)
  #19 0x7ffd1657d0d1 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x4d0d1)


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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

I managed to get this to fail in the debugger (for the cross-module-sm-pic-a.ll 
test):

01655e9d001e()  Unknown
01655e9d0019()  Unknown
017e5eb6b410()  Unknown
017c5eb63810()  Unknown
017e5eb6b410()  Unknown
017e5eb6b410()  Unknown
016f5eb65b58()  Unknown
  > lli.exe!llvm::MCJIT::runFunction(llvm::Function * F, 
llvm::ArrayRef ArgValues) Line 578 C++
lli.exe!llvm::ExecutionEngine::runFunctionAsMain(llvm::Function * Fn, 
const std::vector> & argv, const char * 
const * envp) Line 467C++
lli.exe!main(int argc, char * * argv, char * const * envp) Line 646 
C++
[Inline Frame] lli.exe!invoke_main() Line 78C++
lli.exe!__scrt_common_main_seh() Line 288   C++
kernel32.dll!7ffd145a7034() Unknown
ntdll.dll!7ffd1657d0d1()Unknown

It appears to be failing within the interpreted code. Maybe this is a problem 
between the memory allocator and the Memory Manager.

I tried running these tests repeatedly with scudo (sanitizer) on Linux but 
didn't see it fail. I also tried repeating these tests with rpmalloc on Windows 
and didn't see the failures so it does appear to be specific to scudo on 
Windows. I'll keep investigating.


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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 320806.
russell.gallop added a comment.

Remove -fsanitize=scudo support for Windows in LLVM cmake, using 
LLVM_INTEGRATED_CRT_ALLOC instead.
Remove scudo_cxx from LLVM_INTEGRATED_CRT_ALLOC.


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

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/lib/Support/CMakeLists.txt

Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -58,7 +58,7 @@
   string(REGEX REPLACE "(/|)$" "" LLVM_INTEGRATED_CRT_ALLOC "${LLVM_INTEGRATED_CRT_ALLOC}")
 
   if(NOT EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}")
-message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc and mimalloc are supported.")
+message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc, mimalloc and scudo are supported.")
   endif()
 
   if(LLVM_INTEGRATED_CRT_ALLOC MATCHES "rpmalloc$")
@@ -73,6 +73,8 @@
 	  message(FATAL_ERROR "Cannot find the mimalloc static library. To build it, first apply the patch from https://github.com/microsoft/mimalloc/issues/268 then build the Release x64 target through ${LLVM_INTEGRATED_CRT_ALLOC}\\ide\\vs2019\\mimalloc.sln")
 endif()
 set(system_libs ${system_libs} "${MIMALLOC_LIB}" "-INCLUDE:malloc")
+  elseif(LLVM_INTEGRATED_CRT_ALLOC MATCHES "scudo")
+set(system_libs ${system_libs} "${LLVM_INTEGRATED_CRT_ALLOC}" "-INCLUDE:malloc")
   endif()
 endif()
 
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -790,6 +790,9 @@
 elseif (LLVM_USE_SANITIZER STREQUAL "Leaks")
   append_common_sanitizer_flags()
   append("-fsanitize=leak" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "Unsupported value of LLVM_USE_SANITIZER: ${LLVM_USE_SANITIZER}")
 endif()
Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 312827.
russell.gallop edited the summary of this revision.
russell.gallop added a comment.

Apologies for the delay, I've had other things taking my time.

Latest version uploaded. This fixes stage1 lit tests (on Windows and Linux) and 
adds scudo_cxx to linking.

Usage has changed slightly, now -DLLVM_INTEGRATED_CRT_ALLOC should point to the 
directory with scudo built libraries (e.g. 
c:/git/llvm-project/stage1/lib/clang/12.0.0/lib/windows) rather than the 
library itself. -fsanitize=scudo works for the lit tests (as clang adds the 
libraries when driving the linker) but doesn't work for building LLVM as cmake 
links with the linker directly so this uses LLVM_INTEGRATED_CRT_ALLOC.

There are two issues I'm aware of now:

- Linking c-index-test.exe with scudo fails due to new being redefined

  lld-link: error: void * __cdecl operator new(unsigned __int64) was replaced

Or from link.exe

  clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: 
"void * __cdecl operator new(unsigned __int64)" (??2@YAPEAX_K@Z) already 
defined in libclang.lib(libclang.dll)
  ...



- With scudo all lit tests (generally) pass, but there are some intermittent 
failures (Access Violation) on the following tests:

> ExecutionEngine/MCJIT/test-global-init-nonzero-sm-pic.ll
> ExecutionEngine/MCJIT/test-ptr-reloc-sm-pic.ll
> ExecutionEngine/MCJIT/multi-module-sm-pic-a.ll

This happens with VS2019 or clang-cl as the toolchain. I've run multiple times 
without scudo and don't see these. Unfortunately I haven't been able to get 
this to fail in a debugger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c
  llvm/CMakeLists.txt
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/lib/Support/CMakeLists.txt

Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -58,7 +58,7 @@
   string(REGEX REPLACE "(/|)$" "" LLVM_INTEGRATED_CRT_ALLOC "${LLVM_INTEGRATED_CRT_ALLOC}")
 
   if(NOT EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}")
-message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc and mimalloc are supported.")
+message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc, mimalloc and scudo are supported.")
   endif()
 
   if(LLVM_INTEGRATED_CRT_ALLOC MATCHES "rpmalloc$")
@@ -73,6 +73,8 @@
 	  message(FATAL_ERROR "Cannot find the mimalloc static library. To build it, first apply the patch from https://github.com/microsoft/mimalloc/issues/268 then build the Release x64 target through ${LLVM_INTEGRATED_CRT_ALLOC}\\ide\\vs2019\\mimalloc.sln")
 endif()
 set(system_libs ${system_libs} "${MIMALLOC_LIB}" "-INCLUDE:malloc")
+  elseif(EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}/clang_rt.scudo-x86_64.lib" AND EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}/clang_rt.scudo_cxx-x86_64.lib")
+  set(system_libs ${system_libs} "${LLVM_INTEGRATED_CRT_ALLOC}/clang_rt.scudo-x86_64.lib" "${LLVM_INTEGRATED_CRT_ALLOC}/clang_rt.scudo_cxx-x86_64.lib" "-INCLUDE:malloc")
   endif()
 endif()
 
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -790,6 +790,9 @@
 elseif (LLVM_USE_SANITIZER STREQUAL "Leaks")
   append_common_sanitizer_flags()
   append("-fsanitize=leak" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "Unsupported value of 

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 301331.
russell.gallop added a comment.
Herald added a subscriber: dexonsmith.

Apologies for the delay, I've had a few other things on.

I've worked through the tests. Several I've marked as unsupported on windows as 
they use things unixy things like nm, LD_PRELOAD or fork(). I added a C++ 
threads version of threads.c to test that on Windows (cxx_threads.cpp).

There are 5 remaining failures:

  Failed Tests (5):
Scudo-x86_64 :: aligned-new.cpp
Scudo-x86_64 :: mismatch.cpp
Scudo-x86_64 :: options.cpp
Scudo-x86_64 :: sized-delete.cpp
Scudo-x86_64 :: sizes.cpp
  
  Testing Time: 11.61s
Unsupported:  8
Passed : 13
Failed :  5

These are all down to new/delete not being replaced by the scudo versions, so 
scudoAllocate and scudoDeallocate don't get the appropriate size and Type 
parameters. The cxx library should be linked by SanitizerArgs::addArgs so I'm 
not sure why this is. @cryptoad, it looks like you did some work in 
scudo_new_delete.cpp, did you have this working?

Does new/delete need to work for this, or should I mark these tests as 
unsupported on Windows for the purposes of a step towards getting the 
standalone scudo working?

Stage 2 builds with 
-DLLVM_INTEGRATED_CRT_ALLOC=/stage1/lib/clang/12.0.0/lib/windows/clang_rt.scudo-x86_64.lib
 -DLLVM_USE_CRT_RELEASE=MT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c
  llvm/CMakeLists.txt
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/lib/Support/CMakeLists.txt

Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -58,7 +58,7 @@
   string(REGEX REPLACE "(/|)$" "" LLVM_INTEGRATED_CRT_ALLOC "${LLVM_INTEGRATED_CRT_ALLOC}")
 
   if(NOT EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}")
-message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc and mimalloc are supported.")
+message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc, mimalloc and scudo are supported.")
   endif()
 
   if(LLVM_INTEGRATED_CRT_ALLOC MATCHES "rpmalloc$")
@@ -73,6 +73,8 @@
 	  message(FATAL_ERROR "Cannot find the mimalloc static library. To build it, first apply the patch from https://github.com/microsoft/mimalloc/issues/268 then build the Release x64 target through ${LLVM_INTEGRATED_CRT_ALLOC}\\ide\\vs2019\\mimalloc.sln")
 endif()
 set(system_libs ${system_libs} "${MIMALLOC_LIB}" "-INCLUDE:malloc")
+  elseif(LLVM_INTEGRATED_CRT_ALLOC MATCHES "scudo")
+  set(system_libs ${system_libs} "${LLVM_INTEGRATED_CRT_ALLOC}" "-INCLUDE:malloc")
   endif()
 endif()
 
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -790,6 +790,9 @@
 elseif (LLVM_USE_SANITIZER STREQUAL "Leaks")
   append_common_sanitizer_flags()
   append("-fsanitize=leak" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "Unsupported value of LLVM_USE_SANITIZER: ${LLVM_USE_SANITIZER}")
 endif()
@@ -797,6 +800,9 @@
 if (LLVM_USE_SANITIZER STREQUAL "Address")
   append_common_sanitizer_flags()
   append("-fsanitize=address" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS 

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2288660 , @aganea wrote:

> I'm also in favor, I think this is good direction ahead. It'd be nice if 
> following issues were fixed -- in subsequent patches if you wish:
>
> - Stage1 `ninja check-scudo` fails many tests for me, see F13037612: 
> errors.txt .

I intend to fix these (or mark an unsupported) for this patch.

> - Stage2 `ninja check-llvm` fails after a while on high-core machines (4TB 
> issue mentionned in comments above). Lowering `AllocatorSize` to 256GB would 
> fix the issue on the short term.

I'll add that.

> - Fix & test the "exclusive" mode (or just skip to scudo-standalone if it's 
> too complicated).

I don't intend to do that for this patch, as shared mode works. I will 
re-evaluate after the above.

I also intend to remove the -fsanitize related changes. I believe that they're 
not required on Windows.

Thanks
Russ




Comment at: compiler-rt/lib/scudo/scudo_platform.h:67
 #if SANITIZER_CAN_USE_ALLOCATOR64
 # if defined(__aarch64__) && SANITIZER_ANDROID
 const uptr AllocatorSize = 0x40ULL;  // 256G.

aganea wrote:
> `&& SANITIZER_WINDOWS` ?
I think that should be:


```
# if (defined(__aarch64__) && SANITIZER_ANDROID) || SANITIZER_WINDOWS
```

I'll either do that or add a new elif.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> I'm going to concentrate on the standalone port as I think that's the way 
> forward. If I get that working then can work through the other issues.

I'm considering a slight change of plan. @cryptoad, in the name of incremental 
development, would you be happy for me to put the scudo sanitizer version up 
(with working tests), or is this deprecated now? It may not be the best for 
performance long term but it's a smaller step than the standalone port and 
seems to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2274383 , @cryptoad wrote:

> In D86694#2273815 , @aganea wrote:
>
>> If 4.4 TB of virtual pages are mapped in each process (this happens on 
>> startup), then we quickly exaust the 48-bit (256 TB) addressable space with 
>> 72+ programs running (on a 36-core). Any idea where this 4.4 TB mapping 
>> comes from?
>
> The size of the Primary is defined in 
> https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/scudo/scudo_platform.h#L75
> Scudo reserves that size but doesn't commit it, then it incrementally commits 
> when memory is needed within the reserved region.

I guess using scudo as a general purpose allocator that could set a limit on 
the number of cores that can be used at once as @aganea found. Would there be 
any problem with making this very small (e.g. a couple of GB)?

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2273815 , @aganea wrote:

> @russell.gallop I see a lots of failing tests when running `ninja check-all` 
> on a Scudo-enabled build (stage 2). Do you see the same thing on your end?

Not a lot but a few, including DynamicLibrary.Shutdown:

  Failed Tests (4):
LLVM-Unit :: 
Support/DynamicLibrary/./DynamicLibraryTests.exe/DynamicLibrary.Shutdown
LLVM :: ExecutionEngine/MCJIT/multi-module-sm-pic-a.ll
LLVM :: ExecutionEngine/OrcMCJIT/multi-module-sm-pic-a.ll
LLVM :: ExecutionEngine/OrcMCJIT/stubs-sm-pic.ll

And the MCJIT ones seem intermittent (different ones fail each time).

> If 4.4 TB of virtual pages are mapped in each process (this happens on 
> startup), then we quickly exhaust the 48-bit (256 TB) addressable space with 
> 72+ programs running (on a 36-core). Any idea where this 4.4 TB mapping comes 
> from?

I guess that's just a problem with a larger number of cores then.

I'm going to concentrate on the standalone port as I think that's the way 
forward. If I get that working then can work through the other issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2270433 , @aganea wrote:

> Thanks for working on this @russell.gallop!
>
> I've reproduced your tests, please see below. The only difference is that 
> I've used a ThinLTO build for stage2:

Thanks. It's good to know they're reproducible, and that it scales better than 
the default allocator.

> (a hardware CRC or AES implemention will certainly help for Scudo)

Actually this wasn't too hard to try out. I added "-msse4.2" to CMAKE_C_FLAGS 
and CMAKE_CXX_FLAGS (as suggested in scudo_allocator.cpp). This helps, but 
scudo is still a bit behind the default allocator for me on 6 cores:

  > set 
SCUDO_OPTIONS=allocator_release_to_os_interval_ms=-1:QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0:DeleteSizeMismatch=0:DeallocationTypeMismatch=0
  > hyperfine.exe -m 5 -w 1 "cd stage3\repro && 
f:\git\llvm-project\stage2\bin\lld-link @response.txt" "cd stage3_scudo\repro 
&& f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt"
  Benchmark #1: cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link 
@response.txt
Time (mean ± σ): 269.922 s ±  2.706 s[User: 0.0 ms, System: 18.6 ms]
Range (min … max):   265.969 s … 272.188 s5 runs
  
  Benchmark #2: cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt
Time (mean ± σ): 285.522 s ±  0.440 s[User: 12.3 ms, System: 9.9 ms]
Range (min … max):   284.907 s … 286.056 s5 runs
  
  Summary
'cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link @response.txt' 
ran
  1.06 ± 0.01 times faster than 'cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 291511.
russell.gallop added a comment.

Re-upload patch with G LLVM Github Monorepo set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/sanitizer_common/CMakeLists.txt
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c
  llvm/CMakeLists.txt
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/lib/Support/CMakeLists.txt

Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -73,7 +73,7 @@
   string(REGEX REPLACE "(/|)$" "" LLVM_INTEGRATED_CRT_ALLOC "${LLVM_INTEGRATED_CRT_ALLOC}")
 
   if(NOT EXISTS "${LLVM_INTEGRATED_CRT_ALLOC}")
-message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc and mimalloc are supported.")
+message(FATAL_ERROR "Cannot find the path to `git clone` for the CRT allocator! (${LLVM_INTEGRATED_CRT_ALLOC}). Currently, rpmalloc, snmalloc, mimalloc and scudo are supported.")
   endif()
 
   if(LLVM_INTEGRATED_CRT_ALLOC MATCHES "rpmalloc$")
@@ -89,6 +89,8 @@
 	  message(FATAL_ERROR "Cannot find the mimalloc static library. To build it, first apply the patch from https://github.com/microsoft/mimalloc/issues/268 then build the Release x64 target through ${LLVM_INTEGRATED_CRT_ALLOC}\\ide\\vs2019\\mimalloc.sln")
 endif()
 set(system_libs ${system_libs} "${MIMALLOC_LIB}" "-INCLUDE:malloc")
+  elseif(LLVM_INTEGRATED_CRT_ALLOC MATCHES "scudo")
+  set(system_libs ${system_libs} "${LLVM_INTEGRATED_CRT_ALLOC}" "-INCLUDE:malloc")
   endif()
 endif()
 
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -789,6 +789,9 @@
 elseif (LLVM_USE_SANITIZER STREQUAL "Leaks")
   append_common_sanitizer_flags()
   append("-fsanitize=leak" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "Unsupported value of LLVM_USE_SANITIZER: ${LLVM_USE_SANITIZER}")
 endif()
@@ -796,6 +799,9 @@
 if (LLVM_USE_SANITIZER STREQUAL "Address")
   append_common_sanitizer_flags()
   append("-fsanitize=address" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "This sanitizer not yet supported in the MSVC environment: ${LLVM_USE_SANITIZER}")
 endif()
Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -572,8 +572,8 @@
   if(NOT WIN32)
 message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC is only supported on Windows.")
   endif()
-  if(LLVM_USE_SANITIZER)
-message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC cannot be used along with LLVM_USE_SANITIZER!")
+  if(LLVM_USE_SANITIZER AND NOT LLVM_USE_SANITIZER STREQUAL "Scudo")
+message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC cannot be used along with LLVM_USE_SANITIZER (apart from Scudo)!")
   endif()
   if(CMAKE_BUILD_TYPE AND uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
 message(FATAL_ERROR "The Debug target isn't supported along with LLVM_INTEGRATED_CRT_ALLOC!")
Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// 

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2242559 , @cryptoad wrote:

> In D86694#2242150 , @russell.gallop 
> wrote:
>
>> In D86694#2242140 , @cryptoad wrote:
>>
>>> That's awesome! Is it meant to eventually be committed or only be used for 
>>> comparison purposes?
>>
>> I'd like it to be committed, but can't claim I know the code from 
>> https://reviews.llvm.org/D42519 well enough. The good news is that I can 
>> build LLVM on Windows with this. Is there a good sanity check that it is 
>> actually using Scudo rather than silently using the standard alloc?
>
> Nothing except the tests. Compiling a sizeable application with Scudo as well.

Compiling clang and lld with Scudo and ThinLTO linking clang seems to work.

> The other point that is worth mentioning is that we moved all dev efforts to 
> the "standalone" version of Scudo (eg: the one not depending on 
> sanitizer_common in the standalone/ subdirectory).
> There is enough differences that there could be some significant 
> performance/mem footprint changes between the 2.

Thanks. I'm looking at porting the standalone variant, drawing on D42519 
 and Windows support from sanitizer_common. 
Does that sound like a reasonable approach?

> Also depending on what you want to compare, disabling the Quarantine and 
> other optional security features will make things faster and use less memory.

Would these be good flags to use:

  set 
SCUDO_OPTIONS=allocator_release_to_os_interval_ms=-1:QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0:DeleteSizeMismatch=0:DeallocationTypeMismatch=0


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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 291235.
russell.gallop edited the summary of this revision.
russell.gallop added a comment.
Herald added subscribers: phosek, hiraditya.

Fixup scudo (sanitizer based) to work on Windows.

This makes use of the CRT alloc hooks from D71786 
.

To build with scudo on Windows use: 
-DLLVM_INTEGRATED_CRT_ALLOC=/stage1/lib/clang/12.0.0/lib/windows/clang_rt.scudo-x86_64.lib
 -DLLVM_USE_CRT_RELEASE=MT 
-DLLVM_USE_SANITIZER=Scudo is supported in this patch, but isn't required. 
@cryptoad on Linux does this do anything other than add libraries when using 
clang to drive the linker?

Limitations:
-Note that this is not using hardware CRC32.
-This just hooks in the C scudo library, not the cxx library

I evaluated this on a 3 stage LLVM build on Windows 10 2004 (in vs2019 16.7.3 
environment) on a 6-core i7-8700k.

- stage1 builds the scudo sanitizer on Windows: requires 
-DLLVM_ENABLE_PROJECTS=clang;lld;compiler-rt
- stage2: built with -DCMAKE_C_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_LINKER="/bin/lld-link.exe" -DLLVM_USE_CRT_RELEASE=MT 
-DCMAKE_BUILD_TYPE=Release
- stage2_scudo: as stage2 plus 
-DLLVM_INTEGRATED_CRT_ALLOC=/lib/clang/12.0.0/lib/windows/clang_rt.scudo-x86_64.lib

Then evaluated linking clang with ThinLTO:

- stage3: -DCMAKE_C_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_LINKER="/bin/lld-link.exe" -DLLVM_USE_CRT_RELEASE=MT 
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LTO=Thin
- stage3_scudo: -DCMAKE_C_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="/bin/clang-cl.exe" 
-DCMAKE_LINKER="/bin/lld-link.exe" -DLLVM_USE_CRT_RELEASE=MT 
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LTO=Thin

I set /threads:12 and removed /lldltocache.

Without SCUDO_OPTIONS scudo seems to be about 25% slower.

  >"hyperfine.exe" -m 3 -w 1 "cd stage3\repro && 
f:\git\llvm-project\stage2\bin\lld-link @response.txt" "cd stage3_scudo\repro 
&& f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt"
  Benchmark #1: cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link 
@response.txt
Time (mean ± σ): 268.209 s ±  4.966 s[User: 18.1 ms, System: 6.6 ms]
Range (min … max):   263.223 s … 273.155 s3 runs
  
  Benchmark #2: cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt
Time (mean ± σ): 334.312 s ±  4.002 s[User: 2.4 ms, System: 14.6 ms]
Range (min … max):   329.889 s … 337.683 s3 runs
  
  Summary
'cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link @response.txt' 
ran
  1.25 ± 0.03 times faster than 'cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt'

I set scudo options to disable quarantine and mismatch checking and it seems to 
be about 8% slower.

  >set 
SCUDO_OPTIONS=allocator_release_to_os_interval_ms=-1:QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0:DeleteSizeMismatch=0:DeallocationTypeMismatch=0
  >"hyperfine.exe" -m 3 -w 1 "cd stage3\repro && 
f:\git\llvm-project\stage2\bin\lld-link @response.txt" "cd stage3_scudo\repro 
&& f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt"
  Benchmark #1: cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link 
@response.txt
Time (mean ± σ): 273.772 s ±  3.624 s[User: 1.3 ms, System: 8.6 ms]
Range (min … max):   269.806 s … 276.909 s3 runs
  
  Benchmark #2: cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt
Time (mean ± σ): 296.593 s ±  2.362 s[User: 1.3 ms, System: 13.9 ms]
Range (min … max):   293.917 s … 298.391 s3 runs
  
  Summary
'cd stage3\repro && f:\git\llvm-project\stage2\bin\lld-link @response.txt' 
ran
  1.08 ± 0.02 times faster than 'cd stage3_scudo\repro && 
f:\git\llvm-project\stage2_scudo\bin\lld-link @response.txt'

It's worth noting that the run without scudo was not using all CPU so still 
hitting some locking issues but was still over 90%. With both scudo runs it was 
pegged at 100% CPU until near the end of the link.  From this it appears that 
the locking behaviour is better than the default allocator and would win out on 
a wide enough processor. The loss in straight line performance is potentially 
due to CRC calculations (not using hardware).

@cryptoad are those the best scudo flags to evaluate for performance?

I'm looking at porting scudo standalone to Windows.


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

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2242150 , @russell.gallop 
wrote:

> You marked D42519  as WIP, can you remember 
> what was still TBD?

Maybe more tests could be enabled...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D86694#2242140 , @cryptoad wrote:

> That's awesome! Is it meant to eventually be committed or only be used for 
> comparison purposes?

I'd like it to be committed, but can't claim I know the code from 
https://reviews.llvm.org/D42519 well enough. The good news is that I can build 
LLVM on Windows with this. Is there a good sanity check that it is actually 
using Scudo rather than silently using the standard alloc?

You marked D42519  as WIP, can you remember 
what was still TBD?

Also, it might make sense to separate out the "use" of sanitize=Scudo in LLVM, 
from providing Windows support. I put them together here for evaluation 
purposes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86694

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

[PATCH] D86694: [scudo] Allow -fsanitize=scudo on Linux and Windows (WIP, don't land as is)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: cryptoad, aganea, hans.
Herald added subscribers: llvm-commits, Sanitizers, cfe-commits, mgorny.
Herald added projects: clang, Sanitizers, LLVM.
russell.gallop requested review of this revision.

This is basically https://reviews.llvm.org/D42519 updated for monorepo with a 
few changes to allow -fsanitize=scudo in clang-cl and permit 
LLVM_USE_SANITIZER=Scudo.

This is to allow evaluation of Scudo as a replacement memory allocator on 
Windows for comparison with https://reviews.llvm.org/D71786


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86694

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c
  llvm/cmake/modules/HandleLLVMOptions.cmake

Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -775,6 +775,9 @@
 elseif (LLVM_USE_SANITIZER STREQUAL "Leaks")
   append_common_sanitizer_flags()
   append("-fsanitize=leak" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "Unsupported value of LLVM_USE_SANITIZER: ${LLVM_USE_SANITIZER}")
 endif()
@@ -782,6 +785,9 @@
 if (LLVM_USE_SANITIZER STREQUAL "Address")
   append_common_sanitizer_flags()
   append("-fsanitize=address" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+elseif (LLVM_USE_SANITIZER STREQUAL "Scudo")
+  append_common_sanitizer_flags()
+  append("-fsanitize=scudo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 else()
   message(FATAL_ERROR "This sanitizer not yet supported in the MSVC environment: ${LLVM_USE_SANITIZER}")
 endif()
Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, win32
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: win32
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: win32
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
 #include 
 #include 
-#include 
+#include 
+#ifdef _WIN32
+# include 
+#else
+# include 
+# include 
+#endif
 
+#ifdef _WIN32
+DWORD getpagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo)
+{
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long 

[PATCH] D79147: Switch to using -debug-info-kind=constructor as default (from =limited)

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Hi,

It looks like is causing one of the debuginfo-tests: 
llgdb-tests/nrvo-string.cpp to fail, run on Linux. Failure as below. I don't 
think the debuginfo-tests are run on any bot (but probably should be!). I 
bisected the failure back to this change.

Please could you take a look?

Thanks
Russ

  FAIL: debuginfo-tests :: llgdb-tests/nrvo-string.cpp (1 of 1)
   TEST 'debuginfo-tests :: llgdb-tests/nrvo-string.cpp' 
FAILED 
  Script:
  --
  : 'RUN: at line 4';   /home//git/llvm-project/stage1/bin/clang 
--driver-mode=g++ -O0 -fno-exceptions --target=x86_64-unknown-linux-gnu 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp -o 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.tmp.out
 -g
  : 'RUN: at line 5';   
/home//git/llvm-project/debuginfo-tests/llgdb-tests/test_debuginfo.pl 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.tmp.out
  : 'RUN: at line 6';   /home//git/llvm-project/stage1/bin/clang 
--driver-mode=g++ -O1 -fno-exceptions --target=x86_64-unknown-linux-gnu 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp -o 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.tmp.out
 -g
  : 'RUN: at line 7';   
/home//git/llvm-project/debuginfo-tests/llgdb-tests/test_debuginfo.pl 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.tmp.out
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  Debugger output was:
  Breakpoint 1 at 0x4004f8: file 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp, line 
23.
  Breakpoint 2 at 0x400563: file 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp, line 
39.
  
  Breakpoint 1, get_string () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:23
  23stop();
  $1 = 3
  
  Breakpoint 2, get_string2 () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:39
  39stop();
  
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.debugger.script:6:
 Error in sourced command file:
  There is no member named i.
  
  --
  Command Output (stderr):
  --
  
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:52:11:
 error: CHECK: expected string not found in input
  // CHECK: = 5
^
  
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.gdb.output:8:1:
 note: scanning from here
  Breakpoint 2, get_string2 () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:39
  ^
  
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.gdb.output:8:10:
 note: possible intended match here
  Breakpoint 2, get_string2 () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:39
   ^
  
  Input file: 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.gdb.output
  Check file: 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp
  
  -dump-input=help explains the following input dump.
  
  Full input was:
  <<
  1: Breakpoint 1 at 0x4004f8: file 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp, line 
23.
  2: Breakpoint 2 at 0x400563: file 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp, line 
39.
  3:
  4: Breakpoint 1, get_string () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:23
  5: 23 stop();
  6: $1 = 3
  7:
  8: Breakpoint 2, get_string2 () at 
/home//git/llvm-project/debuginfo-tests/llgdb-tests/nrvo-string.cpp:39
  check:52'0 
X
 error: no match found
  check:52'1  ? 
   possible intended match
  9: 39 stop();
  check:52'0 ~~
 10: 
/home//git/llvm-project/stage1/projects/debuginfo-tests/llgdb-tests/Output/nrvo-string.cpp.debugger.script:6:
 Error in sourced command file:
  check:52'0 
~~~
 11: There is no member named i.
  check:52'0 ~~~
  >>
  
  --


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79147

___
cfe-commits 

[PATCH] D78454: [clangd] Highlight related control flow.

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Hi Sam,

It looks like this is causing a failure on the Windows PS4 buildbot: 
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/32606

Please could you take a look? PS4 target disables RTTI, hence exceptions, by 
default so it is probably related to that.

Thanks
Russ


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78454



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

[PATCH] D78030: [TimeProfiler] Emit clock synchronization point

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop accepted this revision.
russell.gallop added a comment.

LGTM, with a few small comments.

For the record, I wondered whether the time profiler could emit all ts as 
absolute, so I tried it out. This has two problems. 1). The "Total" numbers 
also need adjusting to be at the beginning of the trace time rather than zero, 
which feels a little odd, and it hard to see how long things are on the time 
scale. 2). All of the times end up being very large numbers so this bloats 
traces considerably.




Comment at: clang/test/Driver/check-time-trace-sections.py:23
+
+# Make sure that the 'beginningOfTime' is not earlier than 10 seconds ago.
+if seconds_since_epoch - beginning_of_time > 10:

Could also check that beginningOfTime isn't after seconds_since_epoch.



Comment at: llvm/lib/Support/TimeProfiler.cpp:78
   TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
-  : StartTime(steady_clock::now()), ProcName(ProcName),
-Pid(sys::Process::getProcessId()), Tid(llvm::get_threadid()),
-TimeTraceGranularity(TimeTraceGranularity) {
+  : BeginningOfTime(system_clock::now()), StartTime(steady_clock::now()),
+ProcName(ProcName), Pid(sys::Process::getProcessId()),

Note that this will record BeginningOfTime for each TimeTraceProfiler started 
on a thread, but that won't be used. This shouldn't cause any harm  and I don't 
think that is very frequent so I think that this is okay.



Comment at: llvm/lib/Support/TimeProfiler.cpp:238
+
+// Emit the absolute time of the moment when this TimeProfiler started
+// measurements. This can be used to combine the profiling data from

Nit. This is a bit wordy. How about "Emit the absolute time when this 
TimeProfiler started."?


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

https://reviews.llvm.org/D78030



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

[PATCH] D78027: [TimeProfiler] Emit real process ID and thread names

[Russell Gallop via Phabricator via cfe-commits]

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

Would be nice if the test could check that the pid was being set to something 
but not sure how you could do that (beyond checking that it's a number).

Apart from that, LGTM.


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

https://reviews.llvm.org/D78027



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

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop abandoned this revision.
russell.gallop added a comment.

This was submitted as the sequence of patches:
https://reviews.llvm.org/D70904 - Tidying up in TimeProfiler.cpp
https://reviews.llvm.org/D70950 - Add ProcName to TimeTraceProfiler
https://reviews.llvm.org/D71059 - [LLD][ELF] Add time-trace to ELF LLD (1/2) 
(multi-thread support in TimeProfiler)
https://reviews.llvm.org/D71060 - [LLD][ELF] Add time-trace to ELF LLD (2/2) 
(Add time trace to LLD)

Closing this review.


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] D70950: Add ProcName to TimeTraceProfiler

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D70950#1850796 , @rnk wrote:

> This broke ClangBuildAnalyzer on Windows because it has a very naive check 
> for "clang":
>  https://github.com/aras-p/ClangBuildAnalyzer/blob/master/src/main.cpp#L148
>  I was wondering why this wasn't working on Windows anymore. =/


I think this should be fixed by this recent change to ClangBuildAnalyzer: 
https://github.com/aras-p/ClangBuildAnalyzer/commit/7ccac789ba5846c04392755a71856b18b7f0c2c1

That should cope with both clang-11 and clang.exe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70950



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

[PATCH] D73162: [test] Avoid loop-unroll.c test getting confused by fadd in git revision

[Russell Gallop via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4662f6e1c778: [test] Avoid loop-unroll.c test getting 
confused by fadd in git revision (authored by russell.gallop).

Changed prior to commit:
  https://reviews.llvm.org/D73162?vs=239808=239812#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73162

Files:
  clang/test/CodeGen/loop-unroll.c


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -37,6 +37,8 @@
 // CHECK-DISABLE-UNROLL-NOT: fmul
 // CHECK-DISABLE-UNROLL-NOT: fadd
 // CHECK-DISABLE-UNROLL-NOT: store
+// Limit scope of checking so this does not match "fadd" within git version 
string
+// CHECK-DISABLE-UNROLL: !0 =
 
 int printf(const char * restrict format, ...);
 


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -37,6 +37,8 @@
 // CHECK-DISABLE-UNROLL-NOT: fmul
 // CHECK-DISABLE-UNROLL-NOT: fadd
 // CHECK-DISABLE-UNROLL-NOT: store
+// Limit scope of checking so this does not match "fadd" within git version string
+// CHECK-DISABLE-UNROLL: !0 =
 
 int printf(const char * restrict format, ...);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73162: [test] Avoid loop-unroll.c test getting confused by fadd in git revision

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D73162#1834379 , @asbirlea wrote:

> Oh, wow! Might I ask you add the same for fmul? We may get a revision like 
> that next time :).
>  Thank you!


fmul shouldn't have the same problem as git hashes are only hex characters.

In D73162#1834392 , @fhahn wrote:

> I think  we could also just add a single match for something after the 
> generated functions, to limit the scope of CHECK-NOT, e.g add something like 
> `CHECK : !0 =`


Thanks, that's cleaner.

Note that the diff here is a little confused. There are now no changes to the 
previous check lines.


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

https://reviews.llvm.org/D73162



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

[PATCH] D73162: [test] Avoid loop-unroll.c test getting confused by fadd in git revision

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 239808.
russell.gallop added a comment.

Update to check for start of metadata.


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

https://reviews.llvm.org/D73162

Files:
  clang/test/CodeGen/loop-unroll.c


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -37,9 +37,11 @@
 // CHECK-DISABLE-UNROLL-NOT: fmul
 // CHECK-DISABLE-UNROLL-NOT: fadd
 // CHECK-DISABLE-UNROLL-NOT: store
+// Limit scope of checking so this does not match "fadd" within git version 
string
+// CHECK-DISABLE-UNROLL: !0 =

 int printf(const char * restrict format, ...);

 void for_test() {
   double A[1000], B[1000];
   int L = 500;


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -37,9 +37,11 @@
 // CHECK-DISABLE-UNROLL-NOT: fmul
 // CHECK-DISABLE-UNROLL-NOT: fadd
 // CHECK-DISABLE-UNROLL-NOT: store
+// Limit scope of checking so this does not match "fadd" within git version string
+// CHECK-DISABLE-UNROLL: !0 =

 int printf(const char * restrict format, ...);

 void for_test() {
   double A[1000], B[1000];
   int L = 500;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73162: [test] Avoid loop-unroll.c test getting confused by fadd in git revision

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added a reviewer: asbirlea.
Herald added a subscriber: zzheng.
Herald added a project: clang.

Saw this test failing as it was matching fadd in a (local) git revision:

  F:\git\llvm-project\clang\test\CodeGen\loop-unroll.c:38:30: error: 
CHECK-DISABLE-UNROLL-NOT: excluded string found in input
  // CHECK-DISABLE-UNROLL-NOT: fadd
   ^
  :69:92: note: found here
  !1 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project.git 
a3eebdb6c1376e528a6fadd5eb33bb6eb986a126)"}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73162

Files:
  clang/test/CodeGen/loop-unroll.c


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -12,15 +12,15 @@
 // CHECK-ENABLE-UNROLL: br i1 %[[EXITCOND:[a-z0-9_\.]+]], label 
%[[FORBODY5:[a-z0-9_\.]+]], label %[[FORBODY]]
 // CHECK-ENABLE-UNROLL: [[FORBODY5]]:
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store

 // CHECK-DISABLE-UNROLL-LABEL: @for_test()
 // CHECK-DISABLE-UNROLL: br label %[[FORBODY:[a-z0-9_\.]+]]
 // CHECK-DISABLE-UNROLL: [[FORBODY]]:
@@ -29,17 +29,17 @@
 // CHECK-DISABLE-UNROLL: br i1 %[[EXITCOND:[a-z0-9_\.]+]], label 
%[[FORBODY5:[a-z0-9_\.]+]], label %[[FORBODY]]
 // CHECK-DISABLE-UNROLL: [[FORBODY5]]:
 // CHECK-DISABLE-UNROLL: fmul
-// CHECK-DISABLE-UNROLL: fadd
+// CHECK-DISABLE-UNROLL: fadd double
 // CHECK-DISABLE-UNROLL: store
 // CHECK-DISABLE-UNROLL: fmul
-// CHECK-DISABLE-UNROLL: fadd
+// CHECK-DISABLE-UNROLL: fadd double
 // CHECK-DISABLE-UNROLL: store
 // CHECK-DISABLE-UNROLL-NOT: fmul
-// CHECK-DISABLE-UNROLL-NOT: fadd
+// CHECK-DISABLE-UNROLL-NOT: fadd double
 // CHECK-DISABLE-UNROLL-NOT: store

 int printf(const char * restrict format, ...);

 void for_test() {
   double A[1000], B[1000];
   int L = 500;


Index: clang/test/CodeGen/loop-unroll.c
===
--- clang/test/CodeGen/loop-unroll.c
+++ clang/test/CodeGen/loop-unroll.c
@@ -12,15 +12,15 @@
 // CHECK-ENABLE-UNROLL: br i1 %[[EXITCOND:[a-z0-9_\.]+]], label %[[FORBODY5:[a-z0-9_\.]+]], label %[[FORBODY]]
 // CHECK-ENABLE-UNROLL: [[FORBODY5]]:
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store
 // CHECK-ENABLE-UNROLL: fmul
-// CHECK-ENABLE-UNROLL: fadd
+// CHECK-ENABLE-UNROLL: fadd double
 // CHECK-ENABLE-UNROLL: store

 // CHECK-DISABLE-UNROLL-LABEL: @for_test()
 // CHECK-DISABLE-UNROLL: br label %[[FORBODY:[a-z0-9_\.]+]]
 // CHECK-DISABLE-UNROLL: [[FORBODY]]:
@@ -29,17 +29,17 @@
 // CHECK-DISABLE-UNROLL: br i1 %[[EXITCOND:[a-z0-9_\.]+]], label %[[FORBODY5:[a-z0-9_\.]+]], label %[[FORBODY]]
 // CHECK-DISABLE-UNROLL: [[FORBODY5]]:
 // CHECK-DISABLE-UNROLL: fmul
-// CHECK-DISABLE-UNROLL: fadd
+// CHECK-DISABLE-UNROLL: fadd double
 // CHECK-DISABLE-UNROLL: store
 // CHECK-DISABLE-UNROLL: fmul
-// CHECK-DISABLE-UNROLL: fadd
+// CHECK-DISABLE-UNROLL: fadd double
 // CHECK-DISABLE-UNROLL: store
 // CHECK-DISABLE-UNROLL-NOT: fmul
-// CHECK-DISABLE-UNROLL-NOT: fadd
+// CHECK-DISABLE-UNROLL-NOT: fadd double
 // CHECK-DISABLE-UNROLL-NOT: store

 int printf(const char * restrict format, ...);

 void for_test() {
   double A[1000], B[1000];
   int L = 500;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73123: [CMake] Fixes for including LLVM into another CMake project

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: chandlerc, dcoughlin, jroelofs, mgorny, pcc, 
tstellar, beanz, dsanders.
Herald added subscribers: cfe-commits, Charusso.
Herald added projects: clang, LLVM.

This fixes a couple of paths to allow add_subdirectory(llvm) from a higher 
level CMake project. Otherwise should be NFC.

Tested by creating a top level CMakeLists.txt in llvm-project:

  cmake_minimum_required(VERSION 3.4.3)
  
  project(my-super-project)
  
  add_subdirectory(llvm)

And pointing cmake at that instead of llvm/CMakeLists.txt.

In this arrangement, most things are built inside an llvm directory inside the 
build directory. Some tools and files aren't which causes some test fails. This 
makes things consistent by changing CMAKE_BINARY_DIR to LLVM_BINARY_DIR and 
CMAKE_SOURCE_DIR to LLVM_SOURCE_DIR.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73123

Files:
  clang/tools/scan-build/CMakeLists.txt
  clang/tools/scan-view/CMakeLists.txt
  clang/utils/hmaptool/CMakeLists.txt
  llvm/tools/llvm-go/CMakeLists.txt
  llvm/tools/llvm-shlib/CMakeLists.txt
  llvm/unittests/TableGen/CMakeLists.txt

Index: llvm/unittests/TableGen/CMakeLists.txt
===
--- llvm/unittests/TableGen/CMakeLists.txt
+++ llvm/unittests/TableGen/CMakeLists.txt
@@ -13,5 +13,5 @@
   CodeExpanderTest.cpp
   AutomataTest.cpp
   )
-include_directories(${CMAKE_SOURCE_DIR}/utils/TableGen)
+include_directories(${LLVM_SOURCE_DIR}/utils/TableGen)
 target_link_libraries(TableGenTests PRIVATE LLVMTableGenGlobalISel LLVMTableGen)
Index: llvm/tools/llvm-shlib/CMakeLists.txt
===
--- llvm/tools/llvm-shlib/CMakeLists.txt
+++ llvm/tools/llvm-shlib/CMakeLists.txt
@@ -129,7 +129,7 @@
 
   # Get the full name to the libs so the python script understands them.
   foreach(lib ${LIB_NAMES})
-list(APPEND FULL_LIB_NAMES ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${lib}.lib)
+list(APPEND FULL_LIB_NAMES ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${lib}.lib)
   endforeach()
 
   # Need to seperate lib names with newlines.
Index: llvm/tools/llvm-go/CMakeLists.txt
===
--- llvm/tools/llvm-go/CMakeLists.txt
+++ llvm/tools/llvm-go/CMakeLists.txt
@@ -1,5 +1,5 @@
 if(LLVM_BINDINGS MATCHES "go")
-  set(binpath ${CMAKE_BINARY_DIR}/bin/llvm-go${CMAKE_EXECUTABLE_SUFFIX})
+  set(binpath ${LLVM_BINARY_DIR}/bin/llvm-go${CMAKE_EXECUTABLE_SUFFIX})
   add_custom_command(OUTPUT ${binpath}
 COMMAND ${GO_EXECUTABLE} build -o ${binpath} llvm-go.go
 DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/llvm-go.go
Index: clang/utils/hmaptool/CMakeLists.txt
===
--- clang/utils/hmaptool/CMakeLists.txt
+++ clang/utils/hmaptool/CMakeLists.txt
@@ -1,14 +1,14 @@
 set(CLANG_HMAPTOOL hmaptool)
 
-add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL}
+add_custom_command(OUTPUT ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL}
COMMAND ${CMAKE_COMMAND} -E make_directory
- ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin
+ ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin
COMMAND ${CMAKE_COMMAND} -E copy
  ${CMAKE_CURRENT_SOURCE_DIR}/${CLANG_HMAPTOOL}
- ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/
+ ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${CLANG_HMAPTOOL})
 
-list(APPEND Depends ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL})
+list(APPEND Depends ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL})
 install(PROGRAMS ${CLANG_HMAPTOOL}
 DESTINATION bin
 COMPONENT hmaptool)
Index: clang/tools/scan-view/CMakeLists.txt
===
--- clang/tools/scan-view/CMakeLists.txt
+++ clang/tools/scan-view/CMakeLists.txt
@@ -13,28 +13,28 @@
 
 if(CLANG_INSTALL_SCANVIEW)
   foreach(BinFile ${BinFiles})
-add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/bin/${BinFile}
+add_custom_command(OUTPUT ${LLVM_BINARY_DIR}/bin/${BinFile}
COMMAND ${CMAKE_COMMAND} -E make_directory
- ${CMAKE_BINARY_DIR}/bin
+ ${LLVM_BINARY_DIR}/bin
COMMAND ${CMAKE_COMMAND} -E copy
  ${CMAKE_CURRENT_SOURCE_DIR}/bin/${BinFile}
- ${CMAKE_BINARY_DIR}/bin/
+ ${LLVM_BINARY_DIR}/bin/
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/bin/${BinFile})
-list(APPEND Depends ${CMAKE_BINARY_DIR}/bin/${BinFile})
+list(APPEND Depends ${LLVM_BINARY_DIR}/bin/${BinFile})
 install(PROGRAMS bin/${BinFile}
 DESTINATION bin
 

[PATCH] D72390: [www] Remove stale text about default c++ standard from cxx_status.html

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Ping. Is this okay to land, or have I missed something? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72390



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

[PATCH] D72390: [www] Remove stale text about default c++ standard from cxx_status.html

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: rsmith, t.p.northover.
Herald added a project: clang.

Since r320250 c++98 is no longer the default, but cxx_status.html still said it 
was. I think the original intent of the statement was to say that clang by 
default supports some c++11 features which is no longer as pertinent 
information so this patch removes this statement. The page no longer mentions 
the default c++ dialect.

An alternative would be to say the default prior to Clang 6.0 (gnu++98) and 
from 6.0 onwards (gnu++14).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72390

Files:
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -59,10 +59,9 @@
   href="https://www.iso.org/standard/50372.html;>ISO
   C++ 2011 standard.
 
-By default, Clang builds C++ code according to the C++98 standard, with many
-C++11 features accepted as extensions. You can use Clang in C++11 mode with the
--std=c++11 option. Clang's C++11 mode can be used
-with https://libcxx.llvm.org/;>libc++ or with gcc's libstdc++.
+You can use Clang in C++11 mode with the -std=c++11 option.
+Clang's C++11 mode can be used with https://libcxx.llvm.org/;>libc++ or with gcc's libstdc++.
 
 
 List of features and minimum Clang version with support


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -59,10 +59,9 @@
   href="https://www.iso.org/standard/50372.html;>ISO
   C++ 2011 standard.
 
-By default, Clang builds C++ code according to the C++98 standard, with many
-C++11 features accepted as extensions. You can use Clang in C++11 mode with the
--std=c++11 option. Clang's C++11 mode can be used
-with https://libcxx.llvm.org/;>libc++ or with gcc's libstdc++.
+You can use Clang in C++11 mode with the -std=c++11 option.
+Clang's C++11 mode can be used with https://libcxx.llvm.org/;>libc++ or with gcc's libstdc++.
 
 
 List of features and minimum Clang version with support
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71347: Add TimeTraceScope constructor without detail arg to simplify code where no detail required

[Russell Gallop via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdf494f7512b0: [Support] Add TimeTraceScope constructor 
without detail arg (authored by russell.gallop).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71347

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -123,7 +123,9 @@
 J.attribute("ts", StartUs);
 J.attribute("dur", DurUs);
 J.attribute("name", E.Name);
-J.attributeObject("args", [&] { J.attribute("detail", E.Detail); });
+if (!E.Detail.empty()) {
+  J.attributeObject("args", [&] { J.attribute("detail", E.Detail); });
+}
   });
 }
 
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -58,6 +58,10 @@
   TimeTraceScope(TimeTraceScope &&) = delete;
   TimeTraceScope =(TimeTraceScope &&) = delete;
 
+  TimeTraceScope(StringRef Name) {
+if (TimeTraceProfilerInstance != nullptr)
+  timeTraceProfilerBegin(Name, StringRef(""));
+  }
   TimeTraceScope(StringRef Name, StringRef Detail) {
 if (TimeTraceProfilerInstance != nullptr)
   timeTraceProfilerBegin(Name, Detail);
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -246,7 +246,7 @@
 
   // Execute the frontend actions.
   {
-llvm::TimeTraceScope TimeScope("ExecuteCompiler", StringRef(""));
+llvm::TimeTraceScope TimeScope("ExecuteCompiler");
 Success = ExecuteCompilerInvocation(Clang.get());
   }
 
Index: clang/lib/Serialization/GlobalModuleIndex.cpp
===
--- clang/lib/Serialization/GlobalModuleIndex.cpp
+++ clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -134,7 +134,7 @@
"' failed: " + toString(std::move(Err)));
   };
 
-  llvm::TimeTraceScope TimeScope("Module LoadIndex", StringRef(""));
+  llvm::TimeTraceScope TimeScope("Module LoadIndex");
   // Read the global index.
   bool InGlobalIndexBlock = false;
   bool Done = false;
@@ -770,7 +770,7 @@
   }
 
   using namespace llvm;
-  llvm::TimeTraceScope TimeScope("Module WriteIndex", StringRef(""));
+  llvm::TimeTraceScope TimeScope("Module WriteIndex");
 
   // Emit the file header.
   Stream.Emit((unsigned)'B', 8);
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -924,8 +924,7 @@
   }
 
   {
-llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
-   StringRef(""));
+llvm::TimeTraceScope TimeScope("PerformPendingInstantiations");
 PerformPendingInstantiations();
   }
 
Index: clang/lib/Parse/ParseAST.cpp
===
--- clang/lib/Parse/ParseAST.cpp
+++ clang/lib/Parse/ParseAST.cpp
@@ -151,7 +151,7 @@
   bool HaveLexer = S.getPreprocessor().getCurrentLexer();
 
   if (HaveLexer) {
-llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
+llvm::TimeTraceScope TimeScope("Frontend");
 P.Initialize();
 Parser::DeclGroupPtrTy ADecl;
 for (bool AtEOF = P.ParseFirstTopLevelDecl(ADecl); !AtEOF;
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -249,7 +249,7 @@
 
 void HandleTranslationUnit(ASTContext ) override {
   {
-llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
+llvm::TimeTraceScope TimeScope("Frontend");
 PrettyStackTraceString CrashInfo("Per-file LLVM IR generation");
 if (FrontendTimesIsEnabled) {
   LLVMIRGenerationRefCount += 1;
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -896,7 +896,7 @@
 
   {
 PrettyStackTraceString CrashInfo("Per-function optimization");
-llvm::TimeTraceScope TimeScope("PerFunctionPasses", StringRef(""));
+llvm::TimeTraceScope TimeScope("PerFunctionPasses");
 
 PerFunctionPasses.doInitialization();
 for (Function  : *TheModule)
@@ -907,13 +907,13 @@
 
   {
 

[PATCH] D71347: Add TimeTraceScope constructor without detail arg to simplify code where no detail required

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added a reviewer: anton-afanasyev.
Herald added subscribers: cfe-commits, arphaman, hiraditya.
Herald added projects: clang, LLVM.

Also don't write out detail when it is empty, which reduces size of generated 
traces.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71347

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -123,7 +123,9 @@
 J.attribute("ts", StartUs);
 J.attribute("dur", DurUs);
 J.attribute("name", E.Name);
-J.attributeObject("args", [&] { J.attribute("detail", E.Detail); });
+if (!E.Detail.empty()) {
+  J.attributeObject("args", [&] { J.attribute("detail", E.Detail); });
+}
   });
 }
 
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -58,6 +58,10 @@
   TimeTraceScope(TimeTraceScope &&) = delete;
   TimeTraceScope =(TimeTraceScope &&) = delete;
 
+  TimeTraceScope(StringRef Name) {
+if (TimeTraceProfilerInstance != nullptr)
+  timeTraceProfilerBegin(Name, StringRef(""));
+  }
   TimeTraceScope(StringRef Name, StringRef Detail) {
 if (TimeTraceProfilerInstance != nullptr)
   timeTraceProfilerBegin(Name, Detail);
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -246,7 +246,7 @@
 
   // Execute the frontend actions.
   {
-llvm::TimeTraceScope TimeScope("ExecuteCompiler", StringRef(""));
+llvm::TimeTraceScope TimeScope("ExecuteCompiler");
 Success = ExecuteCompilerInvocation(Clang.get());
   }
 
Index: clang/lib/Serialization/GlobalModuleIndex.cpp
===
--- clang/lib/Serialization/GlobalModuleIndex.cpp
+++ clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -134,7 +134,7 @@
"' failed: " + toString(std::move(Err)));
   };
 
-  llvm::TimeTraceScope TimeScope("Module LoadIndex", StringRef(""));
+  llvm::TimeTraceScope TimeScope("Module LoadIndex");
   // Read the global index.
   bool InGlobalIndexBlock = false;
   bool Done = false;
@@ -770,7 +770,7 @@
   }
 
   using namespace llvm;
-  llvm::TimeTraceScope TimeScope("Module WriteIndex", StringRef(""));
+  llvm::TimeTraceScope TimeScope("Module WriteIndex");
 
   // Emit the file header.
   Stream.Emit((unsigned)'B', 8);
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -924,8 +924,7 @@
   }
 
   {
-llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
-   StringRef(""));
+llvm::TimeTraceScope TimeScope("PerformPendingInstantiations");
 PerformPendingInstantiations();
   }
 
Index: clang/lib/Parse/ParseAST.cpp
===
--- clang/lib/Parse/ParseAST.cpp
+++ clang/lib/Parse/ParseAST.cpp
@@ -151,7 +151,7 @@
   bool HaveLexer = S.getPreprocessor().getCurrentLexer();
 
   if (HaveLexer) {
-llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
+llvm::TimeTraceScope TimeScope("Frontend");
 P.Initialize();
 Parser::DeclGroupPtrTy ADecl;
 for (bool AtEOF = P.ParseFirstTopLevelDecl(ADecl); !AtEOF;
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -249,7 +249,7 @@
 
 void HandleTranslationUnit(ASTContext ) override {
   {
-llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
+llvm::TimeTraceScope TimeScope("Frontend");
 PrettyStackTraceString CrashInfo("Per-file LLVM IR generation");
 if (FrontendTimesIsEnabled) {
   LLVMIRGenerationRefCount += 1;
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -896,7 +896,7 @@
 
   {
 PrettyStackTraceString CrashInfo("Per-function optimization");
-llvm::TimeTraceScope TimeScope("PerFunctionPasses", StringRef(""));
+llvm::TimeTraceScope TimeScope("PerFunctionPasses");
 
 PerFunctionPasses.doInitialization();
 for (Function  : *TheModule)
@@ -907,13 +907,13 @@
 
   {
 

[PATCH] D70950: Add ProcName to TimeTraceProfiler

[Russell Gallop via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaedeab7f85ca: [Support] Add ProcName to TimeTraceProfiler 
(authored by russell.gallop).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70950

Files:
  clang/test/Driver/check-time-trace.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp


Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -14,6 +14,7 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
 #include 
 #include 
 #include 
@@ -58,8 +59,8 @@
 };
 
 struct TimeTraceProfiler {
-  TimeTraceProfiler(unsigned TimeTraceGranularity = 0)
-  : StartTime(steady_clock::now()),
+  TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
+  : StartTime(steady_clock::now()), ProcName(ProcName),
 TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
@@ -167,7 +168,7 @@
   J.attribute("ts", 0);
   J.attribute("ph", "M");
   J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", "clang"); });
+  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
 });
 
 J.arrayEnd();
@@ -179,15 +180,18 @@
   SmallVector Entries;
   StringMap CountAndTotalPerName;
   const TimePointType StartTime;
+  const std::string ProcName;
 
   // Minimum time granularity (in microseconds)
   const unsigned TimeTraceGranularity;
 };
 
-void timeTraceProfilerInitialize(unsigned TimeTraceGranularity) {
+void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
+ StringRef ProcName) {
   assert(TimeTraceProfilerInstance == nullptr &&
  "Profiler should not be initialized");
-  TimeTraceProfilerInstance = new TimeTraceProfiler(TimeTraceGranularity);
+  TimeTraceProfilerInstance = new TimeTraceProfiler(
+  TimeTraceGranularity, llvm::sys::path::filename(ProcName));
 }
 
 void timeTraceProfilerCleanup() {
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -19,7 +19,8 @@
 /// Initialize the time trace profiler.
 /// This sets up the global \p TimeTraceProfilerInstance
 /// variable to be the profiler instance.
-void timeTraceProfilerInitialize(unsigned TimeTraceGranularity);
+void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
+ StringRef ProcName);
 
 /// Cleanup the time trace profiler, if it was initialized.
 void timeTraceProfilerCleanup();
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -218,7 +218,7 @@
 
   if (Clang->getFrontendOpts().TimeTrace) {
 llvm::timeTraceProfilerInitialize(
-Clang->getFrontendOpts().TimeTraceGranularity);
+Clang->getFrontendOpts().TimeTraceGranularity, Argv0);
   }
   // --print-supported-cpus takes priority over the actual compilation.
   if (Clang->getFrontendOpts().PrintSupportedCPUs)
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -12,7 +12,7 @@
 // CHECK-NEXT: "pid":
 // CHECK-NEXT: "tid":
 // CHECK-NEXT: "ts":
-// CHECK: "name": "clang"
+// CHECK: "name": "clang{{.*}}"
 // CHECK: "name": "process_name"
 
 template 


Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -14,6 +14,7 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
 #include 
 #include 
 #include 
@@ -58,8 +59,8 @@
 };
 
 struct TimeTraceProfiler {
-  TimeTraceProfiler(unsigned TimeTraceGranularity = 0)
-  : StartTime(steady_clock::now()),
+  TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
+  : StartTime(steady_clock::now()), ProcName(ProcName),
 TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
@@ -167,7 +168,7 @@
   J.attribute("ts", 0);
   J.attribute("ph", "M");
   J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", "clang"); });
+  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
 });
 
 J.arrayEnd();
@@ 

[PATCH] D70950: Add ProcName to TimeTraceProfiler

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added a reviewer: anton-afanasyev.
Herald added subscribers: cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

This was hard-coded to "clang". This change allows it to to be used on 
processes other than clang (such as lld).

This gets reported as clang-10 on Linux and clang.exe on Windows so adapted 
test to accommodate this.

This change is working towards LLD time trace support, RFC here: 
https://reviews.llvm.org/D69043


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70950

Files:
  clang/test/Driver/check-time-trace.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp


Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -14,6 +14,7 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
 #include 
 #include 
 #include 
@@ -58,8 +59,8 @@
 };
 
 struct TimeTraceProfiler {
-  TimeTraceProfiler(unsigned TimeTraceGranularity = 0)
-  : StartTime(steady_clock::now()),
+  TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
+  : StartTime(steady_clock::now()), ProcName(ProcName),
 TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
@@ -167,7 +168,7 @@
   J.attribute("ts", 0);
   J.attribute("ph", "M");
   J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", "clang"); });
+  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
 });
 
 J.arrayEnd();
@@ -179,15 +180,18 @@
   SmallVector Entries;
   StringMap CountAndTotalPerName;
   const TimePointType StartTime;
+  const std::string ProcName;
 
   // Minimum time granularity (in microseconds)
   const unsigned TimeTraceGranularity;
 };
 
-void timeTraceProfilerInitialize(unsigned TimeTraceGranularity) {
+void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
+ StringRef ProcName) {
   assert(TimeTraceProfilerInstance == nullptr &&
  "Profiler should not be initialized");
-  TimeTraceProfilerInstance = new TimeTraceProfiler(TimeTraceGranularity);
+  TimeTraceProfilerInstance = new TimeTraceProfiler(
+  TimeTraceGranularity, llvm::sys::path::filename(ProcName));
 }
 
 void timeTraceProfilerCleanup() {
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -19,7 +19,8 @@
 /// Initialize the time trace profiler.
 /// This sets up the global \p TimeTraceProfilerInstance
 /// variable to be the profiler instance.
-void timeTraceProfilerInitialize(unsigned TimeTraceGranularity);
+void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
+ StringRef ProcName);
 
 /// Cleanup the time trace profiler, if it was initialized.
 void timeTraceProfilerCleanup();
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -218,7 +218,7 @@
 
   if (Clang->getFrontendOpts().TimeTrace) {
 llvm::timeTraceProfilerInitialize(
-Clang->getFrontendOpts().TimeTraceGranularity);
+Clang->getFrontendOpts().TimeTraceGranularity, Argv0);
   }
   // --print-supported-cpus takes priority over the actual compilation.
   if (Clang->getFrontendOpts().PrintSupportedCPUs)
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -12,7 +12,7 @@
 // CHECK-NEXT: "pid":
 // CHECK-NEXT: "tid":
 // CHECK-NEXT: "ts":
-// CHECK: "name": "clang"
+// CHECK: "name": "clang{{.*}}"
 // CHECK: "name": "process_name"
 
 template 


Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -14,6 +14,7 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
 #include 
 #include 
 #include 
@@ -58,8 +59,8 @@
 };
 
 struct TimeTraceProfiler {
-  TimeTraceProfiler(unsigned TimeTraceGranularity = 0)
-  : StartTime(steady_clock::now()),
+  TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
+  : StartTime(steady_clock::now()), ProcName(ProcName),
 TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
@@ -167,7 +168,7 @@
   J.attribute("ts", 0);
   

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Thanks for the updated timings. I have no objection to this going in.

I haven't gone through the code changes in detail so I think you should 
probably get approval from someone who has (such as @hans).


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

https://reviews.llvm.org/D69825



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

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

I've re-implemented this using thread local time tracing.

Have broken the changes down into a couple of patches. First one doing some 
minor tidying up here (https://reviews.llvm.org/D70904).


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] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D69825#1760400 , @aganea wrote:

> In D69825#1760373 , @russell.gallop 
> wrote:
>
> > It looks like the git apply didn't work, but the script continued so this 
> > was a duff experiment, please ignore. I'll try to fix this and run it again.
>


Note for other reviewers, the abba_test.ps1 script uses a git format patch, not 
the format you can download from Phabricator.

> Thanks Russell! Can you try running the script overnight? It'll give a better 
> result and we'll see the error bar.

Okay. With the patch sorted, I ran for 10 hours overnight.

  Total iterations:  21
  
   | Min   |Mean   |   Median  | 
Max   |
 A |  00:14:11.0340636 |  00:14:33.2494597 |  00:14:25.3644475 |  
00:15:25.4082274 |
 B |  00:13:57.9445995 |  00:14:20.3073502 |  00:14:09.0937819 |  
00:16:06.2885457 |
  Diff | -00:00:13.0894641 | -00:00:12.9421096 | -00:00:16.2706656 |  
00:00:40.8803183 |

From looking at the individual times this was stable for a couple of hours then 
there were some particularly long runs (on A and B), possibly some scheduled 
maintenance tasks, so I'd recommend using the median value to ignore these 
outliers. Based on the median this saves about 1.9% for me.


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

https://reviews.llvm.org/D69825



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D69825#1760279 , @russell.gallop 
wrote:

> So in this case it saved 0.5% of time. Using the previous maths, with 2378 
> clang-cl jobs, this implies process creation time of 29ms. This was fairly 
> soon after a reboot. Maybe I'm just lucky and none of the process creation 
> problems are affecting me on this system (at this moment).


It looks like the git apply didn't work, but the script continued so this was a 
duff experiment, please ignore. I'll try to fix this and run it again.


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

https://reviews.llvm.org/D69825



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D69825#1758949 , @aganea wrote:

> Thanks for the feedback Russell!
>
> Can you possibly try again with `/MT`? (ie. `-DLLVM_USE_CRT_RELEASE=MT`)


I tried adding this to my stage 1 builds and it didn't make any significant 
difference (times in seconds):

No patch (573.879, 568.519, 567.709)
With Patch (578.253, 558.515, 562.368)

In case it makes a difference, this is from a VS2017 command prompt with Visual 
Studio 15.9.17.

> In the `abba_test.ps1` script, `ninja check-all` is only used for preparing 
> clang.exe with the patch. The A/B loop //does not// use `check-all`.

Ah, yes. Sorry, my mistake.

> I also found out that on some of our multi-socket servers, Ninja defaulted to 
> only the # threads from the first socket. If you had a 2x 18-core system, 
> Ninja would default to 38 threads (18 x 2 + 2) instead of 74 threads (2 x 18 
> x 2 + 2). This behavior seems to be random,  depending on the system. This 
> has been fixed by Ninja PR 1674 
> , I've tested all our systems 
> with this patch and they now all default to the proper # of threads.

I'm not testing on a multi-socket system so don't believe I need that patch.

> I'm doing a new round of tests, I'll get back with updated figures.

Thanks.

I ran the new version of abba_test.ps1 (just building LLVM) with small 
modifications to comment out the "Validate Ninja" step and change "check-all" 
to "all" to save time as I'm concentrating on build build speed. This means 
that I'm applying it to the same git revision as you. For 
clang_bypass_cc1.patch I used the patch I downloaded before (before spinning 
off D70568 ). Running for one hour on a six 
core machine (winver 1803, 14 ninja jobs, CFG enabled, recently rebooted) I get:

  Total iterations:  2
  
   | Min   |Mean   |   Median  | 
Max   |
 A |  00:15:13.0773389 |  00:15:13.7219710 |  00:15:13.7219710 |  
00:15:14.3666030 |
 B |  00:15:08.6134425 |  00:15:09.0363284 |  00:15:09.0363284 |  
00:15:09.4592142 |
  Diff | -00:00:04.4638964 | -00:00:04.6856426 | -00:00:04.6856426 | 
-00:00:04.9073888 |

So in this case it saved 0.5% of time. Using the previous maths, with 2378 
clang-cl jobs, this implies process creation time of 29ms. This was fairly soon 
after a reboot. Maybe I'm just lucky and none of the process creation problems 
are affecting me on this system (at this moment).


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

https://reviews.llvm.org/D69825



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

Thanks for this change. I applied this patch (prior to splitting out 
https://reviews.llvm.org/D70568) and it built and worked okay (I only see one 
clang-cl in process explorer).

I don't see anything like same performance improvement however. I did my own 
assessment of build times. I built 2 x stage 1 toolchains using clang 9.0.0 
with and without this patch (applied to 
1b9ef3bbb595206b0097b7adec2c1b69eae6fab4 
). Then I 
used each of those to build LLVM (no clang or lld) again.

CMake settings:

  # Build stage 1 (same with and without patch)
  cmake -G Ninja -DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" 
-DCMAKE_LINKER="C:/Program Files/LLVM/bin/lld-link.exe" 
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=clang;lld 
-DLLVM_TARGETS_TO_BUILD=X86 -DCLANG_ENABLE_STATIC_ANALYZER=OFF 
-DCLANG_ENABLE_ARCMT=OFF ../llvm
  
  # Build stage 2 without patch
  cmake -G Ninja 
-DCMAKE_C_COMPILER="/build_stage1_no_patch/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="/build_stage1_no_patch/bin/clang-cl.exe"
 -DCMAKE_LINKER="/build_stage1_no_patch/bin/lld-link.exe" 
-DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD=X86 ../llvm
  
  # Build stage 2 with patch
  cmake -G Ninja 
-DCMAKE_C_COMPILER="/build_stage1_patch/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="/build_stage1_patch/bin/clang-cl.exe" 
-DCMAKE_LINKER="/build_stage1_patch/bin/lld-link.exe" 
-DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD=X86 ../llvm

These both built the patched code.

I got a ~1.5% performance improvement, reducing "ninja all" from 571.5s to 
562.5s seconds on a 6 core machine (average over three 3 builds on a quiet 
machine), times below. For reference I am using winver 1803 and have CFG 
enabled.

Assuming that `time_saved ≈ (process_invocation_overhead * compiler_jobs) / 
parallel_build_jobs`
or (rearranging) `process_invocation_overhead ≈ (time_saved * 
parallel_build_jobs) / compiler_jobs`
(I know this is not perfectly true as build jobs will tail off towards the end 
of a build, but I think is okay for a ballpark estimate.)

I saved 9 seconds, over 1720 compiler jobs (from `ninja -v -n all | grep -c 
clang-cl`), with 14 parallel build jobs which gives the process invocation 
overhead of about 73ms which is in the range you mentioned above (30-100ms). 
Therefore I think that this is in the right ballpark.

I can't see how this would get 20% (from the table) or 30% improvement (from 
the first graph). In `abba_test.ps1` you include running tests (due to 
`check-all`) so it is possible that you are saving time on a very large number 
of clang driver invocations on very small files. This is helpful for LLVM 
developers but I don't think it's representative of other builds. 
Alternatively, is the process overhead higher on your machine(s) for some 
reason (e.g. security software).

To be clear, I'm not against this patch going in, and I can confirm that it is 
a performance improvement so good to have. I just can't see where the claimed 
20-30% saving on build time comes from.

Time data for 3 builds (seconds)
No patch (571.349, 575.353, 567.638)
With Patch (560.870, 563.110, 563.368)


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

https://reviews.llvm.org/D69825



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

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D69043#1714060 , @ruiu wrote:

> I applied this patch and built clang with ThinLTO. Here is the generated file:
>  This file seems much smaller than yours. What am I missing?


I have seen some very empty looking traces when ThinLTO doesn't have much to 
do. Is it possible that the ThinLTO cache is doing a very good job?

> (I couldn't download your file because the server times out perhaps due to 
> the file size.)

That's odd. I can download it okay from here.


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] D69043: [RFC] Adding time-trace to LLD?

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D69043#1712542 , @ruiu wrote:

> This seems useful. Can I see an example output?


Thanks.

Here's an example from Building clang with ThinLTO: F10296420: clang-10.json 


Using linker flags: -Wl,-time-trace -Wl,-time-trace-granularity=5

I increased the granularity from the default, otherwise the trace gets very 
large (~800MiB).

> Speaking of the output file, I'd make --time-trace option to take an output 
> filename, as an output executable doesn't always have an extension (On 
> Windows it's almost always .exe, but on Unix, extension is usually omitted).

Okay. I'll look at doing that.


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] D69043: [RFC] Adding time-trace to LLD?

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: ruiu, pcc, anton-afanasyev.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, hiraditya, 
arichardson, mehdi_amini, emaste.
Herald added a reviewer: espindola.
Herald added projects: clang, LLVM.

The clang -ftime-trace feature is useful for analysing build time in compiles. 
I've been experimenting with adding time-trace support to LLD for analysing 
link times.

Adding the option -time-trace produces a .json file alongside the output ELF 
file (just ELF so far). This works for LTO and ThinLTO, picking up the relevant 
markers from LLVM. Example ThinLTO build:
F10284608: ThinLTOTimeTrace.PNG 

This is still work in progress so shouldn't be submitted in its present state. 
There are a few things that still need doing:

- Improve how events are collected. (The current method combines multiple 
threads (ThinLTO) in one stack with a mutex which won't scale well. Making this 
thread local would probably be better.)
- Adding test(s)
- Re-enable the assertion for monotonically increasing times which I don't have 
working yet.

Do people think that this will be useful/worth pursuing?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69043

Files:
  clang/tools/driver/cc1_main.cpp
  lld/ELF/Driver.cpp
  lld/ELF/ICF.cpp
  lld/ELF/MarkLive.cpp
  lld/ELF/Options.td
  lld/ELF/Writer.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -13,8 +13,9 @@
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
-#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include 
 #include 
 #include 
@@ -37,10 +38,11 @@
   TimePointType End;
   std::string Name;
   std::string Detail;
+  uint64_t Tid;
 
   Entry(TimePointType &, TimePointType &, std::string &, std::string &)
   : Start(std::move(S)), End(std::move(E)), Name(std::move(N)),
-Detail(std::move(Dt)){};
+Detail(std::move(Dt)), Tid(llvm::get_threadid()){};
 
   // Calculate timings for FlameGraph. Cast time points to microsecond precision
   // rather than casting duration. This avoid truncation issues causing inner
@@ -59,26 +61,47 @@
 };
 
 struct TimeTraceProfiler {
-  TimeTraceProfiler() {
+  TimeTraceProfiler(StringRef ProgName) {
 StartTime = steady_clock::now();
+this->ProgName = ProgName;
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
+std::lock_guard Lock(Mu);
 Stack.emplace_back(steady_clock::now(), TimePointType(), std::move(Name),
Detail());
   }
 
   void end() {
+std::lock_guard Lock(Mu);
+uint64_t Tid = llvm::get_threadid();
+
 assert(!Stack.empty() && "Must call begin() first");
-auto  = Stack.back();
+
+// Find the latest entry from this thread on the stack.
+auto LatestEvent =
+std::find_if(Stack.rbegin(), Stack.rend(),
+ [&](const Entry ) { return Val.Tid == Tid; });
+// Should always be able to find a stack entry on this thread.
+assert(LatestEvent != Stack.rend() &&
+   "end() without begin() found on thread");
+
+// Get reference from iterator.
+auto  = *LatestEvent;
+
 E.End = steady_clock::now();
 
 // Check that end times monotonically increase.
-assert((Entries.empty() ||
-(E.getFlameGraphStartUs(StartTime) + E.getFlameGraphDurUs() >=
- Entries.back().getFlameGraphStartUs(StartTime) +
- Entries.back().getFlameGraphDurUs())) &&
-   "TimeProfiler scope ended earlier than previous scope");
+//auto LastEntryOnThreadIter = std::find_if(Entries.rbegin(),
+//Entries.rend(), [&](const Entry ) {
+//  return Val.Tid == Tid;
+//});
+//auto  = *LastEntryOnThreadIter;
+//assert((Entries.empty() ||
+//(E.getFlameGraphStartUs(StartTime) + E.getFlameGraphDurUs() >=
+// LastEntryOnThread.getFlameGraphStartUs(StartTime) +
+// LastEntryOnThread.getFlameGraphDurUs())) &&
+//   "TimeProfiler scope ended earlier than previous scope");
 
 // Calculate duration at full precision for overall counts.
 DurationType Duration = E.End - E.Start;
@@ -92,18 +115,21 @@
 // templates from within, we only want to add the topmost one. "topmost"
 // happens to be the ones that don't have any currently open entries above
 // itself.
-if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry ) {
-  return Val.Name == E.Name;
-}) == Stack.rend()) {
+if (std::find_if(std::next(LatestEvent), Stack.rend(),
+  

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

This is failing the sanitizer lint check:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/23819/steps/64-bit%20check-sanitizer/logs/stdio

/compiler-rt/lib/ubsan/ubsan_checks.inc:22:  Lines should be <= 80 
characters long  [whitespace/line_length] [2]
/compiler-rt/lib/ubsan/ubsan_checks.inc:23:  Lines should be <= 80 
characters long  [whitespace/line_length] [2]

Please could you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
russell.gallop marked an inline comment as done.
Closed by commit rG9d9ac46a08d7: Remove rest of time-trace message as it is 
inconsistent style (authored by russell.gallop).

Changed prior to commit:
  https://reviews.llvm.org/D68710?vs=224278=224290#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68710

Files:
  clang/include/clang/Driver/Options.td
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1791,7 +1791,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, 
Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time 
profiler">,
   Flags<[CC1Option, CoreOption]>;


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1791,7 +1791,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 224278.
russell.gallop added a comment.

Update so --help helps user find trace file.
Documentation has the further details on how to view it.


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

https://reviews.llvm.org/D68710

Files:
  clang/include/clang/Driver/Options.td
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler.  Generates JSON file based on output 
filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, 
Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time 
profiler">,
   Flags<[CC1Option, CoreOption]>;


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler.  Generates JSON file based on output filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop marked 2 inline comments as done.
russell.gallop added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1797
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename. "
+   "Results can be analyzed with chrome://tracing or `Speedscope App "
+   "`_ for flamegraph visualization." >,

sylvestre.ledru wrote:
> I am not a big fan of adding products in the in-product help.
> If Chrome removes the feature or Speedscope goes done, we will still list 
> that in the old version of clang.
> Deprecated doc is more common and accepted.
> 
> I would remove this line and the next line and keep the doc.
> 
> I would remove this line and the next line and keep the doc.

I agree. Updated to do that via the tablegen file. Removed the changes to the 
rst file which is generated from tablegen.


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

https://reviews.llvm.org/D68710



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

[PATCH] D68710: Remove time-trace message as it inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: anton-afanasyev, sylvestre.ledru.
Herald added a project: clang.

https://reviews.llvm.org/D68260 removed some of the message that -ftime-trace 
prints out. Running large builds still prints out a lot of "Time trace 
json-file dumped to " messages which are not the normal style for clang and are 
not necessary for locating the trace file.

There are other options which create files but don't tell you where they've 
written them. For example, this command generates a time trace, an yaml 
optimization record and a dependency file but only tells you about the time 
trace file:

$ clang -c foo.cpp -ftime-trace -fsave-optimisation-record -MD
Time trace json-file dumped to foo.json
$ ls
foo.cpp foo.d foo.json foo.o foo.opt.yaml

Supplemented the command line reference to help users locate this file and 
updated Options.td which is used to generate ClangCommandLineReference and 
clang --help.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68710

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,10 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename. "
+   "Results can be analyzed with chrome://tracing or `Speedscope App "
+   "`_ for flamegraph visualization." >,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, 
Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time 
profiler">,
   Flags<[CC1Option, CoreOption]>;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1942,8 +1942,9 @@
 
 .. option:: -ftime-trace
 
-Turn on time profiler. Results can be analyzed with chrome://tracing or
-`Speedscope App `_ for flamegraph visualization
+Turn on time profiler. Generates JSON file based on output filename.  Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization
 
 .. option:: -ftime-trace-granularity=
 


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,10 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output filename. "
+   "Results can be analyzed with chrome://tracing or `Speedscope App "
+   "`_ for flamegraph visualization." >,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1942,8 +1942,9 @@
 
 .. option:: -ftime-trace
 
-Turn on time profiler. Results can be analyzed with chrome://tracing or
-`Speedscope 

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> The other thing worth checking is the clang PGO self-host on Windows.
>  This has the potential to break that, and the fix would be to add a linker 
> flag in LLVM's cmake.

This does indeed break PGO self-host with lld-link (applied on top of r373200):

  <...>\bin\lld-link.exe /nologo utils\not\CMakeFiles\not.dir\not.cpp.obj 
utils\not\CMakeFiles\not.dir\__\__\resources\windows_version_resource.rc.res 
/out:bin\not.exe /implib:lib\not.lib /pdb:bin\not.pdb /version:0.0 /machine:x64 
-fuse-ld=lld /STACK:1000 /INCREMENTAL:NO /subsystem:console 
lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib 
delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib 
kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib 
oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST 
/MANIFESTFILE:bin\not.exe.manifest
  lld-link: warning: ignoring unknown argument '-fuse-ld=lld'
  lld-link: error: could not open 'clang_rt.profile-x86_64.lib': no such file 
or directory

So at the very least this change would need that path adding.

I agree with Nico that having to add a path dependent on the LLVM version 
sounds like a pain.

Is it possible for the compiler to embed a /libpath as well as the dependent 
lib? That goes back to having a path embedded, though you could override it if 
required so could be an improvement over things at the moment.

It sounds to me like:

> ...folks have to explicitly pass 
> /libpath:\path\to\clang\lib\clang\$changing_version\lib\windows to the 
> linker, which to me is a pretty poor experience

and

> I'd like to get away from having paths embedded in the object if possible.

are hard to reconcile.

It may be possible to have the Windows installer add the path to LIB 
environment variable but that would rely on having run the installer, and could 
cause problems if you have multiple versions of LLVM around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65543



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

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

[Russell Gallop via Phabricator via cfe-commits]

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

I don't know a lot about the structure of clang but it LGTM from the point of 
view of the code and traces coming out.

I'm not very keen on having two "Frontend" sections, but I think it works okay. 
It moves Clang CodeGen under "Frontend". "Total Frontend" and "Total Backend" 
between them now cover almost all of the execution time (so is more accurate 
than without this change).

I think there's always going to be a trade off with time-trace between 
presenting something simple that makes sense to an end user and revealing how 
LLVM/Clang is actually structured inside, which is much more complicated (and 
doesn't nicely fit into scopes).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63325



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

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added inline comments.



Comment at: llvm/lib/Support/TimeProfiler.cpp:67
 
 // Only include sections longer than TimeTraceGranularity msec.
-if (duration_cast(E.Duration).count() > TimeTraceGranularity)

This comment looks wrong since this change. Please can you update or reword it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63325



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 203124.
russell.gallop added a comment.

Re-added test cases using variables and added comment. This now tests both 
formats.


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

https://reviews.llvm.org/D62850

Files:
  clang/test/CodeGen/builtins-x86.c


Index: clang/test/CodeGen/builtins-x86.c
===
--- clang/test/CodeGen/builtins-x86.c
+++ clang/test/CodeGen/builtins-x86.c
@@ -399,6 +399,17 @@
 #ifndef OPENCL
   (void) _mm_pause();
 #endif
+
+  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, imm_i_0_8);
+
+  // Using non-immediate argument supported for gcc compatibility
   tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, tmp_i);
   tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, tmp_i);
   tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, tmp_i);
@@ -407,6 +418,7 @@
   tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, tmp_i);
   tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, tmp_i);
   tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, tmp_i);
+
   tmp_V1LLi = __builtin_ia32_pmuludq(tmp_V2i, tmp_V2i);
   tmp_V2LLi = __builtin_ia32_pmuludq128(tmp_V4i, tmp_V4i);
   tmp_V8s = __builtin_ia32_psraw128(tmp_V8s, tmp_V8s);
@@ -417,6 +429,17 @@
   tmp_V8s = __builtin_ia32_psllw128(tmp_V8s, tmp_V8s);
   tmp_V4i = __builtin_ia32_pslld128(tmp_V4i, tmp_V4i);
   tmp_V2LLi = __builtin_ia32_psllq128(tmp_V2LLi, tmp_V2LLi);
+
+  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrlwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psrldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, imm_i_0_8);
+
+  // Using non-immediate argument supported for gcc compatibility
   tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, tmp_i);
   tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, tmp_i);
   tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, tmp_i);
@@ -425,6 +448,7 @@
   tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, tmp_i);
   tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, tmp_i);
   tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, tmp_i);
+
   tmp_V4i = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
   (void) __builtin_ia32_monitor(tmp_vp, tmp_Ui, tmp_Ui);
   (void) __builtin_ia32_mwait(tmp_Ui, tmp_Ui);


Index: clang/test/CodeGen/builtins-x86.c
===
--- clang/test/CodeGen/builtins-x86.c
+++ clang/test/CodeGen/builtins-x86.c
@@ -399,6 +399,17 @@
 #ifndef OPENCL
   (void) _mm_pause();
 #endif
+
+  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, imm_i_0_8);
+
+  // Using non-immediate argument supported for gcc compatibility
   tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, tmp_i);
   tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, tmp_i);
   tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, tmp_i);
@@ -407,6 +418,7 @@
   tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, tmp_i);
   tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, tmp_i);
   tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, tmp_i);
+
   tmp_V1LLi = __builtin_ia32_pmuludq(tmp_V2i, tmp_V2i);
   tmp_V2LLi = __builtin_ia32_pmuludq128(tmp_V4i, tmp_V4i);
   tmp_V8s = __builtin_ia32_psraw128(tmp_V8s, tmp_V8s);
@@ -417,6 +429,17 @@
   tmp_V8s = __builtin_ia32_psllw128(tmp_V8s, tmp_V8s);
   tmp_V4i = __builtin_ia32_pslld128(tmp_V4i, tmp_V4i);
   tmp_V2LLi = __builtin_ia32_psllq128(tmp_V2LLi, tmp_V2LLi);
+
+  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrlwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psrldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, imm_i_0_8);
+
+  // Using non-immediate argument supported for gcc compatibility
   tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, tmp_i);
   tmp_V4i = 

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> We support non immediate on these because gcc does.

Thanks. Your comment crossed in mid-air. Okay, so is this test worth changing, 
or should it have both versions (immediate and non-immediate)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62850



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> I'll have a look and see if there is a reason why these don't fail in the 
> same way (which would make the test fail in it's current form).

These do not have the "I" prefix (//  I   -> Required to constant fold to an 
integer constant expression.) in BuiltinsX86.def which is probably why they 
don't error.

Checking against the LLVM side, there is a comment in IntrinsicsX86.td:

  // Oddly these don't require an immediate due to a gcc compatibility issue.
  def int_x86_mmx_pslli_w : GCCBuiltin<"__builtin_ia32_psllwi">,
  Intrinsic<[llvm_x86mmx_ty], [llvm_x86mmx_ty,
 llvm_i32_ty], [IntrNoMem]>;

That comment was added recently by Craig in r355993. So it looks like they 
should work with either for compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62850



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> Then the test should be failing? Or is the current form also legal?

Hmm, __builtin_ia32_insertps128() errors if you pass a variable, but these 
don't (e.g.):
mytest.c:122:13: error: argument to '__builtin_ia32_insertps128' must be a 
constant integer

  tmp_V4f = __builtin_ia32_insertps128(tmp_V4f, tmp_V4f, tmp_i);

I'll have a look and see if there is a reason why these don't fail in the same 
way (which would make the test fail in it's current form).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62850



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> Is the compiler missing a check that these parameters are immediates?

I don't think that there can be a check, or this would have been noticed.

I don't know whether this is possible and/or desirable. Do users use one 
version of the builtin and want the compiler to decide whether to use the 
immediate form?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62850



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> .. or?

I'm not sure what you mean.

> Is this fixing a current test failure?

No. The test is not failing, but it is not doing what was intended as these 
builtins are for generating the immediate form of the corresponding instruction 
and they were generating actually generating a register form.

This test doesn't check the instructions generated are correct, but fixing that 
is a bigger task...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62850



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

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: craig.topper, cfe-commits.
russell.gallop added a project: clang.

Some of these test cases were using a variable instead of a literal so were not 
generating the immediate form of the corresponding instruction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62850

Files:
  clang/test/CodeGen/builtins-x86.c


Index: clang/test/CodeGen/builtins-x86.c
===
--- clang/test/CodeGen/builtins-x86.c
+++ clang/test/CodeGen/builtins-x86.c
@@ -399,14 +399,14 @@
 #ifndef OPENCL
   (void) _mm_pause();
 #endif
-  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, tmp_i);
-  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, tmp_i);
-  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, tmp_i);
-  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, tmp_i);
-  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, tmp_i);
+  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, imm_i_0_8);
   tmp_V1LLi = __builtin_ia32_pmuludq(tmp_V2i, tmp_V2i);
   tmp_V2LLi = __builtin_ia32_pmuludq128(tmp_V4i, tmp_V4i);
   tmp_V8s = __builtin_ia32_psraw128(tmp_V8s, tmp_V8s);
@@ -417,14 +417,14 @@
   tmp_V8s = __builtin_ia32_psllw128(tmp_V8s, tmp_V8s);
   tmp_V4i = __builtin_ia32_pslld128(tmp_V4i, tmp_V4i);
   tmp_V2LLi = __builtin_ia32_psllq128(tmp_V2LLi, tmp_V2LLi);
-  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, tmp_i);
-  tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, tmp_i);
-  tmp_V8s = __builtin_ia32_psrlwi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_psrldi128(tmp_V4i, tmp_i);
-  tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, tmp_i);
-  tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, tmp_i);
+  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrlwi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psrldi128(tmp_V4i, imm_i_0_8);
+  tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, imm_i_0_8);
+  tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, imm_i_0_8);
+  tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, imm_i_0_8);
   tmp_V4i = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
   (void) __builtin_ia32_monitor(tmp_vp, tmp_Ui, tmp_Ui);
   (void) __builtin_ia32_mwait(tmp_Ui, tmp_Ui);


Index: clang/test/CodeGen/builtins-x86.c
===
--- clang/test/CodeGen/builtins-x86.c
+++ clang/test/CodeGen/builtins-x86.c
@@ -399,14 +399,14 @@
 #ifndef OPENCL
   (void) _mm_pause();
 #endif
-  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, tmp_i);
-  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, tmp_i);
-  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, tmp_i);
-  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, tmp_i);
-  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, tmp_i);
-  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, tmp_i);
+  tmp_V4s = __builtin_ia32_psllwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_pslldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psllqi(tmp_V1LLi, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrawi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psradi(tmp_V2i, imm_i_0_8);
+  tmp_V4s = __builtin_ia32_psrlwi(tmp_V4s, imm_i_0_8);
+  tmp_V2i = __builtin_ia32_psrldi(tmp_V2i, imm_i_0_8);
+  tmp_V1LLi = __builtin_ia32_psrlqi(tmp_V1LLi, imm_i_0_8);
   tmp_V1LLi = __builtin_ia32_pmuludq(tmp_V2i, tmp_V2i);
   tmp_V2LLi = __builtin_ia32_pmuludq128(tmp_V4i, tmp_V4i);
   tmp_V8s = __builtin_ia32_psraw128(tmp_V8s, tmp_V8s);
@@ -417,14 +417,14 @@
   tmp_V8s = __builtin_ia32_psllw128(tmp_V8s, tmp_V8s);
   tmp_V4i = __builtin_ia32_pslld128(tmp_V4i, tmp_V4i);
   tmp_V2LLi = __builtin_ia32_psllq128(tmp_V2LLi, tmp_V2LLi);
-  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_pslldi128(tmp_V4i, tmp_i);
-  tmp_V2LLi = __builtin_ia32_psllqi128(tmp_V2LLi, tmp_i);
-  tmp_V8s = __builtin_ia32_psrlwi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_psrldi128(tmp_V4i, tmp_i);
-  tmp_V2LLi = __builtin_ia32_psrlqi128(tmp_V2LLi, tmp_i);
-  tmp_V8s = __builtin_ia32_psrawi128(tmp_V8s, tmp_i);
-  tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, tmp_i);
+  tmp_V8s = __builtin_ia32_psllwi128(tmp_V8s, 

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> I was going to suggest that maybe what we should do is just embed the 
> basename, i.e. /nodefaultlib:clang_rt.profile-x86_64.lib ...

Do you mean /defaultlib:clang_rt.profile-x86_64.lib?

> ... and then we just ask users to add one /libpath: flag to their linker 
> invocation

I'll try that. If it works, should we revisit the ubsan dependent lib support, 
to be consistent?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61742



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

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

> it looks like passing -no-canonical-prefixes makes this path relative

Do you see this working? I tried writing a test and it doesn't appear to work 
for me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61742



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

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a subscriber: ruiu.
russell.gallop added a comment.

> Hello, this embeds an absolute path into the generated .obj file, which means 
> the output now is no longer deterministic (since it depends on the absolute 
> path to clang_rt.profile-x86_64.lib).

Yes, it embeds the absolute path. Note that -fsanitize=undefined has done that 
since r241225 (in 2015).

> This means the output will be different on different machines, which breaks 
> things like caching in distributed builds and whatnot.

I assume that your distributed build sandboxes tool paths so this is 
deterministic from one users point of view, just not deterministic across users 
with tools installed in different locations so it defeats the cache. Yes, 
that's not ideal.

> I can't see an obvious way to save this patch either, since you won't know at 
> compile time what the CWD will be at link time.

I assume you're considering relative paths here. Yes, I don't think that would 
work nicely.

> As-is, this breaks Chromium's deterministic builds. ("Only" the coverage 
> builds, and they don't yet check for determinism, which is why it took us a 
> while to notice.)
>  Suggestions?

If this was made into a library name instead of a path (e.g. 
--dependent-lib=clang_rt.profile-x86_64.lib) then it would solve the problem of 
different paths on different nodes, but then you would have to tell the linker 
where to find this library.

That is *almost* as bad as having to specify the full path to the library to 
the linker in the first place! It's not quite as bad as you don't have to 
specify the architecture and you only need to specify it once regardless of 
whether you're using profiling or sanitizers. If lld-link added the clang 
library path itself, then the library name would be all that was needed from 
the compiler side.

Rui, would it be possible for lld-link to search the path to the clang 
libraries (e.g. C:\Program Files\LLVM\lib\clang\8.0.0\lib\windows) by default? 
Would this have any undesirable side effects?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61742



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

[PATCH] D62200: [Driver][Windows] Add dependent lib argument for -fprofile-generate and -fcs-profile-generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: rnk, hans, bogner.
russell.gallop added a project: clang.

Follows on from r360674 which added it for -fprofile-instr-generate.

Identified when doing: https://reviews.llvm.org/D62063


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62200

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -62,12 +62,16 @@
 // RUN: %clang_cl /Z7 -### -- %s 2>&1 | FileCheck -check-prefix=gdefcolumn %s
 // gdefcolumn-NOT: -dwarf-column-info
 
-// RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-GENERATE %s
-// RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
+// RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-INSTR-GENERATE %s
+// RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-INSTR-GENERATE-FILE %s
+// CHECK-PROFILE-INSTR-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+// CHECK-PROFILE-INSTR-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
+
+// RUN: %clang_cl -### /FA -fprofile-generate -- %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-GENERATE %s
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=llvm" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 
2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file 
-- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
-// CHECK-PROFILE-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
 // RUN: %clang_cl -### /FA -fprofile-instr-use -- %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-USE %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -803,6 +803,10 @@
 CmdArgs.push_back("-fprofile-instrument=csllvm");
   }
   if (PGOGenArg) {
+if (TC.getTriple().isWindowsMSVCEnvironment()) {
+  CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+   TC.getCompilerRT(Args, "profile")));
+}
 if (PGOGenArg->getOption().matches(
 PGOGenerateArg ? options::OPT_fprofile_generate_EQ
: options::OPT_fcs_profile_generate_EQ)) {


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -62,12 +62,16 @@
 // RUN: %clang_cl /Z7 -### -- %s 2>&1 | FileCheck -check-prefix=gdefcolumn %s
 // gdefcolumn-NOT: -dwarf-column-info
 
-// RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE %s
-// RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
+// RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-INSTR-GENERATE %s
+// RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-INSTR-GENERATE-FILE %s
+// CHECK-PROFILE-INSTR-GENERATE: "-fprofile-instrument=clang" "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+// CHECK-PROFILE-INSTR-GENERATE-FILE: "-fprofile-instrument-path=/tmp/somefile.profraw"
+
+// RUN: %clang_cl -### /FA -fprofile-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE %s
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=llvm" "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
-// CHECK-PROFILE-GENERATE-FILE: "-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
 // RUN: %clang_cl -### /FA -fprofile-instr-use -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-USE %s
Index: 

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D61742#1497259 , @rnk wrote:

> Thanks, I would like to do this for the sanitizers as well, since this is a 
> constant pain point for users


That would be good. I think that this might already work for UBSan.


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

https://reviews.llvm.org/D61742



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

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 199016.
russell.gallop added a comment.
Herald added a subscriber: mstorsjo.

Prevent adding -dependent-lib on mingw32 and add tests.


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

https://reviews.llvm.org/D61742

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/instrprof-ld.c


Index: clang/test/Driver/instrprof-ld.c
===
--- clang/test/Driver/instrprof-ld.c
+++ clang/test/Driver/instrprof-ld.c
@@ -129,3 +129,17 @@
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: 
"{{.*}}/Inputs/resource_dir{{/|}}lib{{/|}}windows{{/|}}libclang_rt.profile-x86_64.a"
+
+// Test instrumented profiling dependent-lib flags
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-MINGW-X86-64-DEPENDENT-LIB-NOT: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.a"
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -66,7 +66,7 @@
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 
2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file 
-- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -731,8 +731,9 @@
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation , const Driver ,
-   const InputInfo , const ArgList 
,
+static void addPGOAndCoverageFlags(const ToolChain , Compilation ,
+   const Driver , const InputInfo ,
+   const ArgList ,
ArgStringList ) {
 
   auto *PGOGenerateArg = Args.getLastArg(options::OPT_fprofile_generate,
@@ -783,6 +784,11 @@
ProfileGenerateArg->getValue()));
 // The default is to use Clang Instrumentation.
 CmdArgs.push_back("-fprofile-instrument=clang");
+if (TC.getTriple().isWindowsMSVCEnvironment()) {
+  // Add dependent lib for clang_rt.profile
+  CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+   TC.getCompilerRT(Args, "profile")));
+}
   }
 
   Arg *PGOGenArg = nullptr;
@@ -4176,7 +4182,7 @@
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())
-addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
+addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);
 
   if (auto *ABICompatArg = Args.getLastArg(options::OPT_fclang_abi_compat_EQ))
 ABICompatArg->render(Args, CmdArgs);


Index: clang/test/Driver/instrprof-ld.c
===
--- clang/test/Driver/instrprof-ld.c
+++ clang/test/Driver/instrprof-ld.c
@@ -129,3 +129,17 @@
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: "{{.*}}/Inputs/resource_dir{{/|}}lib{{/|}}windows{{/|}}libclang_rt.profile-x86_64.a"
+
+// Test instrumented profiling dependent-lib flags
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-MINGW-X86-64-DEPENDENT-LIB-NOT: "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.a"
Index: 

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: rnk, bogner.
russell.gallop added a project: clang.

This is needed so lld-link can find clang_rt.profile when self hosting on 
Windows with PGO.

Trying to self host on Windows with PGO runs into undefined symbols as lld-link 
doesn't know to link clang_rt.profile as discussed here:
http://lists.llvm.org/pipermail/llvm-dev/2019-January/129500.html

Using clang-cl as a linker knows to add the library but self hosting, using 
-DCMAKE_LINKER=<...>/lld-link.exe doesn't.  I think that the cleanest way to 
make this work is to add a dependent library when using profile-instr-generate 
on Windows.

Note some cmake changes are also required to get PGO self hosting on Windows 
working. I will post them separately.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61742

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -66,7 +66,7 @@
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 
2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file 
-- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -731,8 +731,9 @@
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation , const Driver ,
-   const InputInfo , const ArgList 
,
+static void addPGOAndCoverageFlags(const ToolChain , Compilation ,
+   const Driver , const InputInfo ,
+   const ArgList ,
ArgStringList ) {
 
   auto *PGOGenerateArg = Args.getLastArg(options::OPT_fprofile_generate,
@@ -783,6 +784,11 @@
ProfileGenerateArg->getValue()));
 // The default is to use Clang Instrumentation.
 CmdArgs.push_back("-fprofile-instrument=clang");
+if (TC.getTriple().isOSWindows()) {
+  // Add dependent lib for clang_rt.profile
+  CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+   TC.getCompilerRT(Args, "profile")));
+}
   }
 
   Arg *PGOGenArg = nullptr;
@@ -4176,7 +4182,7 @@
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())
-addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
+addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);
 
   if (auto *ABICompatArg = Args.getLastArg(options::OPT_fclang_abi_compat_EQ))
 ABICompatArg->render(Args, CmdArgs);


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -66,7 +66,7 @@
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: "-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -731,8 +731,9 @@
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation , const Driver ,
-   const InputInfo , const ArgList ,
+static void addPGOAndCoverageFlags(const ToolChain , Compilation ,
+   const Driver , const InputInfo ,
+ 

[PATCH] D61264: Fix inconsistency in calculating DIAG_START values

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: xazax.hun, rsmith.
russell.gallop added a project: clang.
Herald added a subscriber: rnkovacs.

The inconsistency was introduced at r313975. As DIAG_SIZE_CROSSTU and 
DIAG_SIZE_COMMENT are both 100 this should be NFC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61264

Files:
  clang/include/clang/Basic/DiagnosticIDs.h


Index: clang/include/clang/Basic/DiagnosticIDs.h
===
--- clang/include/clang/Basic/DiagnosticIDs.h
+++ clang/include/clang/Basic/DiagnosticIDs.h
@@ -50,8 +50,8 @@
   DIAG_START_PARSE = DIAG_START_LEX   + DIAG_SIZE_LEX,
   DIAG_START_AST   = DIAG_START_PARSE + DIAG_SIZE_PARSE,
   DIAG_START_COMMENT   = DIAG_START_AST   + DIAG_SIZE_AST,
-  DIAG_START_CROSSTU   = DIAG_START_COMMENT   + DIAG_SIZE_CROSSTU,
-  DIAG_START_SEMA  = DIAG_START_CROSSTU   + DIAG_SIZE_COMMENT,
+  DIAG_START_CROSSTU   = DIAG_START_COMMENT   + DIAG_SIZE_COMMENT,
+  DIAG_START_SEMA  = DIAG_START_CROSSTU   + DIAG_SIZE_CROSSTU,
   DIAG_START_ANALYSIS  = DIAG_START_SEMA  + DIAG_SIZE_SEMA,
   DIAG_START_REFACTORING   = DIAG_START_ANALYSIS  + DIAG_SIZE_ANALYSIS,
   DIAG_UPPER_LIMIT = DIAG_START_REFACTORING   + 
DIAG_SIZE_REFACTORING


Index: clang/include/clang/Basic/DiagnosticIDs.h
===
--- clang/include/clang/Basic/DiagnosticIDs.h
+++ clang/include/clang/Basic/DiagnosticIDs.h
@@ -50,8 +50,8 @@
   DIAG_START_PARSE = DIAG_START_LEX   + DIAG_SIZE_LEX,
   DIAG_START_AST   = DIAG_START_PARSE + DIAG_SIZE_PARSE,
   DIAG_START_COMMENT   = DIAG_START_AST   + DIAG_SIZE_AST,
-  DIAG_START_CROSSTU   = DIAG_START_COMMENT   + DIAG_SIZE_CROSSTU,
-  DIAG_START_SEMA  = DIAG_START_CROSSTU   + DIAG_SIZE_COMMENT,
+  DIAG_START_CROSSTU   = DIAG_START_COMMENT   + DIAG_SIZE_COMMENT,
+  DIAG_START_SEMA  = DIAG_START_CROSSTU   + DIAG_SIZE_CROSSTU,
   DIAG_START_ANALYSIS  = DIAG_START_SEMA  + DIAG_SIZE_SEMA,
   DIAG_START_REFACTORING   = DIAG_START_ANALYSIS  + DIAG_SIZE_ANALYSIS,
   DIAG_UPPER_LIMIT = DIAG_START_REFACTORING   + DIAG_SIZE_REFACTORING
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43578: -ftime-report switch support in Clang

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

We also see an assertion failure prior to the revert. At r329738:

  $ cat test.cpp
  template  struct A {
template  using rebind_alloc = _Other;
  };
  template  struct _Wrap_alloc {
template 
using rebind_alloc = typename A<_Alloc>::template rebind_alloc<_Other>;
template  using rebind = _Wrap_alloc;
  };
  _Wrap_alloc::rebind w;
  
  $ clang++ -c test.cpp
  
  $ clang++ -c test.cpp -ftime-report
  clang-7: llvm/lib/Support/Timer.cpp:133: void llvm::Timer::startTimer(): 
Assertion `!Running && "Cannot start a running timer"' failed.


Repository:
  rL LLVM

https://reviews.llvm.org/D43578



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