In D52266#1240304, @hans wrote:

Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch.

Since it helps existing msbuild configs, adding this seems like a good thing to me.

FWIW the recommendation against /Ox in my version is because of https://github.com/ulfjack/ryu/pull/70#issuecomment-412168459

In D52143#1236151, @zturner wrote:

Not formally in any kind of mailing list discussion

Thanks! Changed the comment; landing.

Thanks!

fix case on variable

The revert helped, see e.g. http://lab.llvm.org:8011/builders/lld-x86_64-darwin13/builds/25686 and http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/23378 (compare to previous build)

I reverted this (and follow-on fix attempts r342154 r342180 r342182 r342193) in r342336 in an attempt to heal the lld bots.

Ah, looks like all changes are mentioned at the top, just not as comments further down.

https://reviews.llvm.org/rLLD342334 too. Thanks!

Also https://reviews.llvm.org/rL342331 and https://reviews.llvm.org/rLLD342332

Thanks! Submitting...

In D51956#1235313, @llvm-commits wrote:

The symptoms of a collision are just going to be you can’t debug the
program, so not very severe imo, especially since it would almost certainly
be resolved on the next incremental build

Thanks!

ruiu comments

improve comment

r342248, thanks!

rebase, minor cleanups

In D51561#1234862, @zturner wrote:

All of the lit tests have this restriction, is this something you would do
with unit tests but not lit tests?

Unrelatedly, another problem with the current design is that llvm.srcdir.txt must contain an absolute path, which makes it impossible to build on one machine and then copy artifacts and test inputs to another machine and run tests on another machine. DebugInfoPDBTests is the only test in all llvm, lld, clang tests that has this restriction. Requiring some defined fixed pwd for unit tests with inputs would address this issue as well. (As would finding a way to not require reading a file off disk for this test.)

In D51561#1227163, @thakis wrote:

In D51561#1226583, @zturner wrote:

These aren't demangle tests fwiw. But that just complicates the cmake
logic, and I don't see the benefit of doing so. Writing 3k of data to the
disk is hardly going to be a noticeable part of the configure step.

The benefits are:

1. Unit tests reading inputs are bad form; doing that should require an explicit opt-in.
2. While writing 3k of data to disk isn't causing measurable perf issues, writing a text file next to every test binary just because one test binary needs it is still plain ugly.
   
   And not doing it is super easy to do. I don't understand the pushback here.

Actually, this replaces https://reviews.llvm.org/D51887

This replaces https://reviews.llvm.org/D51957.

The situation on Windows is like on Linux. I built chrome.dll with our pinned lld (which was compiled with clang), with a locally-built lld (built by msvc 2017), and then with this patch and the other two patches linked from here. Min-of-5 times are comparable (full data in parens):

In D51887#1232119, @zturner wrote:

In D51887#1231332, @thakis wrote:

Did some measurements on my linux box (didn't have access to my win box). There, just naively calling xxHash64 on the 1.4GB chrome.dll.pdb file after creating it, doing the parallel xxHas64 computation, this patch, and the old nondeterministic guid code all take about the same amount of time (around 32s, min-of-4 links with each approach is within 0.2s of that). I'll try this on Windows tomorrow.

(Naive: https://reviews.llvm.org/D51956 Parallel: https://reviews.llvm.org/D51957)

I'm a little surprised it's this fast. Are you sure it's even doing anything? What kind of hard drive are you using? A SATA III bus interface, which is still probably the single most common interface used by SSDs, has a theoretical maximum transfer rate of 600MB/s, so you'd be adding at least 2.5 seconds to the link, and that's under optimal circumstances. A SATA II interface, which is also still common enough, is 300MB/s. nVME and PCIe interfaces can reach gigabytes / second but we definitely shoudln't be basing measurements off of those. I don't really have a super strong opinion, but it's something to think about.

(Forcing an email to be sent since I accidentally clicked "save" before adding llvm-commits)

Did some measurements on my linux box (didn't have access to my win box). There, just naively calling xxHash64 on the 1.4GB chrome.dll.pdb file after creating it, doing the parallel xxHas64 computation, this patch, and the old nondeterministic guid code all take about the same amount of time (around 32s, min-of-4 links with each approach is within 0.2s of that). I'll try this on Windows tomorrow.

tests pass

In D51887#1230485, @thakis wrote:

(I don't understand yet how /pdbaltpath: makes it into the hash)

(Landed r341945 to address the 2 differing bytes that caused the guid for rsds.test to be different. It's still different due to different /pdbaltpath: flags, but the pdb contents are now identical except for guid and timestamp. I don't understand yet how /pdbaltpath: makes it into the hash, but for the same /pdbaltpath: it seems to be deterministic now. Looking more…)

The new test fails like so on my mac laptop:

In D51887#1229530, @ruiu wrote:

I wonder if you can just compute a hash for a mmap'ed PDB file instead of defining a new type of Stream. That's what we do for ELF when computing a build-id, and I found that's easy to do. It is also easy to parallelize hash computation by making it a tree hash.

In D51561#1226583, @zturner wrote:

These aren't demangle tests fwiw. But that just complicates the cmake
logic, and I don't see the benefit of doing so. Writing 3k of data to the
disk is hardly going to be a noticeable part of the configure step.

In D51561#1226388, @zturner wrote:

Doesn’t mean it’s a good or bad pattern, just that “only 1 file will use
it” is already false, because we can make lldb use it as a followup and
that will get many more.

I did think about alternative solutions, If this CL wasn’t here, the
alternatives i came up would be to either not write this test, or spend
several days/weeks writing a tool whose only purpose was to make it
possible to write this test.

We may call these "unit" tests, but they're really gtests, and developers everywhere use gtest for more than just unit tests. We already have tests that create temp files. I don't see why reaching back into the source directory for inputs is too heavyweight for us.

Also also, opening a file from a unit test kind of makes it not a unit test, since those are supposed to not hit the disk etc. Maybe that test wants to be its own binary instead that's called from a lit script, instead of being a unit test?

Sorry, didn't see this earlier. Writing a llvm.srcdir.txt next to every single unit test binary seems like a pretty roundabout way of doing this. How about instead passing the src dir in a runtime flag in http://llvm-cs.pcc.me.uk/utils/lit/lit/formats/googletest.py#107 if the lit config asks for it? Then you don't need to touch the disk to get the src dir path, it's not passed to _all_ binaries, and cmake doesn't have to write a file to disk for every test binary.

What's the status here? Did this land?

In D51652#1224017, @pcc wrote:

I wasn't imagining that we'd write 32 bits of hash to the repro directory but rather the 64-bit xxhash that we currently truncate to 32 bits to get it to fit into the timestamp field. Without that, isn't there a high probability of collisions after 65536 files (which could be small depending on the project)?

Quoting from the PE spec (see bug for more): "The raw data of this debug entry may be empty". So I don't think it was a bug. Maybe tools had problems with the zero-sized entries, but they didn't bother to update the spec. So I'd suggest to go with this and only add more code if needed (?). We could add a size and just add the 4-byte hash to TimeDateStamps to fill it in at the end, but while it's easy to do it feels a bit cargo-culty.

MSVC on my laptop has the same behavior as on my desktop:

pcc: what if you do cl /Zi /Brepro foo.c?

The test fails on my system like so:

r341313, thanks! I had svn mvd the files. Diffs for mv'd files look a bit weird; phab is probably doing the best it can do here.

We don't match gcc's -Wextra behvior. We generally try to not put a ton of stuff in Wextra that isn't in -Wall. (We also generally don't put a lot of stuff in -Wall that isn't enabled by default.) So I don't think we want this.

r341130 (clang-tools-extra), r341132 (clang), r341134 (lld), and r341135 (llvm). Thanks!

In D51174#1211835, @zturner wrote:

What if we produced a .lib file that had only the minimal set of object
files needed to link this, and make MicrosoftDemangle.h not include other
llvm headers, then you could just link against that object file and it
should be effectively independent.

Would that work?

FWIW I'd like to use the new demangler in demumble [1], but if I need to pull in most of Support of it then I probably won't do that. I agree this shouldn't impact LLVM much (or at all), but having stand-alone-ish demangler code is probably nice for several other projects too. So since the itanium demangler is still in the other place and likely will stay there, maybe it's nice to keep the ms demangler next to it. (But as I said, don't weigh this feedback heavily.)

r340498, thanks!

r340487, thanks!

r340482, thanks!

r340481, thanks!

Thanks, that's a good suggestion. I put the new function in Common/Args.h; I'm not sure if that's the best place.

r340420, thanks!

r340348, thanks!

Add tests.

In D50979#1206282, @thakis wrote:

In D50979#1206211, @rsmith wrote:

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Thanks for pointing this out, good to hear that this even happens to find a bug :-) I'll add some tests to document the progression.

In D50979#1206211, @rsmith wrote:

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Can you explicitly mention that this intentionally doesn't use an absolute path in MicrosoftMangleContextImpl() or similar?

r340048, thanks!

Forgot to add cfe-commits :-( Doing that now.