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

[Erich Keane via Phabricator via llvm-commits]

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