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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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:
> > > 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.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16088
+  // is in a TCB that the Callee is not.
+  for (const auto *Attr : Caller->specific_attrs<EnforceTCBAttr>()) {
+    StringRef CallerTCB = Attr->getTCBName();
----------------
Can you rename this to be something other than a type name?


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

https://reviews.llvm.org/D91898