[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#3038277 , @ychen wrote:

> In D77491#3038204 , @tambre wrote:
>
>> Abandoning since I don't think there's any further review/actions to be done 
>> here.
>> D58531  would be the solution for the 
>> raised `pthread_create()` issue.
>
> How about making this "Closed" instead of "Abandoned" considering the patch 
> was actually landed?

Good point. I assumed this wasn't possible as the web UI didn't expose an 
option for this.
But I have managed using `arc close-revision`. Needed to reclaim and get it 
into an accepted state to be able to do it though. Sorry for the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D77491#3038204 , @tambre wrote:

> Abandoning since I don't think there's any further review/actions to be done 
> here.
> D58531  would be the solution for the raised 
> `pthread_create()` issue.

How about making this "Closed" instead of "Abandoned" considering the patch was 
actually landed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre abandoned this revision.
tambre added a comment.

Abandoning since I don't think there's any further review/actions to be done 
here.
D58531  would be the solution for the raised 
`pthread_create()` issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2870291 , @jdoerfert wrote:

> First:
> Do I assume right this this feature was simply disabled without any plan to:
>
> - inform the authors (me)
> - update the documentation
> - re-enable support eventually or provide alternatives
>
> XFAILing a test and calling it a day seems inadequate IMHO.
>
> Second:
> Would an approach like this still work: https://reviews.llvm.org/D58531 ?

Informing you would've probably been appropriate in hindsight.
I'm not aware of any relevant documentation that would've been appropriate to 
update.

Re-enabling the support depends on someone taking up the work to correctly 
implement the prototype recognition, which seems to be done in D58531 
 and would work as-is.
However, at the time it seemed to me that the whole builtin declaration 
recognition could use a rewrite to be able to this support this case without 
hacks and require less manual C++ type juggling.
I considered that work beyond my experience with the codebase and the benefits 
of this work easily seemed to outweigh the lack of additional annotations for 
this single function. I received no further feedback on the disabling of that 
test after explaining the reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert reopened this revision.
jdoerfert added a comment.
Herald added a subscriber: dexonsmith.

> A pthread_create() related test is XFAIL-ed, as it relied on it being 
> recognized as a builtin based on its name.
> The builtin declaration syntax is too restrictive and doesn't allow custom 
> structs, function pointers, etc.
> It seems to be the only case and fixing this would require reworking the 
> current builtin syntax, so this seems acceptable.

First:
Do I assume right this this feature was simply disabled without any plan to:

- inform the authors (me)
- update the documentation
- re-enable support eventually or provide alternatives

XFAILing a test and calling it a day seems inadequate IMHO.

Second:
Would an approach like this still work: https://reviews.llvm.org/D58531 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2300013 , @rjmccall wrote:

> Being permissive about recognizing builtins when the expected signature 
> requires a type that lookup can't find seems completely reasonable.  We don't 
> really want to force library functions to take the custom-typechecking path 
> just because we want to infer an attribute for them.

Proposed fix in https://reviews.llvm.org/D88518.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Being permissive about recognizing builtins when the expected signature 
requires a type that lookup can't find seems completely reasonable.  We don't 
really want to force library functions to take the custom-typechecking path 
just because we want to infer an attribute for them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2299938 , @rsmith wrote:

> We've hit a fairly subtle miscompile caused by this patch.
>
> glibc's setjmp.h looks like this (irrelevant parts removed):
>
>   struct __jmp_buf_tag { /*...*/ };
>   extern int __sigsetjmp(struct __jmp_buf_tag __env[1], int);
>   typedef struct __jmp_buf_tag sigjmp_buf[1];
>   #define sigsetjmp __sigsetjmp
>
> This worked fine with the old approach. But with the new approach, we decide 
> the declaration of `__sigsetjmp` is not a builtin, because at its point of 
> declaration, we can't compute the "proper" type because `sigjmp_buf` has not 
> been declared yet. As a result, we don't add a `BuiltinAttr` to 
> `__sigsetjmp`, but much more critically, we don't add a `ReturnsTwiceAttr`, 
> which results in miscompiles in calls to this function. (I think `sigsetjmp` 
> is the only affected function with glibc. `jmp_buf` is declared prior to 
> `__setjmp` and friends.)
>
> I suppose we don't actually care what the parameter types for `__sigsetjmp` 
> are, and it would be fine (and much safer) to treat any function with that 
> name as a builtin, like we used to. Perhaps we should have a way of marking 
> builtins as "the given type is what we expect / what we will implicitly 
> declare, but it's OK if it doesn't actually match"?

Marking `__sigsetjmp` as having custom typechecking should suffice (`t` 
attribute in Builtins.def), no? Though a case in 
`Sema::CheckBuiltinFunctionCall()` might also then be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

We've hit a fairly subtle miscompile caused by this patch.

glibc's setjmp.h looks like this (irrelevant parts removed):

  struct __jmp_buf_tag { /*...*/ };
  extern int __sigsetjmp(struct __jmp_buf_tag __env[1], int);
  typedef struct __jmp_buf_tag sigjmp_buf[1];
  #define sigsetjmp __sigsetjmp

This worked fine with the old approach. But with the new approach, we decide 
the declaration of `__sigsetjmp` is not a builtin, because at its point of 
declaration, we can't compute the "proper" type because `sigjmp_buf` has not 
been declared yet. As a result, we don't add a `BuiltinAttr` to `__sigsetjmp`, 
but much more critically, we don't add a `ReturnsTwiceAttr`, which results in 
miscompiles in calls to this function. (I think `sigsetjmp` is the only 
affected function with glibc. `jmp_buf` is declared prior to `__setjmp` and 
friends.)

I suppose we don't actually care what the parameter types for `__sigsetjmp` 
are, and it would be fine (and much safer) to treat any function with that name 
as a builtin, like we used to. Perhaps we should have a way of marking builtins 
as "the given type is what we expect / what we will implicitly declare, but 
it's OK if it doesn't actually match"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2282219 , @tambre wrote:

> In D77491#2282166 , @dmajor wrote:
>
>> This commit broke Firefox builds on Mac with an error in the SDK headers. 
>> Could you please revert if a fix is not readily available?
>>
>> Reproducer:
>>
>>   struct objc_super {};
>>   extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...);
>>   extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
>> ...);
>>
>> Result:
>>
>>   $ clang -c test.mm
>>   test.mm:3:48: error: reference to 'objc_super' is ambiguous
>>   extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
>> ...);
>>  ^
>>   test.mm:1:8: note: candidate found by name lookup is 'objc_super'
>>   struct objc_super {};
>>  ^
>>   note: candidate found by name lookup is 'objc_super'
>>   1 error generated.
>
> Looking into this, got an idea what might be wrong. Gimme an hour or two. :)

Fix in D87917 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2282166 , @dmajor wrote:

> This commit broke Firefox builds on Mac with an error in the SDK headers. 
> Could you please revert if a fix is not readily available?
>
> Reproducer:
>
>   struct objc_super {};
>   extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...);
>   extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
> ...);
>
> Result:
>
>   $ clang -c test.mm
>   test.mm:3:48: error: reference to 'objc_super' is ambiguous
>   extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
> ...);
>  ^
>   test.mm:1:8: note: candidate found by name lookup is 'objc_super'
>   struct objc_super {};
>  ^
>   note: candidate found by name lookup is 'objc_super'
>   1 error generated.

Looking into this, got an idea what might be wrong. Gimme an hour or two. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[dmajor via Phabricator via cfe-commits]

dmajor added a comment.

This commit broke Firefox builds on Mac with an error in the SDK headers. Could 
you please revert if a fix is not readily available?

Reproducer:

  struct objc_super {};
  extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...);
  extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
...);

Result:

  $ clang -c test.mm
  test.mm:3:48: error: reference to 'objc_super' is ambiguous
  extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, 
...);
 ^
  test.mm:1:8: note: candidate found by name lookup is 'objc_super'
  struct objc_super {};
 ^
  note: candidate found by name lookup is 'objc_super'
  1 error generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2280096 , @jrtc27 wrote:

> If someone cares about pthread_create they might wish to follow up on my 
> https://reviews.llvm.org/D58531, which I filed early last year to permit 
> pthread_create to have a proper type in the syntax. It will likely need 
> rebasing, but probably isn't that much work.

Sorry I missed this before! Please feel free to ping patches every ~week if 
they're not getting attention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

If someone cares about pthread_create they might wish to follow up on my 
https://reviews.llvm.org/D58531, which I filed early last year to permit 
pthread_create to have a proper type in the syntax. It will likely need 
rebasing, but probably isn't that much work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe09107ab80dc: [Sema] Introduce BuiltinAttr, per-declaration 
builtin-ness (authored by tambre).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGenCXX/builtins.cpp
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

If there are no further comments I'll commit this in a few days.




Comment at: clang/lib/Sema/SemaDecl.cpp:2088
+
+  if (Error || R.isNull())
+return nullptr;

aaron.ballman wrote:
> Should we do this check *before* we create the C linkage decl spec above?
We should. However, since now only `LazilyCreateBuiltin()` calls this and 
checks these before, we can simply removed them here and pass in the `QualType`.



Comment at: clang/lib/Sema/SemaDecl.cpp:9689
+   Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+  NewFD->addAttr(BuiltinAttr::CreateImplicit(Context, BuiltinID));
+}

rsmith wrote:
> Please can you add a `// FIXME` here that we should probably only recognize 
> this as a builtin in the scope where the MS headers actually declare it, 
> rather than in every scope.
Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 291778.
tambre marked 2 inline comments as done.
tambre added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGenCXX/builtins.cpp
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
 
-// 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith accepted this revision.
rsmith added a comment.

Thanks! This looks good to me (subject to Aaron's comment being addressed). 
Please wait a couple of days for any more comments from the other reviewers.




Comment at: clang/lib/Sema/SemaDecl.cpp:9689
+   Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+  NewFD->addAttr(BuiltinAttr::CreateImplicit(Context, BuiltinID));
+}

Please can you add a `// FIXME` here that we should probably only recognize 
this as a builtin in the scope where the MS headers actually declare it, rather 
than in every scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:2088
+
+  if (Error || R.isNull())
+return nullptr;

Should we do this check *before* we create the C linkage decl spec above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Thanks for the review. All tests still pass, should be good for another round.




Comment at: clang/lib/Sema/SemaDecl.cpp:9672-9673
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+const auto *LinkageDecl =
+dyn_cast(NewFD->getDeclContext());
+

rsmith wrote:
> This will give the wrong answer for
> ```
> extern "C" {
> namespace X {
> void __builtin_foo();
> }
> }
> ```
> ... which does have C language linkage. Instead, please call 
> `FunctionDecl::getLanguageLinkage()`, which knows how to handle these cases.
Good suggestion. This fixes the long-standing FIXME inherited from 
`getBuiltinID()`. I've added a test for this.



Comment at: clang/lib/Serialization/ASTReader.cpp:914
+  return II.hadMacroDefinition() || II.isPoisoned() ||
+ II.getObjCOrBuiltinID() || II.hasRevertedTokenIDToIdentifier() ||
  (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&

rsmith wrote:
> We now consider `getObjCOrBuiltinID()` here for the `IsModule` case, where we 
> didn't before. Is that an intentional change?
Unintentional, fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 290232.
tambre marked an inline comment as done.
tambre added a comment.

Remove now obsolete FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGenCXX/builtins.cpp
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 290229.
tambre marked 3 inline comments as done.
tambre added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGenCXX/builtins.cpp
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added inline comments.



Comment at: clang/lib/Headers/intrin.h:435
 #if defined(__i386__) || defined(__x86_64__)
-static __inline__ void __DEFAULT_FN_ATTRS
-__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n) {
+void __DEFAULT_FN_ATTRS __movsb(unsigned char *__dst,
+unsigned char const *__src, size_t __n) {

The functions with inline definitions should still be `static inline` so that 
we don't emit them as strong external defintiions.



Comment at: clang/lib/Sema/SemaDecl.cpp:9672-9673
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+const auto *LinkageDecl =
+dyn_cast(NewFD->getDeclContext());
+

This will give the wrong answer for
```
extern "C" {
namespace X {
void __builtin_foo();
}
}
```
... which does have C language linkage. Instead, please call 
`FunctionDecl::getLanguageLinkage()`, which knows how to handle these cases.



Comment at: clang/lib/Sema/SemaDecl.cpp:9690-9691
+if (!Error && !BuiltinType.isNull()) {
+  // We want noexcept declarations to match. Create an identical
+  // function type, but remove the exception spec.
+  const FunctionProtoType *Type =

Please use `ASTContext::hasSameTypeIgnoringExceptionSpec` instead.



Comment at: clang/lib/Serialization/ASTReader.cpp:914
+  return II.hadMacroDefinition() || II.isPoisoned() ||
+ II.getObjCOrBuiltinID() || II.hasRevertedTokenIDToIdentifier() ||
  (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&

We now consider `getObjCOrBuiltinID()` here for the `IsModule` case, where we 
didn't before. Is that an intentional change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2254967 , @rjmccall wrote:

> I didn't see the specific example, sorry.  I agree that my description is 
> more applicable to builtins in the `__builtin` namespace, which describes 
> most of the builtins with custom typechecking.  Is the problem just 
> `__va_start`?

I grepped for all builtins with custom typechecking.
I found two occurences in MSVC's headers: `__va_start` and 
`__GetExceptionInfo`, but none on my Debian Unstable machine.

> If we have to treat all declarations as builtins for the custom-typechecking 
> builtins just to land this patch, I don't think that's the worst result in 
> the world, and we can incrementally go from there.  `__va_start` actually has 
> a signature, it just effectively has optional arguments, which is something 
> we could definitely teach the builtins database and signature-matcher about.

I'd still prefer we go incrementally.

It would be nice to receive another round of code review after having fixed the 
issues pointed out by Richard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 289986.
tambre added a comment.

Improve comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
-// CHECK-NOT: FunctionDecl

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I didn't see the specific example, sorry.  I agree that my description is more 
applicable to builtins in the `__builtin` namespace, which describes most of 
the builtins with custom typechecking.  Is the problem just `__va_start`?

If we have to treat all declarations as builtins for the custom-typechecking 
builtins just to land this patch, I don't think that's the worst result in the 
world, and we can incrementally go from there.  `__va_start` actually has a 
signature, it just effectively has optional arguments, which is something we 
could definitely teach the builtins database and signature-matcher about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D77491#2254065 , @rjmccall wrote:

> The builtins with custom type-checking are all true intrinsics like 
> `__builtin_operator_new` and so on.  They really can't be validly declared by 
> the user program.  The thing that seems most likely to avoid random compiler 
> crashes would be to either forbid explicit declarations of them or treat 
> those as no longer being builtins.  If we need to maintain compatibility with 
> people making custom declarations, we would need to always treat them as 
> builtins and run the risk of crashing if someone declares one with a bad 
> signature.  But I don't think it's unfair of us to break those people; that 
> is seriously not reasonable user behavior.
>
> It's possible that custom declarations are people trying to create 
> compatibility shims for compilers that don't provide these as builtins.  
> Those people should be guarding their custom declarations, preferably with 
> `__has_builtin`.

I fully agree.

However, I believe you forget to account for the example that I brought up.
In particular, MSVC's header `vadefs.h` includes a declaration of 
`__va_start()`, which would cause almost any code including standard headers to 
fail to compile on Windows.
This issue isn't isolated to some old MSVC version, as the declaration is still 
there in the latest Visual Studio Preview version 14.28.29213.

How about turning it into an error only on non-Windows?
Though keeping that as a followup might be even better, as this will probably 
be merged into 11.0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The builtins with custom type-checking are all true intrinsics like 
`__builtin_operator_new` and so on.  They really can't be validly declared by 
the user program.  The thing that seems most likely to avoid random compiler 
crashes would be to either forbid explicit declarations of them or treat those 
as no longer being builtins.  If we need to maintain compatibility with people 
making custom declarations, we would need to always treat them as builtins and 
run the risk of crashing if someone declares one with a bad signature.  But I 
don't think it's unfair of us to break those people; that is seriously not 
reasonable user behavior.

It's possible that custom declarations are people trying to create 
compatibility shims for compilers that don't provide these as builtins.  Those 
people should be guarding their custom declarations, preferably with 
`__has_builtin`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Thanks for the review!
I believe I've managed to address your comments.

In D77491#2248454 , @rsmith wrote:

> What happens for builtins with the "t" (custom typechecking) flag, for which 
> the signature is intended to have no meaning? Do we always give them builtin 
> semantics, or never, or ... something else? I think it might be reasonable to 
> require them to always be declared as taking unspecified arguments -- `()` in 
> C and `(...)` in C++, or to simply say that the user cannot declare such 
> functions themselves.

The options I see are:

1. Disallow.
2. Run the custom typechecking.
3. Require as taking unspecified arguments.
4. Simply check the name.

Implementing running custom typechecking seems it'd be tricky and not worth it.
Disallowing isn't an option as a cursory search shows that this is used in the 
wild.
Requiring unspecified arguments isn't feasible either as the real-world usages 
seem to include arguments in their declarations. E.g. __va_start from MSVC's 
vadefs.h 
,
 which is also tested by `clang/test/SemaCXX/microsoft-varargs.cpp`.

I've thus gone with simply marking as them builtins if the name matches and 
they're `extern "C"`. The C linkage requirement didn't exist before, but seems 
a safe improvement to make.




Comment at: clang/lib/Headers/intrin.h:148
 void __writemsr(unsigned long, unsigned __int64);
-static __inline__
-void *_AddressOfReturnAddress(void);
-static __inline__
-unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
-static __inline__
-unsigned char _BitScanReverse(unsigned long *_Index, unsigned long _Mask);
+__inline__ void *_AddressOfReturnAddress(void);
+__inline__ unsigned char _BitScanForward(unsigned long *_Index,

rsmith wrote:
> Does the `__inline__` here do anything for a builtin function? Can we remove 
> it along with the `static`?
It does not. Removed from all.



Comment at: clang/lib/Headers/intrin.h:200
 void __incgsword(unsigned long);
-static __inline__
-void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__ void __movsq(unsigned long long *, unsigned long long const 
*,
+   size_t);

rsmith wrote:
> Why is `static` being removed from some of the functions in this header but 
> not others?
I removed `static` from builtins that showed up as problematic in tests. But 
like `inline`, there's really no effect. I've removed both from all builtins in 
this header.



Comment at: clang/lib/Sema/SemaDecl.cpp:9668-9694
+  // In C builtins get merged with implicitly lazily created declarations.
+  // In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+  if (getLangOpts().CPlusPlus) {
+if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+FunctionDecl *D = CreateBuiltin(II, BuiltinID, NewFD->getLocation());
+

rsmith wrote:
> I think this needs more refinement:
> 
> * You appear to be creating and throwing away a new builtin function 
> declaration (plus parameter declarations etc) each time you see a declaration 
> with a matching name, even if one was already created. Given that you don't 
> actually use `D` for anything other than its type, creating the declaration 
> seems redundant and using `ASTContext::GetBuiltinType` would be more 
> appropriate.
> * There are no checks of which scope the new function is declared in; this 
> appears to apply in all scopes, but some builtin names are only reserved in 
> the global scope (those beginning with an underscore followed by a lowercase 
> letter such as `_bittest`), so that doesn't seem appropriate. The old code in 
> `FunctionDecl::getBuiltinID` checked that the declaration is given C language 
> linkage (except for `_GetExceptionInfo`, which was permitted to have C++ 
> language linkage), and that check still seems appropriate to me.
> * The special case permitting `_GetExceptionInfo` to be declared with *any* 
> type seems suspect; the old code permitted it to have different language 
> linkage, not the wrong type.
> * Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't 
> have a notion of "compatible types".
> * You appear to be creating and throwing away a new builtin function 
> declaration (plus parameter declarations etc) each time you see a declaration 
> with a matching name, even if one was already created. Given that you don't 
> actually use `D` for anything other than its type, creating the declaration 
> seems redundant and using `ASTContext::GetBuiltinType` would be more 
> appropriate.
Fixed.

> * There are no checks of which scope the new function is declared 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 289631.
tambre marked 6 inline comments as done.
tambre added a comment.

Remove __inline__ and static from all builtins in intrin.h.
Remove ASTReader/ASTWriter placeholders.
During builtin recognition:

- Use ASTContext::GetBuiltinType() instead of creating a new function 
declaration every time.
- Check for C linkage.
- Handle builtins with custom typechecking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2246948 , @rjmccall wrote:

> I'd still like @rsmith to sign off here as code owner.

Generally, I'm happy with this direction.

What happens for builtins with the "t" (custom typechecking) flag, for which 
the signature is intended to have no meaning? Do we always give them builtin 
semantics, or never, or ... something else? I think it might be reasonable to 
require them to always be declared as taking unspecified arguments -- `()` in C 
and `(...)` in C++, or to simply say that the user cannot declare such 
functions themselves.




Comment at: clang/lib/Headers/intrin.h:148
 void __writemsr(unsigned long, unsigned __int64);
-static __inline__
-void *_AddressOfReturnAddress(void);
-static __inline__
-unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
-static __inline__
-unsigned char _BitScanReverse(unsigned long *_Index, unsigned long _Mask);
+__inline__ void *_AddressOfReturnAddress(void);
+__inline__ unsigned char _BitScanForward(unsigned long *_Index,

Does the `__inline__` here do anything for a builtin function? Can we remove it 
along with the `static`?



Comment at: clang/lib/Headers/intrin.h:200
 void __incgsword(unsigned long);
-static __inline__
-void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__ void __movsq(unsigned long long *, unsigned long long const 
*,
+   size_t);

Why is `static` being removed from some of the functions in this header but not 
others?



Comment at: clang/lib/Sema/SemaDecl.cpp:9668-9694
+  // In C builtins get merged with implicitly lazily created declarations.
+  // In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+  if (getLangOpts().CPlusPlus) {
+if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+FunctionDecl *D = CreateBuiltin(II, BuiltinID, NewFD->getLocation());
+

I think this needs more refinement:

* You appear to be creating and throwing away a new builtin function 
declaration (plus parameter declarations etc) each time you see a declaration 
with a matching name, even if one was already created. Given that you don't 
actually use `D` for anything other than its type, creating the declaration 
seems redundant and using `ASTContext::GetBuiltinType` would be more 
appropriate.
* There are no checks of which scope the new function is declared in; this 
appears to apply in all scopes, but some builtin names are only reserved in the 
global scope (those beginning with an underscore followed by a lowercase letter 
such as `_bittest`), so that doesn't seem appropriate. The old code in 
`FunctionDecl::getBuiltinID` checked that the declaration is given C language 
linkage (except for `_GetExceptionInfo`, which was permitted to have C++ 
language linkage), and that check still seems appropriate to me.
* The special case permitting `_GetExceptionInfo` to be declared with *any* 
type seems suspect; the old code permitted it to have different language 
linkage, not the wrong type.
* Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't 
have a notion of "compatible types".



Comment at: clang/lib/Serialization/ASTReader.cpp:975
   bool HasRevertedTokenIDToIdentifier = readBit(Bits);
-  bool HasRevertedBuiltin = readBit(Bits);
+  readBit(Bits); // Previously used to indicate reverted builtin.
   bool Poisoned = readBit(Bits);

We don't have any stability guarantees for our AST bitcode format yet; you can 
just remove this bit rather than retaining a placeholder.



Comment at: clang/test/Analysis/bstring.cpp:106
   Derived d;
-  memset(_mem, 0, sizeof(Derived));
+  memset(_mem, 0, sizeof(Derived)); // expected-warning {{'memset' will 
always overflow; destination buffer has size 4, but size argument is 8}}
   clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}

This should not be recognized as a builtin, because the `memset` function is 
not `extern "C"`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Serialization/ASTWriter.cpp:3387
 Bits = (Bits << 1) | unsigned(II->isPoisoned());
-Bits = (Bits << 1) | unsigned(II->hasRevertedBuiltin());
+Bits <<= 1; // Previously used to indicate reverted builtin.
 Bits = (Bits << 1) | unsigned(II->hasRevertedTokenIDToIdentifier());

Clang bitcode isn't really a stable format, it's fine to just drop this bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added inline comments.



Comment at: clang/include/clang/Basic/IdentifierTable.h:231
 return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS;
   }
 

rjmccall wrote:
> Do we need to support reverting builtins anymore?
We don't. It'd be possible to set the builtin ID of identifiers to `0` as an 
optimization to avoid declaration compatibility checking if there's a 
declaration that would hide the actual builtin. But I doubt it's worth it.

I've removed code related to this, as nothing was actually checking the 
reverted status anymore.



Comment at: clang/test/CodeGen/callback_pthread_create.c:3
+// RUN: false
+// XFAIL: *
+

rjmccall wrote:
> I guess the problem with pthread_create is that the types are not really 
> reasonable to synthesize.  I wonder if we can use an approach more like what 
> we do with C++, where we don't magically synthesize a declaration but where 
> we do recognize that a particular declaration is compatible with the builtin 
> signature.
Seems like a good idea. Would also reduce code duplication between C and C++. I 
would be willing to look into that as a followup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 288911.
tambre marked 2 inline comments as done.
tambre added a comment.

Remove builtin reverting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Analysis/bstring.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'd still like @rsmith to sign off here as code owner.




Comment at: clang/include/clang/Basic/IdentifierTable.h:231
 return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS;
   }
 

Do we need to support reverting builtins anymore?



Comment at: clang/test/CodeGen/callback_pthread_create.c:3
+// RUN: false
+// XFAIL: *
+

I guess the problem with pthread_create is that the types are not really 
reasonable to synthesize.  I wonder if we can use an approach more like what we 
do with C++, where we don't magically synthesize a declaration but where we do 
recognize that a particular declaration is compatible with the builtin 
signature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

FYI: PR45410, which this fixes, has been marked as a release blocker for 11.0.0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Please review. I would like to get this in a mergeable state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 288792.
tambre added a comment.
Herald added subscribers: danielkiss, jfb.

Rebase, fix new MS builtins and other tests, XFAIL pthread_create() test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Analysis/bstring.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
 
-// CHECK: FunctionDecl 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 267450.
tambre marked 4 inline comments as done.
tambre added a comment.

Fix some intrinsics not being marked as builtin due to being static in the 
headers.
Make some code easier to read, fix test for sigsetjmp in 
Sema/implicit-builtin-decl.c to reflect the original intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Headers/intrin.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
@@ -63,9 +62,5 @@
 struct __jmp_buf_tag {};
 void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre marked an inline comment as done.
tambre added inline comments.



Comment at: clang/lib/AST/Decl.cpp:3169-3175
   } else {
-if (!getIdentifier())
+const auto *Attr = getAttr();
+
+if (!Attr)
   return 0;
 
+BuiltinID = Attr->getID();

aaron.ballman wrote:
> I think this is a bit more clear:
> ```
> } else if (const auto *A = getAttr()) {
>   BuiltinID = A->getID();
> }
> ```
> and initialize `BuiltinID` to zero above.
Done.



Comment at: clang/test/CodeGen/callback_pthread_create.c:17
 
+// FIXME: How to do builtin handling for this?
 int pthread_create(pthread_t *, const pthread_attr_t *,

As many others prior to this patch, `pthread_create` was recognized as a 
builtin due to its name and thus had attributes applied.
Unlike others however, `pthread_create` is the only builtin in `Builtins.def` 
that doesn't have its arguments specified. Doing that would require 
implementing support for function pointers in the builtin database and adding 
yet another special case for `pthread_t` and `pthread_attr_t`.
That'd be quite a bit of work, which I'm not interested in doing.

How about simply removing the hack that is the `pthread_create` builtin entry 
and disabling/removing this test?



Comment at: clang/test/Sema/implicit-builtin-decl.c:64
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration 
of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' 
type, commonly provided in the header .}}
 

aaron.ballman wrote:
> It looks like we're losing test coverage with this change?
Indeed. I've reverted this change and changed the test to instead not test for 
it being recognized as a builtin, since it shouldn't be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/Decl.cpp:3169-3175
   } else {
-if (!getIdentifier())
+const auto *Attr = getAttr();
+
+if (!Attr)
   return 0;
 
+BuiltinID = Attr->getID();

I think this is a bit more clear:
```
} else if (const auto *A = getAttr()) {
  BuiltinID = A->getID();
}
```
and initialize `BuiltinID` to zero above.



Comment at: clang/test/Sema/implicit-builtin-decl.c:64
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration 
of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' 
type, commonly provided in the header .}}
 

It looks like we're losing test coverage with this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+  }
+}
+

rjmccall wrote:
> tambre wrote:
> > rjmccall wrote:
> > > Hmm.  I'm concerned about not doing any sort of semantic compatibility 
> > > check here before we assign the function special semantics.  Have we 
> > > really never done those in C++?
> > > 
> > > If not, I wonder if we can reasonably make an implicit declaration but 
> > > just make it hidden from lookup.
> > Currently there's no semantic compatibility checking for builtin 
> > redeclarations. There is for initial declarations though.
> > 
> > I've added this checking by splitting the actual builtin declaration 
> > creation off from `LazilyCreateBuiltin` into `CreateBuiltin` and checking 
> > if the current declaration is compatible with what the builtin's would be.
> > This results in stronger typechecking than before for builtin declarations, 
> > so some incompatible declarations are no longer marked as builtin. See 
> > `cxx1z-noexcept-function-type.cpp` for an example.
> That makes sense to me in principle.  I'm definitely concerned about 
> `noexcept` differences causing C library functions to not be treated as 
> builtins, though; that seems stricter than we want.  How reasonable is it to 
> weaken this?
I agree having `noexcept` weakened is reasonable.
I've changed it to create an identical type to the NewFD with the exception 
spec removed for the comparison. This fixes it.



Comment at: clang/test/CodeGen/ms-intrinsics.c:23
 void test__stosb(unsigned char *Dest, unsigned char Data, size_t Count) {
   return __stosb(Dest, Data, Count);
 }

`__stosb` and friends aren't marked as builtin because they're declared as 
`static`.
I don't think there's a good reason to have builtins as `static` and we should 
simply remove the `static` specifier from those intrinsics in headers.
Alternatively, we could weaken compatibility checking similar to `noexcept`.
Thoughts?



Comment at: clang/test/Sema/warn-fortify-source.c:20
 void *memcpy(void *dst, const void *src, size_t c);
 static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void 
*src, size_t c) __attribute__((overloadable)) __asm__("merp");
 static void *memcpy(void *const dst __attribute__((pass_object_size(1))), 
const void *src, size_t c) __attribute__((overloadable)) {

rjmccall wrote:
> tambre wrote:
> > Not quite sure what to do here. These were previously recognized as 
> > builtins due to their name despite being incompatible and thus had fortify 
> > checking similar to the real `memcpy`.
> > 
> > Maybe:
> > 1. Introduce a generic version of `ArmBuiltinAliasAttr`.
> > 2. Something like `FormatAttr`.
> That's interesting.  It definitely seems wrong to apply builtin logic to a 
> function that doesn't have a compatible low-level signature.  My inclination 
> is to disable builtin checking here, but to notify the contributors so that 
> they can figure out an appropriate response.
Agreed.
I've removed this test, as there doesn't seem to be an easy way to replicate 
this behaviour.



Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:120
   extern "C" int strcmp(const char *, const char *);
-  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0))) 
noexcept;
+  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0)));
 

tambre wrote:
> This doesn't work anymore since we now properly check builtin declaration 
> compatibility and since C++17 noexcept is part of the function type but 
> builtins aren't noexcept.
> Thoughts?
Fixed by removing `noexcept` for the declaration compatibility comparison.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 265871.
tambre marked 6 inline comments as done.
tambre added a comment.

Weakened noexcept checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGen/ms-intrinsics.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,10 +1,12 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
+// FIXME: Incompatible builtin redeclarations aren't considered builtins and thus don't call the builtin nor inherit their attributes.
+// %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+// %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+
 typedef unsigned long size_t;
 
 #ifdef __cplusplus
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -60,12 +60,15 @@
 
 extern float fmaxf(float, float);
 
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+typedef struct __jmp_buf_tag {
+} sigjmp_buf[1];
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
+int sigsetjmp(struct __jmp_buf_tag[1], int);
+
+// CHECK: FunctionDecl {{.*}}  col:5 implicit sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 sigsetjmp '
 // CHECK-NOT: FunctionDecl
-// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Inherited Implicit
 
 // PR40692
 void pthread_create(); // no warning expected
Index: clang/test/CodeGen/ms-intrinsics.c
===
--- clang/test/CodeGen/ms-intrinsics.c
+++ clang/test/CodeGen/ms-intrinsics.c
@@ -17,6 +17,7 @@
 
 #include 
 
+// FIXME: Why aren't these builtins recognized as builtins?
 #if defined(__i386__) || defined(__x86_64__)
 void test__stosb(unsigned char *Dest, unsigned char Data, size_t Count) {
   return __stosb(Dest, Data, Count);
Index: clang/test/CodeGen/callback_pthread_create.c
===
--- clang/test/CodeGen/callback_pthread_create.c
+++ clang/test/CodeGen/callback_pthread_create.c
@@ -14,6 +14,7 @@
 typedef __darwin_pthread_t pthread_t;
 typedef __darwin_pthread_attr_t pthread_attr_t;
 
+// FIXME: Why isn't this 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 265872.
tambre marked 4 inline comments as done.
tambre added a comment.

Remove memcpy overload tests from warn-fortify-source.c, which relied on 
identifier-based builtin identification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGen/ms-intrinsics.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
 typedef unsigned long size_t;
@@ -13,13 +11,7 @@
 
 extern int sprintf(char *str, const char *format, ...);
 
-#if defined(USE_PASS_OBJECT_SIZE)
-void *memcpy(void *dst, const void *src, size_t c);
-static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
-static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
-  return 0;
-}
-#elif defined(USE_BUILTINS)
+#if defined(USE_BUILTINS)
 #define memcpy(x,y,z) __builtin_memcpy(x,y,z)
 #else
 void *memcpy(void *dst, const void *src, size_t c);
@@ -45,14 +37,7 @@
   };
   struct pair p;
   char buf[20];
-  memcpy(, buf, 20);
-#ifdef USE_PASS_OBJECT_SIZE
-  // Use the more strict checking mode on the pass_object_size attribute:
-  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
-#else
-  // Or just fallback to type 0:
-  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
-#endif
+  memcpy(, buf, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
 }
 
 void call_strncat() {
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -60,12 +60,15 @@
 
 extern float fmaxf(float, float);
 
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+typedef struct __jmp_buf_tag {
+} sigjmp_buf[1];
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
+int sigsetjmp(struct __jmp_buf_tag[1], int);
+
+// CHECK: FunctionDecl {{.*}}  col:5 implicit sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 sigsetjmp '
 // 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+  }
+}
+

tambre wrote:
> rjmccall wrote:
> > Hmm.  I'm concerned about not doing any sort of semantic compatibility 
> > check here before we assign the function special semantics.  Have we really 
> > never done those in C++?
> > 
> > If not, I wonder if we can reasonably make an implicit declaration but just 
> > make it hidden from lookup.
> Currently there's no semantic compatibility checking for builtin 
> redeclarations. There is for initial declarations though.
> 
> I've added this checking by splitting the actual builtin declaration creation 
> off from `LazilyCreateBuiltin` into `CreateBuiltin` and checking if the 
> current declaration is compatible with what the builtin's would be.
> This results in stronger typechecking than before for builtin declarations, 
> so some incompatible declarations are no longer marked as builtin. See 
> `cxx1z-noexcept-function-type.cpp` for an example.
That makes sense to me in principle.  I'm definitely concerned about `noexcept` 
differences causing C library functions to not be treated as builtins, though; 
that seems stricter than we want.  How reasonable is it to weaken this?



Comment at: clang/test/Sema/warn-fortify-source.c:20
 void *memcpy(void *dst, const void *src, size_t c);
 static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void 
*src, size_t c) __attribute__((overloadable)) __asm__("merp");
 static void *memcpy(void *const dst __attribute__((pass_object_size(1))), 
const void *src, size_t c) __attribute__((overloadable)) {

tambre wrote:
> Not quite sure what to do here. These were previously recognized as builtins 
> due to their name despite being incompatible and thus had fortify checking 
> similar to the real `memcpy`.
> 
> Maybe:
> 1. Introduce a generic version of `ArmBuiltinAliasAttr`.
> 2. Something like `FormatAttr`.
That's interesting.  It definitely seems wrong to apply builtin logic to a 
function that doesn't have a compatible low-level signature.  My inclination is 
to disable builtin checking here, but to notify the contributors so that they 
can figure out an appropriate response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre marked 4 inline comments as done.
tambre added a comment.

Thanks for the reviews and design pointers, John!

There are still a few tests failing, but I'd rather not dive in before the 
general approach is considered acceptable.




Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+  }
+}
+

rjmccall wrote:
> Hmm.  I'm concerned about not doing any sort of semantic compatibility check 
> here before we assign the function special semantics.  Have we really never 
> done those in C++?
> 
> If not, I wonder if we can reasonably make an implicit declaration but just 
> make it hidden from lookup.
Currently there's no semantic compatibility checking for builtin 
redeclarations. There is for initial declarations though.

I've added this checking by splitting the actual builtin declaration creation 
off from `LazilyCreateBuiltin` into `CreateBuiltin` and checking if the current 
declaration is compatible with what the builtin's would be.
This results in stronger typechecking than before for builtin declarations, so 
some incompatible declarations are no longer marked as builtin. See 
`cxx1z-noexcept-function-type.cpp` for an example.



Comment at: clang/lib/Sema/SemaExpr.cpp:6047
   OverloadDecl->setParams(Params);
+  Sema->mergeDeclAttributes(OverloadDecl, FDecl);
   return OverloadDecl;

`rewriteBuiltinFunctionDecl()` creates a new function declaration and happened 
to disregard the builtin's attributes. This caused `BuiltinAttr` to go missing 
in a bunch of OpenCL tests, resulting in failures.



Comment at: clang/test/Sema/warn-fortify-source.c:20
 void *memcpy(void *dst, const void *src, size_t c);
 static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void 
*src, size_t c) __attribute__((overloadable)) __asm__("merp");
 static void *memcpy(void *const dst __attribute__((pass_object_size(1))), 
const void *src, size_t c) __attribute__((overloadable)) {

Not quite sure what to do here. These were previously recognized as builtins 
due to their name despite being incompatible and thus had fortify checking 
similar to the real `memcpy`.

Maybe:
1. Introduce a generic version of `ArmBuiltinAliasAttr`.
2. Something like `FormatAttr`.



Comment at: clang/test/SemaCXX/cxx11-compat.cpp:35
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;

For some reason this `printf` didn't get format checking before. Same for 
`SemaCXX/warn-unused-local-typedef.cpp`.



Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:120
   extern "C" int strcmp(const char *, const char *);
-  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0))) 
noexcept;
+  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0)));
 

This doesn't work anymore since we now properly check builtin declaration 
compatibility and since C++17 noexcept is part of the function type but 
builtins aren't noexcept.
Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 264497.
tambre marked an inline comment as not done.
tambre added a comment.

Semantic compatibility checking for C++ builtin redeclarations.
Remove some now unnecessary logic from getBuiltinID().
Update more tests. 4 tests still failing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  clang/test/CodeGen/callback_pthread_create.c
  clang/test/CodeGen/ms-intrinsics.c
  clang/test/Sema/implicit-builtin-decl.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/SemaCXX/cxx11-compat.cpp
  clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
  clang/test/SemaCXX/warn-unused-local-typedef.cpp

Index: clang/test/SemaCXX/warn-unused-local-typedef.cpp
===
--- clang/test/SemaCXX/warn-unused-local-typedef.cpp
+++ clang/test/SemaCXX/warn-unused-local-typedef.cpp
@@ -67,10 +67,10 @@
 
 void test() {
   typedef signed long int superint; // no diag
-  printf("%f", (superint) 42);
+  printf("%ld", (superint)42);
 
   typedef signed long int superint2; // no diag
-  printf("%f", static_cast(42));
+  printf("%ld", static_cast(42));
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-local-typedef"
Index: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
===
--- clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
+++ clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
@@ -117,7 +117,7 @@
 namespace Builtins {
   // Pick two functions that ought to have the same noexceptness.
   extern "C" int strcmp(const char *, const char *);
-  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0))) noexcept;
+  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0)));
 
   // Check we recognized both as builtins.
   typedef int arr[strcmp("bar", "foo") + 4 * strncmp("foo", "bar", 4)];
Index: clang/test/SemaCXX/cxx11-compat.cpp
===
--- clang/test/SemaCXX/cxx11-compat.cpp
+++ clang/test/SemaCXX/cxx11-compat.cpp
@@ -31,7 +31,7 @@
 s = { n }, // expected-warning {{non-constant-expression cannot be narrowed from type 'int' to 'char' in initializer list in C++11}} expected-note {{explicit cast}}
 t = { 1234 }; // expected-warning {{constant expression evaluates to 1234 which cannot be narrowed to type 'char' in C++11}} expected-warning {{changes value}} expected-note {{explicit cast}}
 
-#define PRIuS "uS"
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
 void h(size_t foo, size_t bar) {
Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -1,10 +1,12 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
-// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 
+// FIXME: Incompatible builtin redeclarations aren't considered builtins and thus don't call the builtin nor inherit their attributes.
+// %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+// %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+
 typedef unsigned long size_t;
 
 #ifdef __cplusplus
Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -60,12 +60,15 @@
 
 extern float fmaxf(float, float);
 
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+typedef struct __jmp_buf_tag {
+} sigjmp_buf[1];
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
+int sigsetjmp(struct __jmp_buf_tag[1], int);
+
+// CHECK: FunctionDecl {{.*}}  col:5 implicit sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 sigsetjmp '
 // CHECK-NOT: FunctionDecl
-// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

@rsmith This is a big enough architectural change that I'd appreciate your 
sign-off on the basic approach.




Comment at: clang/lib/Sema/SemaDecl.cpp:3759
   unsigned BuiltinID;
-  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
+  bool RevertedBuiltin{Old->getIdentifier()->hasRevertedBuiltin()};
+  if (Old->isImplicit() &&

rjmccall wrote:
> The old code didn't check this eagerly.  The `Old->isImplicit()` check is 
> unlikely to fire; please make sure that we don't do any extra work unless it 
> does.
Since BuiltinAttr is inherited, we should keep the isImplicit check, because we 
only want to use a special diagnostic when "redeclaring" the implicit builtin 
declaration.



Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+  }
+}
+

Hmm.  I'm concerned about not doing any sort of semantic compatibility check 
here before we assign the function special semantics.  Have we really never 
done those in C++?

If not, I wonder if we can reasonably make an implicit declaration but just 
make it hidden from lookup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491



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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 264447.
tambre marked 3 inline comments as done.
tambre added a comment.

Fix adding BuiltinAttr in C++ mode. Update one test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  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
@@ -60,12 +60,15 @@
 
 extern float fmaxf(float, float);
 
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+typedef struct __jmp_buf_tag {
+} sigjmp_buf[1];
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
+int sigsetjmp(struct __jmp_buf_tag[1], int);
+
+// CHECK: FunctionDecl {{.*}}  col:5 implicit sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 sigsetjmp '
 // CHECK-NOT: FunctionDecl
-// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Inherited Implicit
 
 // PR40692
 void pthread_create(); // no warning expected
Index: clang/test/CodeGen/builtin-redeclaration.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-redeclaration.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -emit-llvm-only %s
+
+// PR45410
+// Ensure we mark local extern redeclarations with a different type as non-builtin.
+void non_builtin() {
+  extern float exp();
+  exp(); // Will crash due to wrong number of arguments if this calls the builtin.
+}
+
+// PR45410
+// We mark exp() builtin as const with -fno-math-errno (default).
+// We mustn't do that for extern redeclarations of builtins where the type differs.
+float attribute() {
+  extern float exp();
+  return exp(1);
+}
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -109,6 +109,7 @@
 extern "C" int printf(const char *format, ...);
 // CHECK: FunctionDecl{{.*}}printf
 // CHECK-NEXT: ParmVarDecl{{.*}}format{{.*}}'const char *'
+// CHECK-NEXT: BuiltinAttr{{.*}}Implicit
 // CHECK-NEXT: FormatAttr{{.*}}Implicit printf 1 2
 
 alignas(8) extern int x;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2108,6 +2108,7 @@
false,
R->isFunctionProtoType());
   New->setImplicit();
+  New->addAttr(BuiltinAttr::CreateImplicit(Context, ID));
 
   // Create Decl objects for each parameter, adding them to the
   // FunctionDecl.
@@ -3330,7 +3331,11 @@
   // there but not here.
   NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
   RequiresAdjustment = true;
-} else if (New->getBuiltinID()) {
+} else if (Old->getBuiltinID()) {
+  // Builtin attribute isn't propagated to the new one yet at this point.
+  // Check if the old is a builtin.
+  // TODO: Maybe we should only warn if the redeclaration is compatible?
+
   // Calling Conventions on a Builtin aren't really useful and setting a
   // default calling convention and cdecl'ing some builtin redeclarations is
   // common, so warn and ignore the calling convention on the redeclaration.
@@ -3756,6 +3761,7 @@
   // If the previous declaration was an implicitly-generated builtin
   // declaration, then at the very least we should use a specialized note.
   unsigned BuiltinID;
+  // TODO: Remove isImplicit check?
   if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
 // If it's actually a library-defined builtin function like 'malloc'
 // or 'printf', just warn about the incompatible redeclaration.
@@ -8859,6 +8865,20 @@
 if (D.isInvalidType())
   NewFD->setInvalidDecl();
 
+// In C builtins get merged with implicitly lazily created declarations.
+// In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+  if (II->getBuiltinID()) {
+// A builtin needs to have C linkage.
+LinkageSpecDecl *linkage =
+dyn_cast(NewFD->getFirstDecl()->getDeclContext());
+if (linkage && linkage->getLanguage() == LinkageSpecDecl::lang_c) {
+  NewFD->addAttr(
+  BuiltinAttr::CreateImplicit(Context, II->getBuiltinID()));
+}
+  

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre updated this revision to Diff 264441.
tambre added a comment.
Herald added a reviewer: aaron.ballman.

Rework builtin declaration handling. Introduce BuiltinAttr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/builtin-redeclaration.c
  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
@@ -60,12 +60,16 @@
 
 extern float fmaxf(float, float);
 
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+typedef struct __jmp_buf_tag
+{
+} sigjmp_buf[1];
+
+int sigsetjmp(struct __jmp_buf_tag[1], int);
 
-// CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 implicit sigsetjmp '
+// CHECK: FunctionDecl {{.*}}  col:5 sigsetjmp '
 // CHECK-NOT: FunctionDecl
-// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Inherited Implicit
 
 // PR40692
 void pthread_create(); // no warning expected
Index: clang/test/CodeGen/builtin-redeclaration.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-redeclaration.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -emit-llvm-only %s
+
+// PR45410
+// Ensure we mark local extern redeclarations with a different type as non-builtin.
+void non_builtin() {
+  extern float exp();
+  exp(); // Will crash due to wrong number of arguments if this calls the builtin.
+}
+
+// PR45410
+// We mark exp() builtin as const with -fno-math-errno (default).
+// We mustn't do that for extern redeclarations of builtins where the type differs.
+float attribute() {
+  extern float exp();
+  return exp(1);
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2108,6 +2108,7 @@
false,
R->isFunctionProtoType());
   New->setImplicit();
+  New->addAttr(BuiltinAttr::CreateImplicit(Context, ID));
 
   // Create Decl objects for each parameter, adding them to the
   // FunctionDecl.
@@ -3330,7 +3331,11 @@
   // there but not here.
   NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
   RequiresAdjustment = true;
-} else if (New->getBuiltinID()) {
+} else if (Old->getBuiltinID()) {
+  // Builtin attribute isn't propagated to the new one yet at this point.
+  // Check if the old is a builtin.
+  // TODO: Maybe we should only warn if the redeclaration is compatible?
+
   // Calling Conventions on a Builtin aren't really useful and setting a
   // default calling convention and cdecl'ing some builtin redeclarations is
   // common, so warn and ignore the calling convention on the redeclaration.
@@ -3756,6 +3761,7 @@
   // If the previous declaration was an implicitly-generated builtin
   // declaration, then at the very least we should use a specialized note.
   unsigned BuiltinID;
+  // TODO: Remove isImplicit check?
   if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
 // If it's actually a library-defined builtin function like 'malloc'
 // or 'printf', just warn about the incompatible redeclaration.
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3164,10 +3164,12 @@
   if (const auto *ABAA = getAttr()) {
 BuiltinID = ABAA->getBuiltinName()->getBuiltinID();
   } else {
-if (!getIdentifier())
+const auto *Attr = getAttr();
+
+if (!Attr)
   return 0;
 
-BuiltinID = getIdentifier()->getBuiltinID();
+BuiltinID = Attr->getID();
   }
 
   if (!BuiltinID)
Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -225,7 +225,7 @@
   }
   void setObjCKeywordID(tok::ObjCKeywordKind ID) { ObjCOrBuiltinID = ID; }
 
-  /// True if setNotBuiltin() was called.
+  /// True if revertBuiltin() was called.
   bool hasRevertedBuiltin() const {
 return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS;
   }
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -3440,3