[attributes] Add a MIG server routine attribute.

[NoQ]

Summary

Continuing the trend started in D54912, i'd like to stuff one more XNU/Mach/Darwin-specific attribute into there. This one's fairly simple and is designed in order to be respected by the Static Analyzer's Mach Interface Generator calling convention checker that's getting added in D57558.

@aaron.ballman, are you actually interested in reviewing these attributes? 'Cause George added you on his reviews but i totally understand that these are fairly exotic.

[NoQ]

clang/include/clang/Basic/AttrDocs.td

4047–4048 ↗

(On Diff #187295)

Is there a way to enforce this automagically via adding some feature to the attribute? Or should i enforce it manually by teaching Static Analyzer to scan the overridden methods when looking for the attribute, like i did in D58397?

[aaron.ballman]

@aaron.ballman, are you actually interested in reviewing these attributes? 'Cause George added you on his reviews but i totally understand that these are fairly exotic.

Yeah, I'd like to review these attributes -- thanks for double-checking with me!

clang/include/clang/Basic/Attr.td

1310 ↗

(On Diff #187295)

Should this be limited to specific targets, or is this a general concept that applies to all targets?

1312 ↗

(On Diff #187295)

Objective-C methods as well?

1313 ↗

(On Diff #187295)

This isn't really a calling convention though, is it? It doesn't change codegen, impact function types, or anything like that.

clang/include/clang/Basic/DiagnosticSemaKinds.td

8702 ↗

(On Diff #187295)

Will users understand "kernel return code"? Should this say kern_return_t explicitly?

No need to use %0 here, just spell out the attribute name directly (unless you expect this to be used by multiple attributes, in which case the name of the diagnostic should be changed).

clang/lib/Sema/SemaDeclAttr.cpp

7142 ↗

(On Diff #187295)

This comment doesn't add a ton of value, I'd probably remove it.

clang/test/Sema/attr-mig.c

6 ↗

(On Diff #187295)

Can you also add a test showing that the attribute doesn't accept arguments.

17 ↗

(On Diff #187295)

Here's an edge case to consider:

__attribute__((mig_server_routine)) int foo(void);

typedef int kern_return_t;

kern_return_t foo(void) { ... }

Do you have to care about situations like that?

clang/test/Sema/attr-mig.cpp

10 ↗

(On Diff #187295)

Can you use the C++ spelling for the attribute, so we have a bit of coverage for that?

[NoQ]

Fxd, thx!

clang/include/clang/Basic/Attr.td

1310 ↗

(On Diff #187295)

I guess it shouldn't. There are a lot of Darwin-inspecific forks of Mach, and restricting to a specific hardware architecture also doesn't seem to make much sense.

1312 ↗

(On Diff #187295)

Mmm, i guess it's technically possible, even if a bit annoying to support. Let me try.

1313 ↗

(On Diff #187295)

That's right, but for whatever reason it's often referred to as "a" calling convention. It makes slight sense because callee/caller-deallocated OOL parameters are kinda similar to callee/caller-saved registers, something like that. I guess i'll call it just "convention".

clang/include/clang/Basic/DiagnosticSemaKinds.td

8702 ↗

(On Diff #187295)

It should say either kern_return_t or IOReturn depending on the specific framework that's being used (the latter is a typedef for the former). I guess i could scan the AST to for a typedef kern_return_t IOReturn and display the appropriate message, but this sort of stuff always sounds like an overkill. For now i change the wording to an exact "a kern_return_t". I could also say "a kern_return_t or an IOReturn", do you have any preference here?

clang/test/Sema/attr-mig.c

17 ↗

(On Diff #187295)

Do you have to care about situations like that?

I hope i not :) kern_return_t is available pretty much everywhere and most likely it's not a problem to update the first declaration of foo() with kern_return_t. Ok if i add a relaxing code later if it turns out that i have to?

clang/test/Sema/attr-mig.cpp

10 ↗

(On Diff #187295)

Is there a vision that i should also provide a namespaced C++ attribute, eg. [[mig::server_routine]]?

[dcoughlin]

This looks good to me as long as Aaron is happy with it.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8702 ↗

(On Diff #187295)

Yeah, I think that scanning the AST would be overkill. This seems sufficiently clear, and we can always improve it if users wind up being confused in practice.

clang/lib/Sema/SemaDeclAttr.cpp

6407 ↗

(On Diff #187497)

I'd prefer this to use getFunctionOrMethodResultType().

clang/test/Sema/attr-mig.c

6 ↗

(On Diff #187497)

What's the purpose of the {{$}}?

17 ↗

(On Diff #187295)

That's what I was hoping to hear. :-)

clang/test/Sema/attr-mig.cpp

10 ↗

(On Diff #187295)

I think this should continue to use [[clang::mig_server_routine]].

clang/test/Sema/attr-mig.m

21 ↗

(On Diff #187497)

I'd change this to use FIXME instead of TODO.

[NoQ]

Fxd.

[NoQ]

clang/test/Sema/attr-mig.c

6 ↗

(On Diff #187497)

Hmm, right. In the previous, emm, revision of this revision i felt the urge to force the end-ofline check in order to discriminate between the two warnings that i introduced, out of which one is an initial segment of the other. But now, given that we have ", and", i guess it's no longer necessary.

[aaron.ballman]

LGTM with a minor documentation nit.

clang/include/clang/Basic/AttrDocs.td

4049 ↗

(On Diff #187656)

can be written -> can also be written

cfe/trunk/include/clang/Basic/Attr.td

 }
 
 def MayAlias : InheritableAttr {
   // FIXME: this is a type attribute in GCC, but a declaration attribute here.
   let Spellings = [GCC<"may_alias">];
   let Documentation = [Undocumented];
 }
 
+def MIGServerRoutine : InheritableAttr {
+  let Spellings = [Clang<"mig_server_routine">];
+  let Subjects = SubjectList<[Function, ObjCMethod, Block]>;
+  let Documentation = [MIGConventionDocs];
+}
+
 def MSABI : DeclOrTypeAttr {
   let Spellings = [GCC<"ms_abi">];
 //  let Subjects = [Function, ObjCMethod];
   let Documentation = [MSABIDocs];
 }
 
 def MSP430Interrupt : InheritableAttr, TargetSpecificAttr<TargetMSP430> {
   // NOTE: If you add any additional spellings, ARMInterrupt's, MipsInterrupt's

cfe/trunk/include/clang/Basic/AttrDocs.td

void f(NSArray *first_param, __strong NSArray *second_param) {
     // ...
   }
 
 Likewise, when applied to a strong local variable, that variable becomes
 ``const`` and is considered externally-retained.
 
 When compiled without ``-fobjc-arc``, this attribute is ignored.
 }]; }
+
+def MIGConventionDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+  The Mach Interface Generator release-on-success convention dictates
+functions that follow it to only release arguments passed to them when they
+return "success" (a ``kern_return_t`` error code that indicates that
+no errors have occured). Otherwise the release is performed by the MIG client
+that called the function. The annotation ``__attribute__((mig_server_routine))``
+is applied in order to specify which functions are expected to follow the
+convention. This allows the Static Analyzer to find bugs caused by violations of
+that convention. The attribute would normally appear on the forward declaration
+of the actual server routine in the MIG server header, but it may also be
+added to arbitrary functions that need to follow the same convention - for
+example, a user can add them to auxiliary functions called by the server routine
+that have their return value of type ``kern_return_t`` unconditionally returned
+from the routine. The attribute can be applied to C++ methods, and in this case
+it will be automatically applied to overrides if the method is virtual. The
+attribute can also be written using C++11 syntax: ``[[mig::server_routine]]``.
+}];
+}

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_opencl_builtin_expected_type : Error<
   "illegal call to %0, expected %1 argument type">;
 
 // OpenCL v2.2 s2.1.2.3 - Vector Component Access
 def ext_opencl_ext_vector_type_rgba_selector: ExtWarn<
   "vector component name '%0' is an OpenCL version 2.2 feature">,
   InGroup<OpenCLUnsupportedRGBA>;
+
+// MIG routine annotations.
+def warn_mig_server_routine_does_not_return_kern_return_t : Warning<
+  "'mig_server_routine' attribute only applies to routines that return a kern_return_t">,
+  InGroup<IgnoredAttributes>;
 } // end of sema category
 
 let CategoryName = "OpenMP Issue" in {
 // OpenMP support.
 def err_omp_expected_var_arg : Error<
   "%0 is not a global variable, static local variable or static data member">;
 def err_omp_expected_var_arg_suggest : Error<
   "%0 is not a global variable, static local variable or static data member; "

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (BOSType > 3) {
     return;
   }
 
   D->addAttr(::new (S.getASTContext()) FortifyStdLibAttr(
       AL.getLoc(), S.getASTContext(), BOSType, Flag,
       AL.getAttributeSpellingListIndex()));
 }
 
+static void handleMIGServerRoutineAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  // Check that the return type is a `typedef int kern_return_t` or a typedef
+  // around it, because otherwise MIG convention checks make no sense.
+  // BlockDecl doesn't store a return type, so it's annoying to check,
+  // so let's skip it for now.
+  if (!isa<BlockDecl>(D)) {
+    QualType T = getFunctionOrMethodResultType(D);
+    bool IsKernReturnT = false;
+    while (const auto *TT = T->getAs<TypedefType>()) {
+      IsKernReturnT = (TT->getDecl()->getName() == "kern_return_t");
+      T = TT->desugar();
+    }
+    if (!IsKernReturnT || T.getCanonicalType() != S.getASTContext().IntTy) {
+      S.Diag(D->getBeginLoc(),
+             diag::warn_mig_server_routine_does_not_return_kern_return_t);
+      return;
+    }
+  }
+
+  handleSimpleAttribute<MIGServerRoutineAttr>(S, D, AL);
+}
+
 //===----------------------------------------------------------------------===//
 // Top Level Sema Entry Points
 //===----------------------------------------------------------------------===//
 
 /// ProcessDeclAttribute - Apply the specific attribute to the specified decl if
 /// the attribute applies to decls.  If the attribute is a type attribute, just
 /// silently ignore it if a GNU attribute.
 static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
 
   case ParsedAttr::AT_ObjCExternallyRetained:
     handleObjCExternallyRetainedAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_FortifyStdLib:
     handleFortifyStdLib(S, D, AL);
     break;
+
+  case ParsedAttr::AT_MIGServerRoutine:
+    handleMIGServerRoutineAttr(S, D, AL);
+    break;
   }
 }
 
 /// ProcessDeclAttributeList - Apply all the decl attributes in the specified
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
                                     const ParsedAttributesView &AttrList,
                                     bool IncludeCXX11Attributes) {

cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)
 // CHECK-NEXT: IFunc (SubjectMatchRule_function)
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
+// CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
 // CHECK-NEXT: MicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: MinSize (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: MinVectorWidth (SubjectMatchRule_function)
 // CHECK-NEXT: Mips16 (SubjectMatchRule_function)
 // CHECK-NEXT: MipsLongCall (SubjectMatchRule_function)
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)

cfe/trunk/test/Sema/attr-mig.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef int kern_return_t;
+#define KERN_SUCCESS 0
+
+__attribute__((mig_server_routine)) kern_return_t var = KERN_SUCCESS; // expected-warning{{'mig_server_routine' attribute only applies to functions, Objective-C methods, and blocks}}
+
+__attribute__((mig_server_routine)) void foo_void(); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+__attribute__((mig_server_routine)) int foo_int(); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+
+__attribute__((mig_server_routine)) kern_return_t bar_extern(); // no-warning
+__attribute__((mig_server_routine)) kern_return_t bar_forward(); // no-warning
+
+__attribute__((mig_server_routine)) kern_return_t bar_definition() { // no-warning
+  return KERN_SUCCESS;
+}
+
+kern_return_t bar_forward() { // no-warning
+  return KERN_SUCCESS;
+}
+
+__attribute__((mig_server_routine(123))) kern_return_t bar_with_argument(); // expected-error{{'mig_server_routine' attribute takes no arguments}}

cfe/trunk/test/Sema/attr-mig.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef int kern_return_t;
+typedef kern_return_t IOReturn;
+#define KERN_SUCCESS 0
+#define kIOReturnSuccess KERN_SUCCESS
+
+class MyServer {
+public:
+  virtual __attribute__((mig_server_routine)) IOReturn externalMethod();
+  virtual __attribute__((mig_server_routine)) void anotherMethod(); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+  virtual __attribute__((mig_server_routine)) int yetAnotherMethod(); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+  [[clang::mig_server_routine]] virtual IOReturn cppAnnotatedMethod();
+  [[clang::mig_server_routine("arg")]] virtual IOReturn cppAnnotatedMethodWithInvalidArgs(); // expected-error{{'mig_server_routine' attribute takes no arguments}}
+  [[clang::mig_server_routine]] virtual int cppInvalidAnnotatedMethod(); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+};
+
+IOReturn MyServer::externalMethod() {
+  return kIOReturnSuccess;
+}

cfe/trunk/test/Sema/attr-mig.m

+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+
+typedef int kern_return_t;
+#define KERN_SUCCESS 0
+
+@interface NSObject
+@end
+
+@interface I: NSObject
+- (kern_return_t)foo __attribute__((mig_server_routine)); // no-warning
+- (void) bar_void __attribute__((mig_server_routine)); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+- (int) bar_int __attribute__((mig_server_routine)); // expected-warning{{'mig_server_routine' attribute only applies to routines that return a kern_return_t}}
+@end
+
+@implementation I
+- (kern_return_t)foo {
+  kern_return_t (^block)() = ^ __attribute__((mig_server_routine)) { // no-warning
+    return KERN_SUCCESS;
+  };
+
+  // FIXME: Warn that this block doesn't return a kern_return_t.
+  void (^invalid_block)() = ^ __attribute__((mig_server_routine)) {};
+
+  return block();
+}
+- (void)bar_void {
+}
+- (int)bar_int {
+  return 0;
+}
+@end