In D87805#2283132, @rnk wrote:

Truly, I think I haven't done enough validation. But, the resulting PDB should actually be identical: it should contain the same type records, in the same order as before.

I suppose a simple way could be to temporarily retain the old algorithm and have them both run side-by-side and assert if things are different.

Thanks a lot for working on this Reid, really neat!

Looks great, thanks!

Just a few more things and we should be good to go:

Please install & run git clang-format before uploading the patch.

Side-note: @dmantipov Could you please use git diff -U99999 when creating patches? This allows reviewers to quickly browse through other parts of the file(s) in Phabricator. That would fix the "Context not available." message.

In D86694#2277456, @cryptoad wrote:

I didn't try to make the Exclusive version work, mostly because I was using the Windows TLS API and the Shared fit right in with those, but it would get rid of a lot of the contention.

That sounds good. Right now, the delta between rpmalloc/mimalloc/snmalloc is less than 5 sec (for a run of 2 min 15 sec), I would assume with the Exclusive mode, SCUDO would fall in the same ballpark. Any other allocator that I've tried that does some level of locking (Hoard, ptmalloc, tcmalloc2) does not scale (is much slower) on many-cores machines.

In D86694#2274682, @cryptoad wrote:

In D86694#2274548, @russell.gallop wrote:

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

You can reduce the size of the Primary for Windows with an additional define in the platform file. You probably want to make sure there is at least a few gigs per region (eg: the total size could be 256G).

Once again the memory is reserved but not committed, and this is on a per-process basis. There shouldn't be more than one Primary, and as such we shouldn't run out of VA space. We could possibly run out of memory if we allocate past the amount of RAM (+swap), but this is the committed amount.

I like a lot the change, it makes the API and the ownership more clear. But I am wondering: why not simply remove the CVIndexMap type? Do we need this extra indirection now? We could simply use a TpiSource API, instead of doing the if (indexMap->isTypeServerMap) in PDB.cpp? e.g. class TpiSource { ... virtual ArrayRef<TypeIndex> indexMap()/typeMap(); }.

@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?

==89136==ERROR: Scudo failed to allocate 0x10000 (65536) bytes of memory at address 0x4c0090e00 (error code: 1455)
==89136==Dumping process modules:
ERROR: Failed to mmap

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?

Hello @david2050! Are you still planning on working on this? If no, can I take over the patch and land it?

LGTM.

Thanks for working on this @russell.gallop!

@russell.gallop Do you think you can upload the patch again by setting the Repository to "Monorepo"?

I've digged a bit more. The issue is that the ICF improperly folds .text$x sections (catch funclets) but not always the "associated" .pdata and .xdata records, in part because those additional sections are attached to .text$mn not to .text$x. This occurs only with MSVC-cl built OBJs, Clang does not create separate .text$x sections for catch functlets (rather, they are merged into the function .text). I'm seeing the bug when linking with the FBX SDK 2016.1 VS2015 (lib\vs2015\x64\release\libfbxsdk-mt.lib).

Please take a look. See https://reviews.llvm.org/D71786#2265789 for details.

Tested, works for me, thanks!

@MaskRay Thank you!

Ping! @MaskRay any further comments?

@xgupta Thanks for the report! Please see rG9026d3b2f942db81b0d8588fd0990d389669dc55

@hans Fixed in rGb9b954b8bbf0feed1aecde78cb6976134e460e91. Since the x86 versions of the LLVM tools are limited in terms of memory, I think it's not worth supporting LLVM_INTEGRATED_CRT_ALLOC on x86.

In D71786#2193070, @hans wrote:

From my point of view, I think it would be fine to only support rpmalloc in this patch. One aspect that Russell touched on is that if we support a bunch of options, it's not clear how well that support will be tested. Supporting only rpmalloc would also simplify the patch a little, and small incremental change is always good I think.

Address build issues.
Document the new flag.
Add more validations in cmake.

No worries!

I've pushed one more fix: rG33ce275fc156c8b015acfad918937028b2cc235c

Address comments.
Added a CrashRecoveryTest.

Rebase.
Address @MaskRay's suggestions.
Made safeLldLink a public API and made it possible to provide user-defined stdout/stderr streams.

In D86496#2236188, @thakis wrote:

In D86496#2235124, @aganea wrote:

@thakis Do you think you can add printenv.exe into https://commondatastorage.googleapis.com/chromium-browser-clang/tools/gnuwin-XX.zip so we can enable the test again on Windows? Please see https://reviews.llvm.org/D86170#inline-799301

We have env as a lit builtin. Can we just use that? What's the not --crash supposed to do here? Why would env crash anyway? I miss context on what this test wants to do.

@thakis Do you think you can add printenv.exe into https://commondatastorage.googleapis.com/chromium-browser-clang/tools/gnuwin-XX.zip so we can enable the test again on Windows? Please see https://reviews.llvm.org/D86170#inline-799301

In D86496#2235112, @MaskRay wrote:

In D86496#2235103, @thakis wrote:

This breaks check-llvm on windows: http://45.33.8.238/win/22625/step_11.txt

Please take a look and revert for now if it takes a while to fix.

In D86496#2235104, @thakis wrote:

(I'm guessing it was failing under cmd previously and fails in msys/gig bash now?)

It worked in a GnuWin32 environment previously but a Chromium (which did not install full GnuWin32) bot was unhappy. So I created this patch. Glad that Alexandre will handle it. Thanks!
Looks like GnuWin32 'env' is not conformant...

I'll fix it.

@MaskRay Thanks will do - I still have one last issue I don't understand, maybe you can help?

In D70378#2234356, @mstorsjo wrote:

Just fwiw, a smaller patch on the same topic was just committed a few days ago, from D86401. This looks like a bigger and more complete fix for the same issue - but before completing it, pay attention to pick up/remove the fix from D86401 (I see that the same vector already gets cleared by this patch as well).

In D86401#2234225, @hans wrote:

Cherry-picked to 11.x as 0c37a9165611880b99b1f9632179864ecb3f2e13.

I'm curious about the background here, though. When would lld::coff::link be called successively? Could we have a test for this?

Thanks, one more minor thing and it should be good to go.

In D86351#2230941, @tschuett wrote:

When you extended the thread pool for high cores counts for Windows, I thought the other way around. I want to you use the distributed thinlto locally. Then each ninja job is single threaded and ninja can schedule everything. This probably won't be a desirable solution for Windows.

This has been suggested by Bruce Dawson here: https://github.com/ninja-build/ninja/issues/1638 - although I haven't seen the prototype he mentions nor any figures. I feel that the direct and indirect costs for context switching between a pool of processes would be higher vs. a pool of threads in a single ongoing process, and that would increase with the number of cores. But I remain open to any idea, as long as it improves build times on Windows.

As suggested.

In D85998#2231001, @zahen wrote:

The build system strives to be deterministic

When you say build system, you mean MSBuild? Other? For example, Fastbuild purposely calls CreateProcess with an empty environment block (not null) to avoid determinism issues. Is that your case? We are also using -nostdinc to avoid any compiler-generated include paths, even in cases where you need to repro something in a VS cmd shell.

Could you please explain a little bit more what motivated this change? You can achieve almost the same with env VCToolsInstallDir=F:\Your_Path\ clang-cl .... You would also want to pass -fmsc-version=... to avoid extracting the version from cl.exe. If you're seeing many file and registry accesses, then clang-cl probably runs without a call to vcvarsall.bat before?

I don't see any measureable improvement in terms of execution time on a 36-core Windows with ninja check-llvm Release with assertions (164 sec both with & without the patch). However there's a measurable saving on a 6-core: 820 sec without patch -> 784 sec with patch, median times after a few runs.

Thanks for the feedback @MaskRay @grimar!

As suggested,

• Set LLD_IN_TEST=2 (still enabled for COFF only)
• Remove stdout & stderr for all iterations but the last one.

In D80833#2221866, @aeubanks wrote:

This seems to be breaking determinism on Windows builds, see https://crbug.com/1117026. Can this be reverted and fixed?

Fix clang-(format|tidy)

Add support for testing LLD as a library, by running the driver several times in a row when LLD_IN_TEST is defined. The value of LLD_IN_TEST tells how many times LLD should loop (3 times right now).

LGTM.

In D85458#2213234, @dibya001 wrote:

As I don't have commit access, could someone please help in merging it?

In D85669#2212552, @jhenderson wrote:

Okay, thanks. Looks like there's more values than are actually documented. Out of interest, what does fsutil behavior disablelastaccess say for you?

D:\llvm-project>fsutil behavior query disablelastaccess
DisableLastAccess = 2  (Géré par le système, désactivé)

Despite NtfsDisableLastAccessUpdate being 0x80000002 in the registry. This link has more information about the meaning of the value: https://dfir.ru/2018/12/08/the-last-access-updates-are-almost-back/

Hi @jhenderson!
I had NtfsDisableLastAccessUpdate = 0x80000002. With this, touch -a -t 199505050555.55 %t.1.o fails to update the "Last access time" to the requested date.
Setting it to 0x80000003 or 3 or 1 works (ie. touch succeeds)
I'm not sure exactly what is going on here, maybe NTFS sets the "Last access time" when the handle is closed, and thus overrides what touch does?
The bottom line is that other users could be in the same situation: the test will succeed or fail depending on the settings on their machine.

Change LGTM, do you have commit access? You should perhaps request it if you're planning on further patches.

Thanks for the update, almost good, see below.
Also, could you please explain what is your practical application for chopping the callstack?

Commited as rGa680ea2c720751a3d724fd8282515924acb3bd32

Code looks good, but I'm still wondering if we could add test coverage? Can we test for PrintStackTrace(StrOS, 1); and validate that only one line is emitted? You could add such a test in one of the files in llvm/unittests/Support/ - or add a new test file if it doesn't fit anywhere. Also don't forget to skip Windows OS (see the Host.cpp test for an example).

In D85669#2207688, @amccarth wrote:

I'm OK with this.

In an ideal world, it would be nice to ensure stripping preserves other the Windows creation timestamp while updating the last modification timestamp.

Hello @dibya001! Thanks for the patch!