Patch LGTM. Thanks for looking into the concerns.

This LGTM; thanks!

In D144846#4167823, @Jake-Egan wrote:

Hi, this change caused some failures on AIX. Could you take a look please?

@philnik, can you update this to always have the key function on AIX and remove it for other platforms?

In D134282#4148884, @hubert.reinterpretcast wrote:

Not that I know of without negative side-effects. For example, if we access using a TOC entry for a label for the individual string, we would end up with more TOC entries which, in turn, can lead to TOC overflow.

In D134282#4148337, @efriedma wrote:

Is there any way we can make AIX targets more resilient handling this kind of alignment change? If we have to, we can choose a point after which passes aren't allowed to increase the alignment of globals, and move this transform before that. But I'd like to avoid that if possible.

In D134282#4141617, @Jake-Egan wrote:

It fails with a different assembler error on different platforms (tried on AIX, linux, and mac). No error if you remove the AIX target.

clang++ -target powerpc64-ibm-aix -c foo.cc

In D142867#4108079, @eaeltsin wrote:

The warning now fires even if overflow is prevented with if constexpr:

if constexpr (width <= 64) {
  if constexpr (width == 64) {
    return 1;
  }
  return -static_cast<int64_t>(uint64_t{1} << (width - 1));
}

https://godbolt.org/z/M3xdcKd3M

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

I think it may be an option to use the gnu++* modes to do deliberately non-conforming things, but I believe there should be an RFC for that first.

LGTM; thanks!

Waiting on changes to fix the visitation to locate the appropriate temporaries.

I made changes to the patch description (which I expect we will be using for the commit message).

Since this is a change only for AIX and z/OS, and considering that this is a patch to prevent ABI changes compared to the current ABI baselines for those platforms, I will land this ahead of the LLVM 16 branching.

Thanks @nicolerabjohn; this LGTM!

@francii, from offline discussion, a change is pending to use a different approach (the current one here breaks load ABI). If the change is going to take time, please mark this as having changes planned (i.e., not ready for review).

In D137756#4018666, @MaskRay wrote:

Most targets reject -p now. It's unnecessary to have another z/OS specific diagnostic. So this patch can be abandoned.

% fclang -p a.cc
clang-16: error: unsupported option '-p' for target 'x86_64-unknown-linux-gnu'

At least on the surface, it looks like there will be a lot of trouble to deal with default arguments:

struct A { A(); ~A(); int x[3]; };
int (&f(const A & = A()))[3];
void bar(int);
void foo() {
  for (auto e : f()) { bar(e); }
}

In D139586#3984118, @cor3ntin wrote:

I will try to do a more detailed review later, but at least I think we might want more tests. Maybe codegen tests that do not rely on [[clang::lifetimebound]], and tests with more chaining (a().b().c()) .

In D139586#3987252, @royjacobson wrote:

The (non-wording) paper makes a pretty convincing case to just apply this retroactively to any C++11 code (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2644r1.pdf). I think we should apply this retroactively, maybe add a pedantic warning when we do a lifetime extension on code before C++23.

LGTM; thanks!

LGTM; thanks!

LGTM; thanks!

LGTM; no objection if we're planning to move ahead without checking the calculated size field in the traceback table.

I am a bit torn over not actually checking that the calculated function sizes are correct, but I guess that we can infer from there being no relocations generated for it that we are going through the right MCExpr processing.

In D137230#3924110, @Esme wrote:

Addressed comments.

LGTM with comment.

In D137110#3899684, @phosek wrote:

What's the use case? I find this potentially problematic. What if the compiler specified for running tests with doesn't support the same set of flags as the one used for building?

In D137110#3899449, @ldionne wrote:

But first, I'd like to understand why you're not setting CMAKE_CXX_COMPILER to the compiler you're trying to run the tests with?

@w2yehia, this can be closed now (the other patch that also covers what this one is solving has landed)?

Confirming LGTM from the AIX side.

This LGTM. Size of Registration changes from 1 byte to (likely) 4 bytes on non-AIX platforms and now incurs an extra memory write, but that should be insignificant.

LGTM from the AIX perspective.

In D135919#3859491, @tahonermann wrote:

the case from https://github.com/llvm/llvm-project/issues/57828 is not for a block-scope variable, but the case in the patch description is...

Thanks, Hubert. Yes, I found that the reported issue occurred for any case where thread safe guard variables are not required. I chose the block-scope variable example for the patch summary because I felt it better presented the issue.

Your question inspired me to do some additional testing though and I see both gcc and icc also exhibit the re-initialization behavior for that case, but not the case reported in https://github.com/llvm/llvm-project/issues/57828.

In D135919#3858808, @rjmccall wrote:

We can't set the flag if initialization is aborted by an exception, which is not ruled out by the use of thread-unsafe statics. So this is not a correct change.