erichkeane added a comment. In D129572#3645917 <https://reviews.llvm.org/D129572#3645917>, @nickdesaulniers wrote:
> In D129572#3645895 <https://reviews.llvm.org/D129572#3645895>, @erichkeane > wrote: > >> Did a post-commit review on the CFE changes, and all look OK to me. > > Thanks for the review! > >> That FIXME is a shame, we should see if we can fix that ASAP. We should AT >> LEAST document in the FIXME what semantics we are looking to emulate from >> GCC, and what those semantics ARE. > > Eh, so the discussion I had off list with @aaron.ballman was: > > "Ok, here's where I feel like I now have a catch-22. I now have: > > diff --git a/clang/include/clang/Sema/Sema.h > b/clang/include/clang/Sema/Sema.h > index 7d33b5047a67..73afedad4a9d 100644 > --- a/clang/include/clang/Sema/Sema.h > +++ b/clang/include/clang/Sema/Sema.h > @@ -3496,6 +3496,9 @@ public: > int X, int Y, int Z); > HLSLShaderAttr *mergeHLSLShaderAttr(Decl *D, const AttributeCommonInfo > &AL, > HLSLShaderAttr::ShaderType > ShaderType); > + FunctionReturnThunksAttr * > + mergeFunctionReturnThunksAttr(Decl *D, const AttributeCommonInfo &AL, > + const FunctionReturnThunksAttr::Kind K); > > void mergeDeclAttributes(NamedDecl *New, Decl *Old, > AvailabilityMergeKind AMK = AMK_Redeclaration); > diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp > index f2b87c6d2e37..7e9507735b93 100644 > --- a/clang/lib/Sema/SemaDecl.cpp > +++ b/clang/lib/Sema/SemaDecl.cpp > @@ -2809,6 +2809,8 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D, > S.mergeHLSLNumThreadsAttr(D, *NT, NT->getX(), NT->getY(), > NT->getZ()); > else if (const auto *SA = dyn_cast<HLSLShaderAttr>(Attr)) > NewAttr = S.mergeHLSLShaderAttr(D, *SA, SA->getType()); > + else if (const auto *FA = dyn_cast<FunctionReturnThunksAttr>(Attr)) > + NewAttr = S.mergeFunctionReturnThunksAttr(D, *FA, FA->getThunkType()); > else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, > Attr)) > NewAttr = cast<InheritableAttr>(Attr->clone(S.Context)); > diff --git a/clang/lib/Sema/SemaDeclAttr.cpp > b/clang/lib/Sema/SemaDeclAttr.cpp > index 9b127fbc395b..69c18a48680a 100644 > --- a/clang/lib/Sema/SemaDeclAttr.cpp > +++ b/clang/lib/Sema/SemaDeclAttr.cpp > @@ -6973,6 +6973,19 @@ Sema::mergeHLSLShaderAttr(Decl *D, const > AttributeCommonInfo &AL, > return HLSLShaderAttr::Create(Context, ShaderType, AL); > } > > +FunctionReturnThunksAttr * > +Sema::mergeFunctionReturnThunksAttr(Decl *D, const AttributeCommonInfo &AL, > + const FunctionReturnThunksAttr::Kind > K) { > + if (const auto *FRT = D->getAttr<FunctionReturnThunksAttr>()) { > + if (FRT->getThunkType() != K) { > + Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL; > + Diag(FRT->getLoc(), diag::note_conflicting_attribute); > + } > + return nullptr; > + } > + return FunctionReturnThunksAttr::Create(Context, K, AL); > +} > + > static void handleMSInheritanceAttr(Sema &S, Decl *D, const ParsedAttr > &AL) { > if (!S.LangOpts.CPlusPlus) { > S.Diag(AL.getLoc(), diag::err_attribute_not_supported_in_lang) > @@ -8043,8 +8056,10 @@ static void handleFunctionReturnThunksAttr(Sema > &S, Decl *D, > << AL << KindStr; > return; > } > - D->dropAttr<FunctionReturnThunksAttr>(); > - D->addAttr(FunctionReturnThunksAttr::Create(S.Context, Kind, AL)); > + > + if (FunctionReturnThunksAttr *FA = > + S.mergeFunctionReturnThunksAttr(D, AL, Kind)) > + D->addAttr(FA); > } > > static void handleSYCLKernelAttr(Sema &S, Decl *D, const ParsedAttr &AL) { > > Consider the following 2 cases. > > Case 1: > > __attribute__((function_return("thunk-extern"))) void f(void); > __attribute__((function_return("keep"))) void f(void) {}; > > $ clang z.c > z.c:1:16: warning: attribute 'function_return' is already applied with > different arguments [-Wignored-attributes] > __attribute__((function_return("thunk-extern"))) void f(void); > ^ > z.c:2:16: note: conflicting attribute is here > __attribute__((function_return("keep"))) void f(void) {}; > ^ > > This LGTM; we want to keep the latest version. So the warning is on > the initial declaration that its attribute will be ignored, and we > point to the definition to identify the conflict. LGTM. > > But now consider this case: > > __attribute__((function_return("thunk-extern"))) > __attribute__((function_return("keep"))) > void double_thunk_keep(void) {} > > $ clang z.c > z.c:18:16: warning: attribute 'function_return' is already applied > with different arguments [-Wignored-attributes] > __attribute__((function_return("keep"))) > ^ > z.c:17:16: note: conflicting attribute is here > __attribute__((function_return("thunk-extern"))) > ^ > > No bueno. We want to point to the first instance as the one being > ignored ("thunk-extern"), not ("keep"). > > If I flip the Diags around in my added > `Sema::mergeFunctionReturnThunksAttr()`, then the second case passes > but the first now fails...i.e. > > Diag(FRT->getLoc(), diag::warn_duplicate_attribute) << FRT; > Diag(AL.getLoc(), diag::note_conflicting_attribute); > > instead of > > Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL; > Diag(FRT->getLoc(), diag::note_conflicting_attribute); > > Any idea what I'm doing wrong? > > (The behavior of GCC is to keep the last occurence of this fn attr, > whether through multiple redeclarations, or one __attribute__ with > multiple instances of function_return). > > I feel like the semantics of the merging routines would be clearer if > they were passed the previous Decl, and the new Decl." > > --- > > Personally, I don't feel the need to fix attribute merging in clang "ASAP." > It does seem like a general problem, not specific to _this_ particular fn > attr. Hence the FIXME. Are we not evaluating attributes in lexical order? I could have sworn we fixed that not long ago. I think part of the issue is that you want to merge these 'differently' than we typically do. Our typical rule is to keep the 1st one I think, and reject the 2nd. I suspect you'd need a new diagnostic for this since this is a 'new' behavior. Something like, `attribute %0 replaces conflicting attribute on this declaration' (then the note). I think this is important to diagnose (which is why I said ASAP), as this seems like a somewhat easy thing to miss, and likely causes shocking/dangerous behavior when done 'wrong'. AND since this is a 'security concern' it moves from "we should fix this soon" to "ASAP" for me. The other open question for me is how we handle attributes-after-definition: __attribute__((function_return("keep"))) void foo(void){} __attribute__((function_return("thunk-extern"))) void foo(void); OR void foo(void){} __attribute__((function_return("thunk-extern"))) void foo(void); One COULD decide that the 'newest declaration' wins here, but, IMO that is the wrong answer. This makes something where mis-ordered #includes could change the behavior here in a shocking way, particularly without a diagnostic. _I_ believe that 'definition should win', but that isn't something we do with attributes generally I believe (though ones that have semantic effect might enforce this ad-hoc). ALSO, I wonder if the 'conflicting attribute' warning for THIS attribute might be better as an error, simply because of the security concerns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129572/new/ https://reviews.llvm.org/D129572 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
