[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D135919#3859520 <https://reviews.llvm.org/D135919#3859520>, @hubert.reinterpretcast wrote:

> In D135919#3859491 <https://reviews.llvm.org/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.
>
> For the block-scope case, https://eel.is/c++draft/stmt.dcl#3: If control re-enters the declaration recursively while the variable is being initialized, the behavior is undefined.

Ah, right, so in principle we can just ignore this possibility.  It would still be good to have some mode that dynamically diagnoses it, though, maybe under UBSan.  We could probably get the Itanium ABI to agree to a standard guard value that means "currently being initialized" which implementations could use if they want to diagnose this case.

For non-block variables, this is a deferred initialization, as set out in [basic.start.dynamic]p7 (https://eel.is/c++draft/basic.start.dynamic#7).  I don't think there's an analogous restriction about recursion, so only the general object lifetime rules apply, [basic.life]p7 (https://eel.is/c++draft/basic.life#7).  A recursive use during initialization would be a use of the "original object" prior to the completion of initialization.  Under the lifetime rules, the acceptability of that depends on what's being done, but generally, anything that accesses memory through it is either UB or yields an unspecified value.  That does not permit us to do arbitrary things whenever we see a recursive reference, though, unless we actually do an analysis of that reference, which we are not doing here.

Note also that, for non-block variables, the exceptions concern does not arise because [basic.start.dynamic]p8 (https://eel.is/c++draft/basic.start.dynamic#8) says that exceptions during initialization trigger `std::terminate`.

I think that suggests that setting the guard to 1 prior to initialization is actually *required* for the deferred initialization of thread-locals, because otherwise we are interfering with the evaluation of uses of the name that are potentially valid prior to initialization, such as storing the address of the variable somewhere without accessing it.  And if the use isn't valid and is one of the UB/unspecified cases, well, it's too bad we can't catch that immediately, but it's no worse than what would happen with static storage duration.

So if you alter this patch to only apply to non-block variables, I think we can move forward with it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135919/new/

https://reviews.llvm.org/D135919