Looks good to me with a minor comment fix. Let's wait for @efriedma to approve the new name before we commit to it, though.

Non gmock changes look good.

In D56516#1358544, @morehouse wrote:

In my local tests, the weak function and its sancov metadata both get discarded with bfd, gold, and lld.

In D51664#1357417, @lattner wrote:

After giving you a hard time about this a few months ago, I've come around to believing that this is the right thing to do. Certain classes of algorithms really do benefit from having a lexicographic ordering comparison that is fast, and I think that this general approach is the best way to go.

In D53541#1358310, @mgrang wrote:

@rnk Thanks a lot for the clarification. Yes, I see o.x: 0 instead of 42. Supporting this case would mean implementing recoverfp as well as support generating the correct parent frame offset for arm64 windows. Do you think this can be done in a follow-up patch? So this patch and D53540 would add the basic support for SEH and we can go fix corner/more complex cases in follow-up patches. There are also several more comprehensive test cases in https://github.com/Microsoft/windows_seh_tests which we plan to address next.

In D55293#1346895, @aganea wrote:

Hi @ruiu ! This is waiting for your final review / approval. Could you possibly take a quick look please? Thank you!

In D56516#1353365, @morehouse wrote:

In D56516#1353359, @bd1976llvm wrote:

• For ELF targets, this fix is functionally correct, but it will have the effect of retaining the section content for the weak symbols in the output, potentially slowing the link and making the output larger (unless you use -gc-sections).

Yes, but this is definitely preferred over breaking weak/strong semantics for our users.

I can't compile the example you gave yet because I haven't applied your patches locally, but this is the "three stack pointer" case that I have in mind:

struct Foo {
  void (*ptr)();
  int x, y, z;
};

In D55781#1351541, @rjmccall wrote:

Okay. In my opinion, then, we should just do this unconditionally.

I have no idea how you ended up here, but looks good, just a few NFC comments. :)

Functionality seems reasonable, I'll let @aaron.ballman finish the review.

lgtm

In D55585#1354878, @aganea wrote:

To the light of all this, does it still makes sense to compute and emit GHASH streams in clang? I'm pretty sure that the cost for serialization and I/O for those streams would be much higher that just computing GHASHes on-the-fly in the LLD. The only marginal benefit would be for incrementally linking, however even that is debatable.

• rebase

In D53541#1353251, @mgrang wrote:

What about the three stack pointer case of stack realignment plus a dynamic alloca? Typically this is the case where recoverfp is necessary.

@rnk Sorry, I missed your comment earlier. I am not sure what scenario the three stack pointer case is. Could you please give me a test case for it?

lgtm

Thanks for the report and patch! Looks good. Do you need someone to commit this?

Thanks for pushing the MC work to support this forward!

Thanks! I'm surprised we passed as much of the Microsoft exception tests as we did with this bug. Maybe it regressed.

lgtm

rebase

@phil-opp, sorry for the breakage, I understand your frustration. From my perspective, the x86 interrupt calling convention was hacked into the x86 backed in a way that doesn't play nice with orthogonal features, like the copy elision I added. Your fix proliferates the wrong direction of the existing design of the interrupt convention. Yes, it fixes the problem, but if I don't push back, require a test, contact Intel, the original authors of this feature, and pressure them to do it in a more proper way, LLVM will continue to become more unmaintanable.

lgtm

Still needs tests, right?

lgtm

lgtm!

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

Here is the original test case from the PR:

#include <stdio.h>
int offset = 0;
static void foo2() { printf("foo2 is called\n"); }
void foo() {
  __asm__ volatile("movq %0,%%gs:(%1)" : : "ir"((void *)&foo2), "r"(offset));
  printf("foo is called\n");
}

If you compile with GCC, you will see that it generates the same assembly that LLVM does:

foo:
.LFB15:
        .cfi_startproc
        subq    $8, %rsp
        .cfi_def_cfa_offset 16
        movl    offset(%rip), %eax
#APP
# 21 "t.c" 1
        movq $foo2,%gs:(%eax)
# 0 "" 2
#NO_APP
        leaq    .LC1(%rip), %rdi
        call    puts@PLT
        addq    $8, %rsp
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc

It looks like this was actually intentionally removed back in rL107727 to fix https://llvm.org/pr4752. This change breaks the test that was added for it, llvm/test/CodeGen/X86/2010-07-06-asm-RIP.ll. I guess, since this is GCC inline asm, the question is, how does GCC handle the 'i' constraint? I suspect that this new behavior may be closer to GCC, but it'll require more analysis.

In D54667#1341110, @andrewrk wrote:

In Zig, a struct acts as a namespace full of static functions, so what you said works perfectly.

I'm also open for suggestions if there was a better way to represent this in LLVM's debug info.

This shouldn't crash anymore after rL350073.

We can change the code to avoid the crash, but what debug info did you want clang to emit? It seems like the best way to describe StructWithNoFields_add is as a static method of the class StructWithNoFields, since it doesn't appear to have a 'this' parameter.

lgtm, thanks! Do you need someone to commit this?

Want to stamp this? It's 4pm on the Friday before Christmas, what could go wrong? :)

One more thing I just realized that was probably obvious to you: Computing ghash in parallel completely eliminates the need for the /debug:ghash flag, and we can eliminate the non-ghash codepath. That's great.

Neat! I didn't have a chance to review with a fine toothed comb, but I like the idea, and here's how I analyze it. We've already talked about how merging a type record without ghash is O(size of type), and with a content hash, you can make that O(size of hash), i.e. look at 8 or 20 bytes instead of up to 64K. But, computing the hash requires reading the type record data, so we made the compiler do it and put it in .debug$H.
If one can't do that because MSVC is being used, parallelizing the hash lookup makes it so that the linker only takes O(size of all types / n cores) wall time to do it, and then it does O(# types) (not size of types) work to deduplicate them.

lgtm

• add positive test for dynamic allocas with intervening call

My experience so far with inalloca has been that, no, those passes are not safe. This is only the latest in a long line of bugs in LLVM around handling dynamic allocas. I think conflating static and dynamic allocas was a mistake, and we would've been better served with a separate construct for allocating stack memory for local variables, but at this stage, we'd be better off standardizing on the terminology of "alloca" for standard locals and pushing dynamic allocas into intrinsic instructions.

In D55900#1336824, @mstorsjo wrote:

However, COFF actually does have some support for this directive, and if you emit an object file, llvm-mc does not assert.

I'm not sure I'm following here - if you emit a COFF object file, from what source? From normal LLVM IR CodeGen, or from MC assembly with a .file directive?

• move to IgnoreParens

Looks good, thanks.

lgtm, thanks!

Go for it.

lgtm

lgtm I think it's worth making this work incrementally like this.

In D55293#1333805, @ruiu wrote:

Is .CRT$XCU just an example, or do you want to do something special for .CRT$XCU? If latter is the case, have you considered just directly implementing what you want do?

@ruiu, the motivation is to have some abstraction over the collection of chunks before "merging" section names. So, this would allow us to enumerate all chunks contributing to the .CRT$XCU section before it is concatenated with all the other .CRT$* sections and then merged into the .rdata section by our builtin /merge: rule. We accumulate those sections in a plain vector currently, and I felt it would be useful to have some name for that grouping.

Oops, too many Zacharies. I mistook you for him (only in this patch, not the other one). He has more ownership over this code, so I will let him approve it.

Regarding the alignment optimization, I implemented it, and I only measured a 3% speedup when making clang.pdb (without ghash, I think, since I was lazy and didn't rebuild...). However, I put in stats to show that it saves 433MB of memory allocations, and that's a significant fraction of the overall process memory usage, so it's definitely worth it. I'll post a patch with harder numbers soon.