Fix for bug 25786 - Assertion "Chunk.Kind == DeclaratorChunk::Function" failed with regparm attribute.

[• a.makarov]

Summary

This patch is to avoid attaching the calling convention attribute to the result type if this attribute was ignored and calling convention was substituted by default.

[rnk]

include/clang/Sema/Sema.h

2938

Rather than adding this extra entry point, I think it would be simpler to extend CheckCallingConvAttr to return whatever extra info we need.

Besides, this new entry point also emits diagnostics, and it has a name that suggests it will not emit diagnostics.

lib/Sema/SemaType.cpp

5866

Right, we've already done the work that you are doing down below. We should just get what we needed to know out of here and pass it on.

5925–5926

At a high level, why would you want to not create an AttributedType here? That seems desirable, the user wrote the type attribute in the source, and that should be reflected in the AST sugar.

If adding this sugar causes assertions later, then the bug must be somewhere else.

test/CodeGen/adding_defaulted_attr_to_type.c

6

You can add more tests. I noticed this crashes for me:

void (__attribute__((regparm(2), stdcall)) foo)(int);

That seems like a good one to add.

You can also rerun the test with i686-unknown-linux-gnu and #ifdefs to see that we accept the convention as desired on 32-bit.

[• a.makarov]

I've updated the patch. Please, re-review it again.
About creating attributed type - I think we shouldn't create it if we ignored specified CC attribute. Ignoring specified CC attribute (and emitting the warning, of course) leads to substituting it by default; calling convention, substituted by default, is the same situation like there is no CC attribute specified for chosen function. Thus, from this point of view, we should not create AttributedType.

[• a.makarov]

Actual updated patch. Sorry for noise.

[• a.makarov]

Ping

[aaron.ballman]

In D15373#315457, @a.makarov wrote:

I've updated the patch. Please, re-review it again.
About creating attributed type - I think we shouldn't create it if we ignored specified CC attribute. Ignoring specified CC attribute (and emitting the warning, of course) leads to substituting it by default; calling convention, substituted by default, is the same situation like there is no CC attribute specified for chosen function. Thus, from this point of view, we should not create AttributedType.

I tend to agree with @rnk on this -- it is a shame to lose that syntactic information in the AST representation (for instance, tools may wish to detect the presence of that attribute in the source to perform different analyses on the type). Even if it is semantically ignored, it is still something the user wrote. For instance, this means we cannot round-trip the user's source code through pretty printing, despite the code not being ill-formed.

test/CodeGen/adding_defaulted_cc_attr_to_type.c

1 ↗

(On Diff #43442)

Can you drop the svn props on the file? Our convention is to not rely on props, but instead save the file with UNIX line endings.

[• a.makarov]

Thanks for review! I've understood your idea, I'm working on updating the patch.

[• a.makarov]

I've updated the patch. Now, I've updated the function unwrapping mechanism to work properly with AttributedType.
Please, re-review the patch.

[• a.makarov]

Oops, forgot to drop svn properties. Done. Sorry for noise.

[aaron.ballman]

This looks correct to me, but my knowledge of type attributes isn't strong enough to officially sign off, so please wait for @rnk or @rsmith to take a look as well.

[• a.makarov]

Ping

[• a.makarov]

Richard, Reid, please take a look.

[rnk]

Thanks, lgtm!

include/clang/Sema/Sema.h

public:
 
   /// Determine if type T is a valid subject for a nonnull and similar
   /// attributes. By default, we look through references (the behavior used by
   /// nonnull), but if the second parameter is true, then we treat a reference
   /// type as valid.
   bool isValidPointerAttrType(QualType T, bool RefOkay = false);
 
   bool CheckRegparmAttr(const AttributeList &attr, unsigned &value);
+  bool getCCFromAttr(const AttributeList &attr, CallingConv &CC);
   bool CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC, 
                             const FunctionDecl *FD = nullptr);
   bool CheckNoReturnAttr(const AttributeList &attr);
   bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
                                       unsigned ArgNum, StringRef &Str,
                                       SourceLocation *ArgLocation = nullptr);
   bool checkSectionName(SourceLocation LiteralLoc, StringRef Str);
   void checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);

lib/Sema/SemaDeclAttr.cpp

D->addAttr(::new (S.Context)
                                 Attr.getAttributeSpellingListIndex()));
     return;
 
   default:
     llvm_unreachable("unexpected attribute kind");
   }
 }
 
-bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC, 
-                                const FunctionDecl *FD) {
+/// This function simply dispatches passed attribute to corresponding
+/// calling convention.
+/// If attribute is ill-formed, it returns 'true' and makes marks attribute as
+/// invalid.
+bool Sema::getCCFromAttr(const AttributeList &attr, CallingConv &CC) {
   if (attr.isInvalid())
     return true;
 
   unsigned ReqArgs = attr.getKind() == AttributeList::AT_Pcs ? 1 : 0;
   if (!checkAttributeNumArgs(*this, attr, ReqArgs)) {
     attr.setInvalid();
     return true;
   }
 
   // TODO: diagnose uses of these conventions on the wrong target.
   switch (attr.getKind()) {
-  case AttributeList::AT_CDecl: CC = CC_C; break;
-  case AttributeList::AT_FastCall: CC = CC_X86FastCall; break;
-  case AttributeList::AT_StdCall: CC = CC_X86StdCall; break;
-  case AttributeList::AT_ThisCall: CC = CC_X86ThisCall; break;
-  case AttributeList::AT_Pascal: CC = CC_X86Pascal; break;
-  case AttributeList::AT_VectorCall: CC = CC_X86VectorCall; break;
+  case AttributeList::AT_CDecl:
+    CC = CC_C;
+    break;
+  case AttributeList::AT_FastCall:
+    CC = CC_X86FastCall;
+    break;
+  case AttributeList::AT_StdCall:
+    CC = CC_X86StdCall;
+    break;
+  case AttributeList::AT_ThisCall:
+    CC = CC_X86ThisCall;
+    break;
+  case AttributeList::AT_Pascal:
+    CC = CC_X86Pascal;
+    break;
+  case AttributeList::AT_VectorCall:
+    CC = CC_X86VectorCall;
+    break;
   case AttributeList::AT_MSABI:
-    CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_C :
-                                                             CC_X86_64Win64;
+    CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_C
+                                                           : CC_X86_64Win64;
     break;
   case AttributeList::AT_SysVABI:
-    CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_X86_64SysV :
-                                                             CC_C;
+    CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_X86_64SysV
+                                                           : CC_C;
     break;
   case AttributeList::AT_Pcs: {
     StringRef StrRef;
     if (!checkStringLiteralArgumentAttr(attr, 0, StrRef)) {
       attr.setInvalid();
       return true;
     }
     if (StrRef == "aapcs") {
       CC = CC_AAPCS;
       break;
     } else if (StrRef == "aapcs-vfp") {
       CC = CC_AAPCS_VFP;
       break;
     }
-
     attr.setInvalid();
     Diag(attr.getLoc(), diag::err_invalid_pcs);
     return true;
   }
-  case AttributeList::AT_IntelOclBicc: CC = CC_IntelOclBicc; break;
-  default: llvm_unreachable("unexpected attribute kind");
+  case AttributeList::AT_IntelOclBicc:
+    CC = CC_IntelOclBicc;
+    break;
+  default:
+    llvm_unreachable("unexpected attribute kind");
+  }
+  return false;
+}
+
+bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC, 
+                                const FunctionDecl *FD) {
+  if (attr.isInvalid())
+    return true;
+
+  if (getCCFromAttr(attr, CC)) {
+    assert(attr.isInvalid() && "Error occured but attribute is not invalid!");
+    return true;
   }
 
   const TargetInfo &TI = Context.getTargetInfo();
   TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC);
   if (A != TargetInfo::CCCR_OK) {
     if (A == TargetInfo::CCCR_Warning)
       Diag(attr.getLoc(), diag::warn_cconv_ignored) << attr.getName();
 

lib/Sema/SemaType.cpp

if (attr.getKind() == AttributeList::AT_Regparm) {
     return true;
   }
 
   // Delay if the type didn't work out to a function.
   if (!unwrapped.isFunctionType()) return false;
 
   // Otherwise, a calling convention.
   CallingConv CC;
   if (S.CheckCallingConvAttr(attr, CC))
     return true;
 
   const FunctionType *fn = unwrapped.get();
   CallingConv CCOld = fn->getCallConv();
   AttributedType::Kind CCAttrKind = getCCTypeAttrKind(attr);
 
   if (CCOld != CC) {
     // Error out on when there's already an attribute on the type
static bool handleFunctionTypeAttr(TypeProcessingState &state,
   }
 
   // Modify the CC from the wrapped function type, wrap it all back, and then
   // wrap the whole thing in an AttributedType as written.  The modified type
   // might have a different CC if we ignored the attribute.
   FunctionType::ExtInfo EI = unwrapped.get()->getExtInfo().withCallingConv(CC);
   QualType Equivalent =
       unwrapped.wrap(S, S.Context.adjustFunctionType(unwrapped.get(), EI));
+
+  // We should add attribute specifier to type only if we didn't subsitute
+  // 'default' calling convention instead of the one provided by the attribute.
+  CallingConv RawCC;
+  bool RecognizedAttr = S.getCCFromAttr(attr, RawCC);
+  assert(!RecognizedAttr &&
+         "Somehow ill-formed calling convention attribute reached here!");
+  TargetInfo::CallingConvCheckResult CCResult =
+      S.Context.getTargetInfo().checkCallingConvention(RawCC);
+  // If calling convention is available (CCCR_OK) or default (CCCR_Ignored), add
+  // it to type.
+  if (CCResult != TargetInfo::CCCR_Warning)
     type = S.Context.getAttributedType(CCAttrKind, type, Equivalent);
   return true;
 }
 
 bool Sema::hasExplicitCallingConv(QualType &T) {
   QualType R = T.IgnoreParens();
   while (const AttributedType *AT = dyn_cast<AttributedType>(R)) {
     if (AT->isCallingConv())
       return true;

test/CodeGen/adding_defaulted_attr_to_type.c

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -verify -emit-llvm -o - %s | FileCheck %s
+
+void (__attribute__ ((regparm (3), stdcall )) *f) (int); // expected-warning {{calling convention 'stdcall' ignored for this target}}
+
+// CHECK: @f = common global void (i32)*

test/Sema/callingconv.c

 
 void __attribute__((intel_ocl_bicc)) inteloclbifunc(float *a) {}
 
 typedef void typedef_fun_t(int);
 typedef_fun_t typedef_fun; // expected-note {{previous declaration is here}}
 void __attribute__((stdcall)) typedef_fun(int x) { } // expected-error {{function declared 'stdcall' here was previously declared without calling convention}}
 
 struct type_test {} __attribute__((stdcall));  // expected-warning {{'stdcall' attribute only applies to functions and methods}}
+
+
+void (__attribute__ ((regparm (3), stdcall("abacaba") )) *test_ignored) (int); // expected-error {{'stdcall' attribute takes no arguments}}