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

In D118550#3302487, @mehdi_amini wrote:

(There are visible changes around the name associated with the tracer in lld that someone familiar with this in lld should approve.)

In D118550#3284934, @int3 wrote:

alternative approach

In D118550#3283693, @int3 wrote:

Yeah, I think that's probably a good idea. It took me a while to figure out that initializing the profiler on a per-thread basis was required; it was not at all obvious why my profiler events had started disappearing while I was parallelizing lld. Having it done automatically would've saved me some headache.

Thanks for the patch. When I originally added the multi-threaded support for the time profiler I considered adding the initialize/finish thread into the threadpool itself (llvm/lib/Support/ThreadPool.cpp). In the end I thought it was a bit messy there so just added it into LTO.cpp. With the increased use of multi-threading I think it would be worth reconsidering putting this into ThreadPool.cpp so we don't need to add this everywhere we want to trace multithreaded code. I think that the RAII code you propose will help this fit into ThreadPool.cpp neatly. For all I know there may be other problems with doing that, but I think it will be neater now this is used in more places. What do you think about trying that?

With an additional change from sizeof(buf) to buf.size() this fixes the check-asan and check-ubsan tests.

In D96120#2597877, @kcc wrote:

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

In D97726#2613898, @mstorsjo wrote:

These weren't present with the patch.

I presume you meant weren't present without the patch?

In D97726#2612212, @mstorsjo wrote:

Can someone with a working windows sanitizers try this one out?

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

Update with suggested changes to MinGW.cpp

In D96120#2591418, @vitalybuka wrote:

This is intended as a step to porting scudo standalone.

Why this is needed for scudo stadalone?

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.

Added comment on AllocatorSize.

In D96120#2545151, @aganea wrote:

A few questions: Does this work on x86 targets?

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.

Add documentation to CMake.rst

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

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.

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

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

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

Resigning as reviewer as I also work for Sony.

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

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
  errors.txt51 KBDownload
  .

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.

In D87478#2267034, @cryptoad wrote:

I think that I one point in time I tried thread_local but a compiler (can't remember which one) was complaining about the constructors not being trivial or something to that extent.
This is fairly blurry at this point, so I am sorry if this is not very helpful, but I just want to make sure this would work on all platforms with gcc/clang.

One small comment. Apart from that, LGTM.

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.

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?

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:

Re-upload patch with G LLVM Github Monorepo set.

In D86694#2242150, @russell.gallop wrote:

Is there a good sanity check that it is actually using Scudo rather than silently using the standard alloc?

Fixup scudo (sanitizer based) to work on Windows.

Thanks for catching that.

In D86694#2242150, @russell.gallop wrote:

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

Fix llvm-config test.

In D86694#2242140, @cryptoad wrote:

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

I assume something in lit is parsing and recognizing this? Should remove that too?

In D85199#2241931, @arsenm wrote:

It's always available now. The requires global-isel is a leftover that should be removed

In D85199#2241905, @arsenm wrote:

That's the right fix

Hi Matt,

I'd also like to see how they (currently supported allocators) compare vs. SCUDO and vs. tcmalloc3, if someone is willing to port those on Windows.

I've applied this to the monorepo and made a few more changes to allow it to build LLVM on Windows here: https://reviews.llvm.org/D86694

Thanks for your work on this. I've just tried out this version with rpmalloc (1.4.0) checked out out at the top of llvm-project.

Fixed comments and added test.

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.

In D82379#2153812, @fhahn wrote:

LGTM, thanks for updating the docs!

I guess it would be even better if we would provide the dependencies in a machine-readable way, e..g. https://python-packaging.readthedocs.io/en/latest/dependencies.html

Patches welcome :)

In D71786#2124872, @hans wrote:

I also don't know what exactly the policy around third_party code is, so discussing this on llvm-dev is probably a good idea.

But perhaps one way of side-stepping the whole issue would be to not check in rpmalloc into LLVM, but make the user point at it if they want to use it through a cmake variable, e.g. -DLLVM_USE_RPMALLOC=c:\path\to\rpmalloc (if such a thing is technically practical, I don't know)

I notice that the improvements to rpmalloc for LLVM are still under discussion (https://github.com/mjansson/rpmalloc/issues/150) and targetted at rpmalloc v1.4.1.

In D79552#2073222, @thakis wrote:

The test is failing on Windows again: http://45.33.8.238/win/16808/step_11.txt Please take a look and revert for now if it takes a while to investigate.

In D80442#2061115, @phosek wrote:

This needs be done on a per-target basis, not globally (e.g. you may want to use COMPILER_RT_BUILD_SANITIZERS=OFF for target1 but not target2). Have you tried setting RUNTIMES_${target}_COMPILER_RT_BUILD_SANITIZERS=OFF? This should be already supported, see for example https://github.com/llvm/llvm-project/blob/master/clang/cmake/caches/Fuchsia-stage2.cmake#L100

Hi Sam,

Removed excess dashes in man page. Checked with:

Change to double dashes.

In D79484#2033645, @thakis wrote:

This breaks tests on Windows: http://45.33.8.238/win/15149/step_11.txt

Please take a look and revert if it takes a while to investigate.

In D71060#1871814, @russell.gallop wrote:

In D71060#1867085, @dmajor wrote:

Should this go into the docs / man page / release notes / etc.?

Probably. I'll take a look.

LGTM, with a few small comments.

LGTM, with assertion messages adjusted.

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

In D76695#1939266, @chill wrote:

FIxed elsewhere https://reviews.llvm.org/rG10bd8422d041e2964c146a38b5514d9d92fc458d

LGTM, thanks.