In D52283#1239983, @zturner wrote:

It’s true that nested containers don’t usually perform well, but in this
case the outer vector *never* resizes. So it seems like it should be ok?

I ran the tests with your patch and everything passes, but of course, Timur mentioned that nothing in our test suite finds this issue. He probably encountered it in Chrome and it was hard to reduce into something we could add to the test suite.

lgtm

In D51700#1236413, @metzman wrote:

Should I remove FuzzerExtFunctionsDlsymWin.cpp?
I'm unsure about doing so because in Marcos's patch he didn't remove FuzzerExtFunctionsWeakAlias.cpp and because I may find how use it properly later, in which case removing it might make the history uglier?

lgtm

Let's land this, I want to try it out. :)

• Fix one last use of setValueSubclassData

• Use private bitfield for subclass data
• Change cached order invalidation
• Add tests for removal and erasure

This is pretty cool. The process launching APIs in LLVM were pretty basic, left a lot to be desired, returned ints, etc etc. This addresses a lot of that.

Thanks, I think this looks good with a minor tweak to the test.

lgtm, thanks!

Needs tests, but I'm on board with this and the implementation.

I think we need to back up and think about what we're doing where and why.

Thanks! It's very likely we can use getenv("USERPROFILE") or the unicode equivalent to eliminate the remaining one. It's also pretty reasonable to just hardcode %USERPROFILE%/AppData/Local if we ever need this functionality.

lgtm

@gbiv suggested that I test this on a large codebase, so I went ahead and built the Chrome unit_tests target with this, and the validation checks passed.

Right, sorry about that. :)

lgtm

lgtm

Any opinions on this? I'm eager to get it in so I can release it and get some faster builds, but I know it's a core data structure change.

I'm just waiting to hear from other sanitizer maintainers about their stance on adding the complexity of supporting mingw headers in the sanitizers.

Needs tests? Maybe they just aren't in the diff?

We should add old FPO data dumping next.

• use less ambiguous name
• pick a default CPU type for simplicity

Unfortunately, it looks like this changed the behavior of newlines embedded in arguments. When parsing response files, we want to treat \n as whitespace. When parsing a command line, CommandLineToArgvW apparently does not. The documentation says it separates arguments on "whitespace characters", but it doesn't say what it considers to be whitespace. Apparently \n is not whitespace.

lgtm

• Be more string-y

In D51941#1230893, @zturner wrote:

When you commit this, make sure to include in the commit message how much speedup you're seeing as a result of this.

lgtm

In D51746#1229744, @zturner wrote:

I don't really know how any of this stuff works, but do we need the LLVM_ENABLE_PDB=ON?

I like the code, but I think this needs more tests, and better testing infrastructure before we can commit it. I would recommend beefing up COFFDumper::printUnwindInfo in llvm-readobj to print the ARM64 unwind opcodes in a human readable format, and then you won't need to write tests with hexadecimal CHECKs like you have now.

Sounds good, it could use an IR test though.

ಠ_ಠ mm_malloc.h, you are a source of much complexity.

I should add more reviewers, since adding a mingw port is not a small matter. I *think* that some of the sanitizer_common code already builds with mingw gcc for tsan-go, right? So, adding support for building with gcc on Windows should not be too onerous for the project as a whole. At least, that's what I think. Other folks can chime in.

In D51887#1229702, @ruiu wrote:

Sequential file access might be fast enough, so I don't know which is better performance-wise. In such situation, I'd probably try to implement an easier one (hashing a mmap'ed buffer) to see if it is satisfactory, or try to implement both to compare if not, to keep it as simple as possible.

lgtm

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.

Thoughts?

Actually +@zturner this time.

Does it build with GCC, clang, or both?

Thanks!

lgtm with a tweaked comment. Thanks!

In D51695#1228444, @kristina wrote:

This is a testcase for the crash, patch fixes it, though I'll amend it to work on MCStreamer level as per what @rnk suggested. I'm going to sound really stupid asking but where should the testcase be, is it a FileCheck based one? I just made two targets, one with pre-patch toolchain and one with current (my working tree) toolchain which handles this just fine. First one crashes, second one doesn't, not sure where tests like that go.

• beef up test

Passing the source location through to MCStreamer seems like a good idea. Before rL315264, this code used to use report_fatal_error. My goal was to make it less crummy, and make sure errors get diagnosed when emitting assembly or emitting object files.

I see that we've done this before in rL310419. This was originally added in rL275180. It'd be nice to figure out what tools generate this nop so we can enumerate all the patterns, but for now let's just do this.

I looked at the documentation for the cmake install command, but I still have no idea what a component is or what effect this option has. Can you elaborate on that?

lgtm, thanks! I thought that was going to be more of a pain. :)

lgtm

Wow, three different ways to set the linker. I'm impressed with how many cooks there are in our kitchen.

I'd suggest fixing this in MCStreamer::EmitCFIStartProc by having it recover in some way that doesn't lead to future assertions. Also, it should be possible to write a standalone .s test case for this.

In D51749#1226398, @aprantl wrote:

Because of LiveDebugValues this patch might still work though.

Does GlobalISel support the MMI side table? Otherwise this seems a fine approach.

BTW, it's possible that this is also what's causing our size regression vs. non-LTO builds. If archives add lazy symbols which are marked as being used from a regular object, that might prevent us from discarding them.

lgtm, thanks!