Users outside of the clang repo may use different googletest versions.

In D117356#3256460, @ychen wrote:

Apologies if I didn't make myself clear. The inlining of the allocator function step is already incorrect. This instcombine bailout is not a desirable direction since it implicitly assumes the earlier allocator inlining is correct but it is not. Whatever attributes would be added in the future should be used for preventing the C++ new/delete from doing any kinds of IPO/IPA (marking them to be deleted is allowed though).

In D117356#3245221, @ychen wrote:

In D117356#3245161, @durin42 wrote:

Ah, @jyknight bailed me out. That's https://github.com/llvm/llvm-project/issues/40384 which looks related, yes. I suspect that would be resolved by this patch.

Agreed that this patch could fix it. However, the underlying issue is that the mismatch is generated by LLVM itself. IMHO preventing the mismatch is a better approach (which D97735 would do, I think we should land that instead. @rsmith (thought?)

Sorry I didn't see to comment on this before, but I'm also skeptical about introducing a new concept of stateful options to this command-line syntax.

Accepting assuming the last comment will be addressed before pushing. Thanks!

This seems to cause a reproducible infinite loop inside the inliner on some inputs. I haven't narrowed down a repro case yet, but I'm going to revert this in the meantime.

LG, with a few minor comments.

It was noted in other discussions that problems have only been reported on the Galaxy S9 and S9+ devices, running older versions of the device firmware (based on Android 8). Although I have no first-hand knowledge, presumably the kernel in the more recent builds has been updated to (correctly) not report HWCAP_ATOMICS as available.

This looks reasonable to me, but I'd like to wait for a conclusion on D115409 first. (Which probably will result in rewriting the commit message, as well).

This looks good to me, but I'd like to wait for a conclusion on D115409 first.

OK, I do think that special case definitely needs to be deleted. It's assuming that the block args are in a particular place in the argument list of the callbr -- the place Clang put them -- and then assigned special behavior based on that location. But I think it shouldn't do so -- they should be able to placed anywhere in the arglist.

It's rather sad that GCC made the quite-unintuitive decision to number the arguments in this way -- LONG AFTER clang had already implemented the other way...

The current design allows you to use blockaddress constants in code outside the asm goto to compute the destination of an asm goto, I think? At least, the IR definition looks like it allows that; not sure how well it works in practice.

This change seems like it's done in the wrong layer -- why do we add a special-case in Clang to emit "={r11},{r11}", instead of fixing LLVM to not break when given the previously-output "={r11},0"?

Correcting the examples to match the style emitted automatically by our tooling is an improvement.

I think this patch was wrong -- we should not be assuming 16-byte alignment for an allocation smaller than 16 bytes.

In D113517#3122217, @rsmith wrote:

In D113517#3121455, @jyknight wrote:

This change allows those future optimizations to apply to throw() as well, in C++17 mode, which is the desirable outcome.

I see. It seems inconsistent that throw(X) would still call unexpected but that throw() would call terminate, but I suppose in the throw() case there is really not much interesting that an unexpected_handler can do other than (take some logging action and) terminate the program -- in particular, it can't do exception translation. So maybe the inconsistency is not a big deal, and it's more important to get the better code generation, especially given how rare throw(X) is compared to throw(). OK, I think I'm convinced that this is the best direction.

In D113517#3120030, @rsmith wrote:

What's the motivation for this change? I believe the current behavior is still conforming: set_unexpected is no longer (officially) part of the standard library (though it still exists as a zombie name), and the default unexpected handler calls terminate, so calling unexpected rather than terminate should have the same effect, except in non-conforming programs that call std::set_unexpected anyway (and for such programs, calling unexpected seems like the behavior the programmer would expect). Do we generate better code if we call terminate rather than unexpected?

The design was already messed up before INLINEASM_BR for exception handling.

In D108243#2995898, @waltl wrote:

Added driver flags, and tests for them

The only two classes that have this behavior are std::string and std::vector. All other classes define their exception-throwing functions in the headers, so they end up calling abort() directly. That looks accidental to me, and not really something we'd want to consider a contract with our users. WDYT?

Ping. Any comment from Apple folks?

In D108389#2955525, @ldionne wrote:

To answer your question more directly, the goal here in terms of behavior is to ensure that a TU using std::vector::at (and friends) under -fno-exceptions doesn't rely on a weak-def symbol located in the dylib -- which it technically doesn't need cause all we do is abort() anyway.

In D106577#2944086, @aaron.ballman wrote:

I don't think that scenario is valid. MBCS-to-unicode mappings are a part of the definition of the MBCS (sometimes officially, sometimes de-facto defined by major vendors), not in the definition of Unicode.

Isn't that scenario basically the one we're in today where the compiler is unaware of what mappings the library provides?

I've gone ahead and created a revert review: https://reviews.llvm.org/D108243

It appears as though this commit was reverted in Apple's XCode Clang fork -- the behavior currently in XCode matches the behavior of upstream Clang prior to this patch. Presuming that's correct, I think we should revert this upstream as well. There doesn't seem to be a good reason to have the semantics of blocks differ between Apple's clang and upstream clang.

In D106577#2904960, @rsmith wrote:

One specific example I'd like to be considered:
Suppose the C standard library implementation's mbstowcs converts a certain multi-byte character C to somewhere in the Unicode private use area, because Unicode version N doesn't have a corresponding character. Suppose further that the compiler is aware of Unicode version N+1, in which a character corresponding to C was added. Is an implementation formed by that combination of compiler and standard library, that defines __STDC_ISO_10646__ to N+1, conforming? Or is it non-conforming because it represents character C as something other than the corresponding short name from Unicode version N+1?

And David Keaton (long-time WG14 member and current convener) replied:

Yikes! It does indeed sound like the library would affect the value of __STDC_ISO_10646__ in that case. Thanks for clarifying the details.

There was no further discussion after that point, so I think the unofficial WG14 stance is that the compiler and the library need to collude on setting the value of that macro.

This was committed in fbd3bb4365e145a5033fdeea0ede978304d22d68.

BTW, looks like the standard wording came from:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_273.htm

In D106577#2904960, @rsmith wrote:

One benefit we don't get with this approach is providing the right value for the macro (without paying the cost of always including stdc-predefs.h).

Perhaps a reasonable path forward here to address the BSD issue can be to add a targetinfo method:

/* Returns true if the expected encoding of wchar_t changes at runtime
   depending on locale for this target.
   Note that clang always encodes wide character literals as utf16/utf32,
   regardless. */
bool usesDynamicWCharEncoding();

which returns false by default, and true for FreeBSD, NetBSD, OpenBSD, and Solaris. Then, the condition for setting this define can be if(TI.getWCharWidth() >= 32 && !TI.usesDynamicWcharEncoding()).

In D106577#2901654, @rsmith wrote:

I think this patch as it stands would cause problems with libc implementations that put their #define somewhere other than than stdc-predef.h (eg, older versions of glibc that put it in features.h or other standard libraries that put it in yvals.h) due to the macro redefinition with a conflicting expansion. Such implementations are non-conforming but certainly exist. Likewise it could break the build for the same reason if we start implicitly including stdc-predef.h at some point. So I think that blindly predefining this macro will prove problematic in at least some circumstances.

Even after the more recent discussion, I still think my initial message was incorrect, and that the compiler should be defining this macro itself, as proposed in this patch. Note that my confusion was not that the macro being defined or not was dependent on libc behavior, only the precise value it should be defined to.

In D106577#2897588, @aaron.ballman wrote:

In D106577#2897522, @jyknight wrote:

I'm not sure we should be populating this.

The _value_ is determined by what libc supports, so it probably needs to be left up to libc to define it.

Why is the value determined by what libc supports? The definition from the standard is:

If this symbol is defined, then every character in the Unicode required set, when stored in an
object of type wchar_t, has the same value as the short identifier of that character.

That doesn't seem to imply anything about the library, just the size of wchar_t.

I'm not sure we should be populating this.

Please make sure to update the description to match the new code before committing.

In D103935#2879061, @tstellar wrote:

This patch causes segmentation faults when you have an application compiled with -std=c++17 (The current gcc default) and libLLVM.so compiled with c++14 (The current LLVM project default). Is this a valid use case or do we require that all library users use the same c++ version as the library.

This commit seems to have broken libc++ in C++98 mode, as it appears to have depended upon the implicit-move extension.

This code doesn't handle multiple alternatives in a constraint.

Ping. I think this is correct, and would like to commit.

(I note the new behavior also matches gcc)

A git diff generally has an extraneous path component [ab] first, e.g. your diff should look like this:

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 4e3af0cb2649..78035c38ee50 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h

In D103833#2803811, @stuart wrote:

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

Sounds like a great plan to me.

Won't this change cause weird effects with LTO, when you're merging multiple TUs' global_ctors arrays before emitting? Won't this end up reversing the order of the files, as well as the order of the functions within a single file? That does not seem likely to be expected, right?