lgtm

I see. Maybe ASLR matters? I have the full stack trace if it helps:
https://reviews.llvm.org/P8253

• fix misleading comment

CReduce on plain C code is lightning fast, no template goo grind away. I got this reduced test case that crashes when this patch is applied:

I reverted this, it caused crashes while compiling harfbuzz in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1167305

In D94639#2497031, @dblaikie wrote:

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

In rG1a92de0064bcf9ef605ca9456812adf411e9ee4e#970263, @ldionne wrote:

Before that commit, under debug mode, we'd disable _LIBCPP_EXTERN_TEMPLATE, but not _LIBCPP_EXTERN_TEMPLATE2. After the commit, we disable both (which is your issue).

Thanks, I think this will solve the issue.

lgtm

Thanks, this seems like the right spot.

I don't see how supporting this attribute would break interop with MSVC. MSVC doesn't support these attributes, only GCC and Clang do. Presumably in the wine environment, some headers using these _GNUC extensions are being included somewhere, and IMO we should accept the attributes and honor them.

In D94639#2496969, @akhuang wrote:

In D94639#2496950, @dblaikie wrote:

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

This actually caused https://crbug.com/1166386. The explanation is that defining _LIBCPP_DEBUG causes _LIBCPP_EXTERN_TEMPLATE to be defined to nothing, which disables the extern declarations. Chrome uses _LIBCPP_DEBUG in debug builds. IIUC, locale needs these extern template declarations, because locale uses static data members with hidden visibility (need to confirm). If you remove the extern template declarations, then duplicate static data members may be emitted into the user's DSO, and when they use locale facets, they will be registered with a new id. I need to dig more to confirm, but this is what I'm going on.

Please add a lit test for this. The test could actually copy llvm-symbolizer into a temporary directory, and you could embed the option in __asan_default_options similar to what we want to do in Chromium. I think it's also reasonable to test that if llvm-symbolizer doesn't exist, we get a warning, not an error.

lgtm

Thanks, I'll push this after a few more tests.

lgtm, thanks!

In D94267#2491643, @aganea wrote:

This is mostly caused by contention when page faulting on either mmap'ed files or zero-pages. KeYieldProcessorEx is spinning while waiting for a lock for bringing pages into the 'working set'.

lgtm, thanks

lgtm

We are also observing this build breakage, so I'm going to go ahead and push this now.

In D94154#2487891, @tmsriram wrote:

In D94154#2487762, @dblaikie wrote:

@rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).

We are going in circles. For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null. When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable. I don't see any issues.

I put some comments on https://bugs.llvm.org/show_bug.cgi?id=48432#c4 about this. I'm not sure we should keep going this direction. Thanks for getting numbers, though.

Please fix the two test failures observed in the harbormaster output. Your change will cause these tools to produce CRLF output, which tests must account for.

In D93668#2485719, @mcgrathr wrote:

It's fine to have a target tuple translate to a default C++ ABI.
But the C++ ABI selection is fundamentally not a target flavor thing. It's just a C++ ABI thing.
So using the target tuple as the sole mechanism to determine C++ ABI is fundamentally wrong.

lgtm with nit

lgtm

• formatting

lgtm

lgtm

I guess a triple of -fuchsia-itanium would be a reasonable way of expressing this.

Generally makes sense, but I had a concern.

lgtm

In D94154#2481491, @dblaikie wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

In D94118#2481099, @pengfei wrote:

I saw the document mentions ESP/RSP should be preserve for regcall. But I cannot think out a situation that ESP/RSP need to preserve either.

• Remove RSP from CSR lists too

Reposting my comment here, since this is where the discussion was:

I did some comparisons of the sanitizer object files, and I think this change broke -gline-tables-only functionality. The new object file had a large .debug_loc section, which is only present when full debug info is enabled, and is not desired in this case.

The Harbormaster precommit test stuff is complaining, but maybe it's just because of the binary files in the patch.

It seems like Harbormaster / Jenkins didn't like the new test, so double check that it passes before landing.

lgtm

I'm not an SCC expert, but Arthur explained to me how this works, and I trust the explanation.

lgtm

lgtm

Thank for the update, apologies for not providing these suggestions the first time.

lgtm

lgtm

lgtm, the comment is a suggestion

In D91659#2458639, @rsmith wrote:

@rnk Your thoughts on this would be appreciated.

lgtm

lgtm

In D92509#2456446, @dexonsmith wrote:

I missed this email yesterday, but your solution SGTM.

It looks like you did not revert 65c5c9f92ec514ae41c8ea407d1c885737d699ec (ADT: Rely on std::aligned_union_t for math in AlignedCharArrayUnion, NFC), which changed it to rely on the math in std::aligned_union_t:

template <typename T, typename... Ts> struct AlignedCharArrayUnion {
  using AlignedUnion = std::aligned_union_t<1, T, Ts...>;
  alignas(alignof(AlignedUnion)) char buffer[sizeof(AlignedUnion)];
};

Should that be reverted too, or if not, why does alignof(std::aligned_union_t<...>) give us the right answer?

Thanks for the fix. At first, misunderstood, I expected that an aggregate containing non-aggregates should be returned indirectly, and that the fix would be in the C++ ABI codepath. However, I see that is not the case. An aggregate may contain non-aggregates, and MSVC will in fact return such a type directly in X0/X1.