[PATCH] D91898: [attributes] Add a facility for defining and enforcing a Trusted Computing Base.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for this effort!



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11087
+// TCB warnings
+def err_tcb_conflicting_attributes : Error<
+  "attributes '%0(\"%2\")' and '%1(\"%2\")' are mutually exclusive">;
----------------
NoQ wrote:
> aaron.ballman wrote:
> > NoQ wrote:
> > > aaron.ballman wrote:
> > > > NoQ wrote:
> > > > > Do i understand correctly that while "unknown attribute" is a warning ("must be an attribute for some other compiler, let's ignore"), misuse of a known attribute is typically an error ("ok, whatever you meant here, i have an opinion about what this really means and i don't like it")?
> > > > It depends strongly on the attribute and the problem at hand. My usual rule of thumb is: if the attribute is being ignored because the code makes no sense, it's an error, but if the attribute can still be useful (perhaps after some adjustment), then warn. e.g., putting a calling convention attribute on a variable of type `int` makes no sense -- that should probably err rather than warn and ignore because the user did something baffling.
> > > > 
> > > > I could go either way on this one. Giving an error and forcing the user to say what they mean is appealing, but it would also be defensible to warn about the conflicting attribute and ignore just that one (retaining the original attribute). I would say leave it as an error and we can downgrade to a warning later if there's feedback from compelling real world use cases.
> > > > retaining the original attribute
> > > 
> > > There's an interesting aspect of error recovery here (which i didn't address yet but added FIXMEs). We probably shouldn't emit additional warnings based on the attributes about which we've already reported a conflict error; that's just unnecessary clutter. That'd mean that we should always discard the non-leaf attribute and keep the leaf attribute (if we can at all discard attributes). Or keep both and repeat the conflict check during checking in order to suppress the warning.
> > > That'd mean that we should always discard the non-leaf attribute and keep the leaf attribute (if we can at all discard attributes). Or keep both and repeat the conflict check during checking in order to suppress the warning.
> > 
> > You can do `TheDecl->dropAttr<EnforceTCB>();` to drop the non-leaf attributes (or, if easy, it's better to not add the problematic attribute in the first place). Would you like to do that work in this patch or as a follow-up? (I'm fine either way.)
> Hmm, that's indeed an easy and reasonable solution. Ideally i should only drop attributes with the same argument but that's too far into the area of diminishing returns for me :)
Yeah, I don't think we need to go that far unless there's some real world use cases demonstrating the need.


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

https://reviews.llvm.org/D91898