Page MenuHomePhabricator

[Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled
Changes PlannedPublic

Authored by lxfind on Dec 4 2020, 8:50 AM.

Details

Summary

This patch is to address https://bugs.llvm.org/show_bug.cgi?id=47833
A relevant discussion can also be found in http://lists.llvm.org/pipermail/llvm-dev/2020-November/146766.html

pthread_self() from glibc is defined with "attribute
((const))". The const attribute tells the compiler that it does
not read nor write any global state and hence always return the same
result. Hence in the following code:

auto x1 = pthread_self();
...
auto x2 = pthread_self();

the second call to pthread_self() can be optimized out. This has been
correct until coroutines. With coroutines, we can have code like this:

auto x1 = pthread_self();
co_await ...
auto x2 = pthread_self();

Now because of the co_await, the function can suspend and resume in a
different thread, in which case the second call to pthread_self()
should return a different result than the first one. Unfortunately
LLVM will still optimize out the second call in the case of
coroutines.

To fix the issue, this patch drops the readnone attribute from the pthread_self function in Clang.

Diff Detail

Event Timeline

lxfind created this revision.Dec 4 2020, 8:50 AM
lxfind requested review of this revision.Dec 4 2020, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 8:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lxfind updated this revision to Diff 310599.Dec 9 2020, 11:50 AM

Fix test

MaskRay added a subscriber: MaskRay.Dec 9 2020, 2:48 PM

It is worth requesting clarification on GCC __attribute__((const)): (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66512)

If the attribute is not suitable, glibc should drop it. The compiler can add readnone/readonly if appropriate.

lxfind added a comment.Dec 9 2020, 4:28 PM

If the attribute is not suitable, glibc should drop it. The compiler can add readnone/readonly if appropriate.

It's a C library interface though, and Coroutine is likely too new for this. I think that in the long run when Coroutine is pervasive we probably should drop it, but for now it's likely going to be hard for the glibc community to consider dropping it.

If the attribute is not suitable, glibc should drop it. The compiler can add readnone/readonly if appropriate.

It's a C library interface though, and Coroutine is likely too new for this. I think that in the long run when Coroutine is pervasive we probably should drop it, but for now it's likely going to be hard for the glibc community to consider dropping it.

My comment is more about "we should bring awareness to the glibc community" (sending a plain-text message to libc-alpha@sourceware.org), otherwise if we decide to add a special case, such a hack will stay in Clang forever.

I don't think we should change the meaning of __attribute__((const)) to exclude depending on thread-id.

However, if we do want to do so, and call the existing uses of __attribute__((const)) in glibc invalid, we need to special case many more functions. Looking through it a little, I see __errno_location, __rpc_thread_variables, __ctype_b_loc, __ctype_tolower_loc, __ctype_toupper_loc, __libc_tsd_address...and I gave up looking after that.

I don't think we should change the meaning of __attribute__((const)) to exclude depending on thread-id.

However, if we do want to do so, and call the existing uses of __attribute__((const)) in glibc invalid, we need to special case many more functions. Looking through it a little, I see __errno_location, __rpc_thread_variables, __ctype_b_loc, __ctype_tolower_loc, __ctype_toupper_loc, __libc_tsd_address...and I gave up looking after that.

Thanks for pointing it out. I didn't realize there are so many of them.
Your proposals in the llvm-dev thread sound very promising. Let me think them over.

lxfind planned changes to this revision.Feb 18 2021, 10:35 AM