From what I understand, the comdat group isn't necessary, so I disabled it on COFF in r372182. Can we simplify ELF to match?

• simplify merging check

@hans has been trying to build clang with PGO on Windows, so I'll defer this to him.

Hm, this broke check-asan on Windows, which exercises some code coverage features:
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/51563

• move test

Taking this to move the test around and try the other version...

In D67579#1670170, @xur wrote:

LGTM. Thanks for the fix!

I wasn't sure why profd was set to internal when looking at PR41380 (as for ELF, all are linkonce_oda). But your description explained that.

BTW, the intrinsic lowering happens before inline (I suppose you are talking the main IPA inline).

In D66843#1669728, @kristina wrote:

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

In D67401#1666357, @broadwaylamb wrote:

In D67401#1665548, @rnk wrote:

lgtm

I don't have commit access, can I ask you to commit this for me? Or is there anything else that needs to be done?

In D66843#1666367, @echristo wrote:

Still really not a fan of this way. I think you can change the incoming DL in compatibility cases to add an address space when none exists on x86.

-eric

lgtm

In D47956#1138555, @rnk wrote:

In D47956#1138521, @rsmith wrote:

Can we now remove the corresponding MSVC-specific hacks elsewhere (eg, ASTContext::isMSStaticDataMemberInlineDefinition), or do we still need those for const-but-not-constexpr static data members?

We should be able to do that, but unfortunately it drastically changes the diagnostics we emit, as you can see from the tortured ifdefs in my test case updates. I gave up before attempting it.

• rebase

In D67304#1664696, @thakis wrote:

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

In D67301#1664700, @MaskRay wrote:

I hope at least for ELF, you can keep the "_Z" prefix check. It doesn't need the ObjC "___Z" extension.

I managed to run into this again:
https://reviews.llvm.org/rL371478#687359

Hm, see rL371513. I'm not sure what the correct fix is.

I see that llvm/.gitattributes already exists, and I assumed that was enough, but I see that it is not. lgtm

In D67301#1662223, @mstorsjo wrote:

In D67301#1661748, @rnk wrote:

Should the _ prefix (for itanium symbols in i386 mode) be stripped by the COFF wrapper function, or by the common demangle function (matching _Z or __Z)?

Maybe, I forget if the extra _ comes before the __imp_ prefix or after it when dllimported.

It's after the __imp_ prefix, so the demangler could handle it.

But there's another consideration, which is that this code could hypothetically be shared with a MachO linker implementation, where the extra underscore prefix is also present. IMO it's best for any generic demangler to match any number of underscores followed by Z.

I'm not entirely decided on whether I think this is a good thing or not.

The caller knows with certainty whether there should be an extra underscore prefix or not, as it is well defined (always in MachO, never in ELF, plus on i386 in COFF). But if we make the demangler implicitly handle both cases, it can also accidentally e.g. try to demangle any symbol which just starts with a capital Z, without any real underscore prefix, on i386 in COFF or in MachO. (I realize the current code in LLD I just committed also has this issue though.) So if we make it a requirement for the caller to compensate for the prefix, it is less convenient, but also less ambiguous.

WDYT?

lgtm

Thanks!

Sorry this got lost for two weeks. :(

Ptal, new patch

In D67304#1663042, @hans wrote:

Should there be a test exercising this part? The updated tests both have -fms-compatibility, so were already just warning.

• rewrite, abandon unification

Looks right to me, but let's wait for @inlgorion to take a look on Monday.

Hit send too soon...

Should the _ prefix (for itanium symbols in i386 mode) be stripped by the COFF wrapper function, or by the common demangle function (matching _Z or __Z)?

Here is some feedback, I apologize for dragging my feet.

lgtm, it's in the LLVM_ namespace, so it couldn't hurt to record it.

• emit an error if we try to clone without a definition

In D67028#1659881, @efriedma wrote:

In your test case, we hit the early return that I linked to, so we don't try to clone, and we don't need to emit an error.

Yes, that's what happens with the Itanium ABI; what happens with the MS ABI?

• merge into thunks.cpp

I think what I said applies to your test case. Basically, in the Itanium C++ ABI, virtual method definitions provide all their thunks as weak_odr. We only emit thunks referenced by vtables as an optimization, and they are marked available_externally. In your test case, we hit the early return that I linked to, so we don't try to clone, and we don't need to emit an error.

In D67028#1653439, @efriedma wrote:

Do we have test coverage for a variadic, covariant thunk for a function without a definition? I don't think there's any way for us to actually emit that, but we should make sure the error message is right.