nickdesaulniers added a comment.
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.
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
