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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1310
 
+def MIGServerRoutine : InheritableAttr {
+  let Spellings = [Clang<"mig_server_routine">];
----------------
aaron.ballman wrote:
> Should this be limited to specific targets, or is this a general concept that applies to all targets?
I guess it shouldn't. There are [[ https://en.wikipedia.org/wiki/Mach_(kernel)#Software_based_on_Mach | a lot of ]] Darwin-inspecific forks of Mach, and restricting to a specific hardware architecture also doesn't seem to make much sense.


================
Comment at: clang/include/clang/Basic/Attr.td:1312
+  let Spellings = [Clang<"mig_server_routine">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [MIGCallingConventionDocs];
----------------
aaron.ballman wrote:
> Objective-C methods as well?
Mmm, i guess it's technically possible, even if a bit annoying to support. Let me try.


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


================
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>;
----------------
aaron.ballman wrote:
> 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).
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?


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


================
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}}
----------------
aaron.ballman wrote:
> Can you use the C++ spelling for the attribute, so we have a bit of coverage for that?
Is there a vision that i should also provide a namespaced C++ attribute, eg. `[[mig::server_routine]]`?


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

https://reviews.llvm.org/D58365