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.

In D71060#1867085, @dmajor wrote:

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

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)

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. =/

Additionally it allows Clang versions prior to 10 to compile current trunk.

In D71059#1838862, @Quuxplusone wrote:

I'm not qualified to comment on the functionality. This seems like it'll be OK with respect to thread_local on Mac OSX. No further comments from me.

Thanks, added const.

Add consts to references in lambda functions.

Ping. Okay to land new version?

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!

Update to check for start of metadata.

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

In D72399#1820866, @rnk wrote:

lgtm

I checked that __declspec(thread) and thread_local seem to be functionally equivalent with MSVC.

Fix condition

In D72399#1810145, @dblaikie wrote:

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

In D71786#1812388, @riccibruno wrote:

Am I the only one who is a little bit uncomfortable with including a replacement to the system allocator in LLVM?

At first, I configured rpmalloc in the "unlimited" mode because I wanted maximum performance. I now reverted to default rpmalloc flags, and the memory usage has decreased (albeit still higher than the Windows Heap), with no impact on link time (meaning that timings are still the same as in the Summary). Below are "Max" values when linking clang.exe with ThinLTO: