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

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

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

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.

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

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 SINC

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

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 alt

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

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 >

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

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 function

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

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

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

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(stru

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

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]; #defin

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

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

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

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"

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

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_msgSe

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

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

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

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. Reposi

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

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 builti

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

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 link

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

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/B

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

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 + Cont

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

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 L

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

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-bi

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

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->getDecl

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

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/includ

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

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/

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

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 cha

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

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 typech

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

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/

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

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

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

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 m

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

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 dec

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

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 meani

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

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 n

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

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

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

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 =

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

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 built

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

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

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

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? ==

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

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-c

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

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

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

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:

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

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 o

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

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(); aaro

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

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: ```

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

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 fu

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

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/inclu

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

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 http

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

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 semanti

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

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:888

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

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

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

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 Rev

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

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/D77

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

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.o