[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Ping @jrtc27


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:2058
+/// Missing a type from 
+GE_Missing_pthread
   };

You can append a comma so that next time the list gets appended the diff does 
not have to touch this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1223
 /// The number of special type IDs.
 const unsigned NumSpecialTypeIDs = 8;
 

This presumably needs to be increased. Are we missing test coverage that would 
have caught this?



Comment at: clang/lib/AST/ASTContext.cpp:9386-9389
+static QualType DecodeFunctionTypeFromStr(
+const char *, bool IsNested, bool IsNoReturn, bool IsNoThrow,
+bool ForceEmptyTy, const ASTContext ,
+ASTContext::GetBuiltinTypeError , unsigned *IntegerConstantArgs);

Our convention for such helper functions is to pass the reference to the 
associated class (`ASTContext` in this case) as the first parameter. (I 
appreciate that `DecodeTypeFromStr` violates this convention.)



Comment at: clang/lib/AST/ASTContext.cpp:9677-9679
+Type = DecodeFunctionTypeFromStr(
+Str, /*IsNested=*/true, /*IsNoReturn=*/false,
+/*IsNoThrow=*/false, /*ForceEmptyTy=*/false, Context, Error, nullptr);

OK. We may eventually want a way to specify noreturn and nothrow here, because 
they're both part of the type in at least some cases, but this seems fine for 
the immediate future.



Comment at: clang/lib/AST/ASTContext.cpp:9739
+  if (TypeStr[0] == ')') {
+assert(IsNested && "unmatched ')' found at end of type list");
+Error = ASTContext::GE_Missing_type;

Maybe "missing return type in builtin function type" or similar would be a more 
useful assert message here?



Comment at: clang/lib/AST/ASTContext.cpp:9756
+  while (TypeStr[0] && TypeStr[0] != '.' && TypeStr[0] != ')') {
+QualType Ty = DecodeTypeFromStr(TypeStr, Context, Error, RequiresICE, 
true);
+if (Error != ASTContext::GE_None)

Please `assert(!RequiresICE || !IsNested)` here; we shouldn't require 
parameters of function pointer parameters to be ICEs.



Comment at: clang/lib/AST/ASTContext.cpp:9765-9767
 // Do array -> pointer decay.  The builtin should use the decayed type.
 if (Ty->isArrayType())
+  Ty = Context.getArrayDecayedType(Ty);

Should we do function -> pointer decay here too, so you can use `(...)` instead 
of `(...)*` in `Builtins.def`?



Comment at: clang/lib/AST/ASTContext.cpp:9775-9776
+
+  if (ForceEmptyTy)
 return {};
 

Do we need this special case and its associated flag? Can we make 
`GetBuiltinType` directly return `QualType();`for `_GetExceptionInfo` instead?



Comment at: clang/lib/AST/ASTContext.cpp:9778-9779
 
   assert((TypeStr[0] != '.' || TypeStr[1] == 0) &&
  "'.' should only occur at end of builtin type list!");
 

This will assert on `(.)`.



Comment at: clang/lib/Sema/SemaDecl.cpp:1971-1972
 return "ucontext.h";
+  case ASTContext::GE_Missing_pthread:
+return "pthread.h";
   }

Could we use `GE_Missing_type` here instead? The `LIBBUILTIN` already lists 
"pthread.h" as the corresponding header, so that seems sufficient. I think we 
only need the special cases above to handle the non-`LIBBUILTIN` cases such as 
`__builtin_fprintf` that nonetheless require a header inclusion (in that case 
to provide `FILE`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 marked an inline comment as done.
jrtc27 added a comment.

In D58531#1662466 , @jdoerfert wrote:

> I like the patch and I think it is fine.
>
> Small nits: 
>  Could we have a test for " we can detect dodgy pthread_create declarations" 
> and maybe `pthread[_attr]_t`?
>  There is an unresolved comment by @shrines.
>  @probinson: "it seems straightforward enough although clearly needs 
> clang-format-diff run over it."
>
> I'll accept this assuming the above points are easy to fix and given that no 
> one expressed concerns but only positive comments were made.


I believe I have addressed everything, with the possible exception of:

> Could we have a test for " we can detect dodgy pthread_create declarations" 
> and maybe `pthread[_attr]_t`?

Is that not what I had already added to 
`clang/test/Sema/implicit-builtin-decl.c`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 updated this revision to Diff 223871.
jrtc27 added a comment.

Rebased, added explicit no-warning test, clang-format'ed, updated assertion 
message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/implicit-builtin-redecl.c

Index: clang/test/Sema/implicit-builtin-redecl.c
===
--- clang/test/Sema/implicit-builtin-redecl.c
+++ clang/test/Sema/implicit-builtin-redecl.c
@@ -24,3 +24,9 @@
 }
 
 typedef int rindex;
+
+// PR40692
+typedef void pthread_t;
+typedef void pthread_attr_t;
+int pthread_create(pthread_t *, const pthread_attr_t *, // no-warning
+   void *(*)(void *), void *);
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -68,4 +68,4 @@
 // CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
 
 // PR40692
-void pthread_create(); // no warning expected
+void pthread_create(); // expected-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header }}
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4966,6 +4966,8 @@
   AddTypeRef(Context.ObjCClassRedefinitionType, SpecialTypes);
   AddTypeRef(Context.ObjCSelRedefinitionType, SpecialTypes);
   AddTypeRef(Context.getucontext_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_attr_tType(), SpecialTypes);
 
   if (Chain) {
 // Write the mapping information describing our module dependencies and how
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4847,6 +4847,43 @@
 }
   }
 }
+
+if (unsigned Pthread_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_T]) {
+  QualType Pthread_tType = GetType(Pthread_t);
+  if (Pthread_tType.isNull()) {
+Error("pthread_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_tDecl) {
+if (const TypedefType *Typedef = Pthread_tType->getAs())
+  Context.setpthread_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_tType->getAs();
+  assert(Tag && "Invalid pthread_t type in AST file");
+  Context.setpthread_tDecl(Tag->getDecl());
+}
+  }
+}
+
+if (unsigned Pthread_attr_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_ATTR_T]) {
+  QualType Pthread_attr_tType = GetType(Pthread_attr_t);
+  if (Pthread_attr_tType.isNull()) {
+Error("pthread_attr_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_attr_tDecl) {
+if (const TypedefType *Typedef =
+Pthread_attr_tType->getAs())
+  Context.setpthread_attr_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_attr_tType->getAs();
+  assert(Tag && "Invalid pthread_attr_t type in AST file");
+  Context.setpthread_attr_tDecl(Tag->getDecl());
+}
+  }
+}
   }
 
   ReadPragmaDiagnosticMappings(Context.getDiagnostics());
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -1968,6 +1968,8 @@
 return "setjmp.h";
   case ASTContext::GE_Missing_ucontext:
 return "ucontext.h";
+  case ASTContext::GE_Missing_pthread:
+return "pthread.h";
   }
   llvm_unreachable("unhandled error kind");
 }
@@ -5970,6 +5972,10 @@
 Context.setsigjmp_bufDecl(NewTD);
   else if (II->isStr("ucontext_t"))
 Context.setucontext_tDecl(NewTD);
+  else if (II->isStr("pthread_t"))
+Context.setpthread_tDecl(NewTD);
+  else if (II->isStr("pthread_attr_t"))
+Context.setpthread_attr_tDecl(NewTD);
 }
 
   return NewTD;
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -9383,6 +9383,11 @@
 //  Builtin Type Computation
 //===--===//
 
+static QualType DecodeFunctionTypeFromStr(
+const char *, bool IsNested, bool IsNoReturn, bool IsNoThrow,
+bool 

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

I like the patch and I think it is fine.

Small nits: 
Could we have a test for " we can detect dodgy pthread_create declarations" and 
maybe `pthread[_attr]_t`?
There is an unresolved comment by @shrines.
@probinson: "it seems straightforward enough although clearly needs 
clang-format-diff run over it."

I'll accept this assuming the above points are easy to fix and given that no 
one expressed concerns but only positive comments were made.




Comment at: clang/lib/AST/ASTContext.cpp:9239
+  ASTContext::GetBuiltinTypeError 
,
+  unsigned *IntegerConstantArgs);
+

I like the static helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D58531#1611356 , @jdoerfert wrote:

> In D58531#1599209 , @probinson wrote:
>
> > We've started running into this too in building the PS4 system. +jdoerfert 
> > who added pthread_create to the builtin list.
> >
> > Looking at the patch, it seems straightforward enough although clearly 
> > needs clang-format-diff run over it.
> >  I don't touch Clang that much so I'm reluctant to okay it myself.
>
>
> @probinson Thanks for making me aware of this patch.
>
> > A separate point is whether it makes sense to be emitting this warning in 
> > the first place for GE_Missing_type. I'd argue that, if we don't know the 
> > type of the builtin, we should never emit the warning
>
> @jrtc27 I hope the immediate need for this is gone after D58091 
>  was finally committed? Given that I caused 
> this mess I can take a look at the patch if you are still interested in it, 
> are you?


Yes, now that it's not a warning if we don't provide a type this is no longer 
completely necessary. I would still be slightly in favour of this patch though 
as then we can detect dodgy `pthread_create` declarations, but it's not a big 
deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D58531#1599209 , @probinson wrote:

> We've started running into this too in building the PS4 system. +jdoerfert 
> who added pthread_create to the builtin list.
>
> Looking at the patch, it seems straightforward enough although clearly needs 
> clang-format-diff run over it.
>  I don't touch Clang that much so I'm reluctant to okay it myself.


@probinson Thanks for making me aware of this patch.

> A separate point is whether it makes sense to be emitting this warning in the 
> first place for GE_Missing_type. I'd argue that, if we don't know the type of 
> the builtin, we should never emit the warning

@jrtc27 I hope the immediate need for this is gone after D58091 
 was finally committed? Given that I caused 
this mess I can take a look at the patch if you are still interested in it, are 
you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Paul Robinson via Phabricator via cfe-commits]

probinson added a reviewer: jdoerfert.
probinson added a comment.

We've started running into this too in building the PS4 system. +jdoerfert who 
added pthread_create to the builtin list.

Looking at the patch, it seems straightforward enough although clearly needs 
clang-format-diff run over it.
I don't touch Clang that much so I'm reluctant to okay it myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:9574
 
-  assert(!RequiresICE && "Result of intrinsic cannot be required to be an 
ICE");
+  assert(!RequiresICE && "Result of function cannot be required to be an ICE");
 

I would just remove "of function" here, since it isn't correct when you are not 
decoding a function, and the two words don't make that much more meaningful 
even in the original case, so I don't think it justifies something like "of 
intrinsic/function".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

A separate point is whether it makes sense to be emitting this warning in the 
first place for `GE_Missing_type`. I'd argue that, if we don't know the type of 
the builtin, we should never emit the warning, since otherwise we always emit 
the warning even for legitimate instances, though that's not really important 
now we're not hitting that case any more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 created this revision.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.

Currently, pthread_create's type string is empty, so GetBuiltinType
fails and any header declaring it gives a -Wbuiltin-requires-header
warning, which gives a false-positive when including pthread.h itself
with -Wsystem-headers. By specifying its type, this false-positive goes
away and it behaves like every other builtin declared in system headers,
only warning when the declaration does not match the expected type.

This requires introducing a way to declare function pointer types in
builtin type strings, which requires refactoring GetBuiltinType to allow
recursive parsing of function types, expressed as a type string within
parentheses.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58531

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Analysis/retain-release.m
  clang/test/Sema/implicit-builtin-decl.c

Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -66,3 +66,5 @@
 // CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
 // CHECK-NOT: FunctionDecl
 // CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+
+int pthread_create(); // expected-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header }}
Index: clang/test/Analysis/retain-release.m
===
--- clang/test/Analysis/retain-release.m
+++ clang/test/Analysis/retain-release.m
@@ -1231,7 +1231,7 @@
 typedef unsigned long __darwin_pthread_key_t;
 typedef __darwin_pthread_key_t pthread_key_t;
 
-int pthread_create(pthread_t *, const pthread_attr_t *,  // C-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header }}
+int pthread_create(pthread_t *, const pthread_attr_t *,  // no-warning
void *(*)(void *), void *);
 
 int pthread_setspecific(pthread_key_t key, const void *value);
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4904,6 +4904,8 @@
   AddTypeRef(Context.ObjCClassRedefinitionType, SpecialTypes);
   AddTypeRef(Context.ObjCSelRedefinitionType, SpecialTypes);
   AddTypeRef(Context.getucontext_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_attr_tType(), SpecialTypes);
 
   if (Chain) {
 // Write the mapping information describing our module dependencies and how
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4560,6 +4560,42 @@
 }
   }
 }
+
+if (unsigned Pthread_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_T]) {
+  QualType Pthread_tType = GetType(Pthread_t);
+  if (Pthread_tType.isNull()) {
+Error("pthread_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_tDecl) {
+if (const TypedefType *Typedef = Pthread_tType->getAs())
+  Context.setpthread_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_tType->getAs();
+  assert(Tag && "Invalid pthread_t type in AST file");
+  Context.setpthread_tDecl(Tag->getDecl());
+}
+  }
+}
+
+if (unsigned Pthread_attr_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_ATTR_T]) {
+  QualType Pthread_attr_tType = GetType(Pthread_attr_t);
+  if (Pthread_attr_tType.isNull()) {
+Error("pthread_attr_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_attr_tDecl) {
+if (const TypedefType *Typedef = Pthread_attr_tType->getAs())
+  Context.setpthread_attr_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_attr_tType->getAs();
+  assert(Tag && "Invalid pthread_attr_t type in AST file");
+  Context.setpthread_attr_tDecl(Tag->getDecl());
+}
+  }
+}
   }
 
   ReadPragmaDiagnosticMappings(Context.getDiagnostics());
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -1943,6 +1943,8 @@
 return "setjmp.h";
   case ASTContext::GE_Missing_ucontext:
 return "ucontext.h";
+  case ASTContext::GE_Missing_pthread:
+return "pthread.h";
   }
   llvm_unreachable("unhandled error kind");
 }
@@ -5809,6 +5811,10 @@