[PATCH] D129572: [X86] initial -mfunction-return=thunk-extern support

[Nick Desaulniers via Phabricator via llvm-commits]

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