[PATCH] D58365: [attributes] Add a MIG server routine attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> @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!



================
Comment at: clang/include/clang/Basic/Attr.td:1310
 
+def MIGServerRoutine : InheritableAttr {
+  let Spellings = [Clang<"mig_server_routine">];
----------------
Should this be limited to specific targets, or is this a general concept that applies to all targets?


================
Comment at: clang/include/clang/Basic/Attr.td:1312
+  let Spellings = [Clang<"mig_server_routine">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [MIGCallingConventionDocs];
----------------
Objective-C methods as well?


================
Comment at: clang/include/clang/Basic/Attr.td:1313
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [MIGCallingConventionDocs];
+}
----------------
This isn't really a calling convention though, is it? It doesn't change codegen, impact function types, or anything like that.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8702
+def warn_mig_server_routine_does_not_return_kern_return_t : Warning<
+  "'%0' attribute only applies to functions that return a kernel return code">,
+  InGroup<IgnoredAttributes>;
----------------
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).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7142
+
+  // Mach Interface Generator annotations.
+  case ParsedAttr::AT_MIGServerRoutine:
----------------
This comment doesn't add a ton of value, I'd probably remove it.


================
Comment at: clang/test/Sema/attr-mig.c:6
+
+__attribute__((mig_server_routine)) kern_return_t var = KERN_SUCCESS; // expected-warning-re{{'mig_server_routine' attribute only applies to functions{{$}}}}
+
----------------
Can you also add a test showing that the attribute doesn't accept arguments.


================
Comment at: clang/test/Sema/attr-mig.c:17
+}
+
+kern_return_t bar_forward() { // no-warning
----------------
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?


================
Comment at: clang/test/Sema/attr-mig.cpp:10
+public:
+  virtual __attribute__((mig_server_routine)) IOReturn externalMethod();
+  virtual __attribute__((mig_server_routine)) void anotherMethod(); // expected-warning{{'mig_server_routine' attribute only applies to functions that return a kernel return code}}
----------------
Can you use the C++ spelling for the attribute, so we have a bit of coverage for that?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58365/new/

https://reviews.llvm.org/D58365