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

2022-10-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Please add a comment something like this: The semantics of dynamic initialization of thread-local variables depends subtly on whether they are block-scope. The initialization of block-scope thread locals can be aborted with an exception and later retried (), and

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

2022-10-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D135919#3859520 , @hubert.reinterpretcast wrote: > In D135919#3859491 , @tahonermann > wrote: > >>> the case from https://github.com/llvm/llvm-project/issues/57828 is not for >>> a

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

2022-10-14 Thread Hubert Tong via Phabricator via cfe-commits
hubert.reinterpretcast added a comment. 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.

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

2022-10-14 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. > 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

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

2022-10-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Sorry, I responded as if you were modifying the guards of static constructors, but you're actually modifying the guards of thread locals. I apologize for the confusion. However, my substantive points actually all still hold: - we have to generate code that behaves

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

2022-10-14 Thread Hubert Tong via Phabricator via cfe-commits
hubert.reinterpretcast added a comment. 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.

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

2022-10-14 Thread John McCall via Phabricator via cfe-commits
rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. 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. More

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

2022-10-14 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135919/new/ https://reviews.llvm.org/D135919

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

2022-10-14 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. So this logically looks/sounds right to me (and I have no code concerns)... But I'm also notoriously bad at anything threading related. I'd love it if one of the other reviewers could sanity check this for me. Repository: rG LLVM Github Monorepo CHANGES SINCE

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

2022-10-13 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann created this revision. tahonermann added reviewers: erichkeane, aaron.ballman, hubert.reinterpretcast, rjmccall. Herald added a project: All. tahonermann requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Previously, for