Page MenuHomePhabricator

[attributes] Add a MIG server routine attribute.
ClosedPublic

Authored by NoQ on Feb 18 2019, 8:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 18 2019, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2019, 8:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ marked an inline comment as done.Feb 19 2019, 11:22 AM
NoQ added inline comments.
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, 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 updated this revision to Diff 187497.Feb 19 2019, 7:28 PM
NoQ marked 13 inline comments as done.

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 accepted this revision.Feb 19 2019, 8:19 PM

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

This revision is now accepted and ready to land.Feb 19 2019, 8:19 PM
aaron.ballman added inline comments.Feb 20 2019, 10:22 AM
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
17 ↗(On Diff #187295)

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

6 ↗(On Diff #187497)

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

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 updated this revision to Diff 187656.Feb 20 2019, 12:50 PM
NoQ marked 5 inline comments as done.

Fxd.

NoQ added inline comments.Feb 20 2019, 12:51 PM
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 accepted this revision.Feb 20 2019, 3:17 PM

LGTM with a minor documentation nit.

clang/include/clang/Basic/AttrDocs.td
4049 ↗(On Diff #187656)

can be written -> can also be written

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 4:01 PM