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

[lxfind]

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.

[lxfind]

Fix test

[MaskRay]

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]

In D92662#2443970, @MaskRay wrote:

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.

[MaskRay]

In D92662#2444196, @lxfind wrote:

In D92662#2443970, @MaskRay wrote:

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 [e-mail]), otherwise if we decide to add a special case, such a hack will stay in Clang forever.

[jyknight]

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.

[lxfind]

In D92662#2446777, @jyknight wrote:

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.

clang/lib/Sema/SemaDecl.cpp

if (getLangOpts().CXXExceptions && getLangOpts().ExternCNoUnwind &&
     const auto *FPT = FD->getType()->getAs<FunctionProtoType>();
     if (!FPT || FPT->getExceptionSpecType() == EST_None)
       FD->addAttr(NoThrowAttr::CreateImplicit(Context, FD->getLocation()));
   }
 
   IdentifierInfo *Name = FD->getIdentifier();
   if (!Name)
     return;
+
+  if (getLangOpts().Coroutines && Name->isStr("pthread_self") &&
+      FD->hasAttr<ConstAttr>()) {
+    FD->dropAttr<ConstAttr>();
+  }
+
   if ((!getLangOpts().CPlusPlus &&
        FD->getDeclContext()->isTranslationUnit()) ||
       (isa<LinkageSpecDecl>(FD->getDeclContext()) &&
        cast<LinkageSpecDecl>(FD->getDeclContext())->getLanguage() ==
        LinkageSpecDecl::lang_c)) {
     // Okay: this could be a libc/libm/Objective-C function we know
     // about.
   } else

clang/test/CodeGenCoroutines/coro-pthread_self.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -O3 -emit-llvm %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+typedef void *pthread_t;
+pthread_t pthread_self(void) __attribute__((__const__));
+
+struct awaitable {
+  bool await_ready() { return false; }
+  void await_suspend(coro::coroutine_handle<> h);
+  void await_resume() {}
+};
+awaitable switch_to_new_thread();
+
+struct task {
+  struct promise_type {
+    task get_return_object() { return {}; }
+    coro::suspend_never initial_suspend() { return {}; }
+    coro::suspend_never final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+};
+
+void check(pthread_t p1, pthread_t p2);
+
+task resuming_on_new_thread() {
+  auto pthread1 = pthread_self();
+  co_await switch_to_new_thread();
+  auto pthread2 = pthread_self();
+  check(pthread1, pthread2);
+}
+
+void non_coroutine() {
+  auto pthread1 = pthread_self();
+  check(pthread1, pthread1);
+  auto pthread2 = pthread_self();
+  check(pthread1, pthread2);
+}
+
+// CHECK-LABEL: define void @_Z13non_coroutinev()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    %call = tail call i8* @_Z12pthread_selfv()
+// CHECK-NEXT:    tail call void @_Z5checkPvS_(i8* %call, i8* %call)
+// CHECK-NEXT:    %call1 = tail call i8* @_Z12pthread_selfv()
+// CHECK-NEXT:    tail call void @_Z5checkPvS_(i8* %call, i8* %call1)
+// CHECK-NEXT:    ret void
+// CHECK-NEXT:  }
+
+// CHECK-LABEL: define internal fastcc void @_Z22resuming_on_new_threadv.resume
+// CHECK:         %[[RELOAD_ADDR:.+.reload.addr]] = getelementptr inbounds %_Z22resuming_on_new_threadv.Frame, %_Z22resuming_on_new_threadv.Frame* %FramePtr, i64 0, i32 {{.+}}
+// CHECK:         %[[RELOAD:.+]] = load i8*, i8** %[[RELOAD_ADDR]], align 8
+// CHECK:         %[[CALL:.+]] = tail call i8* @_Z12pthread_selfv()
+// CHECK:         tail call void @_Z5checkPvS_(i8* %[[RELOAD]], i8* %[[CALL]])