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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ 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">;
----------------
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.


================
Comment at: clang/test/Sema/attr-enforce-tcb-errors.cpp:23
+void both_tcb_and_tcb_leaf_on_separate_redeclarations();
+__attribute__((enforce_tcb_leaf("x"))) // FIXME: This should be an error.
+void both_tcb_and_tcb_leaf_on_separate_redeclarations() {}
----------------
aaron.ballman wrote:
> NoQ wrote:
> > Unfortunately the new facility doesn't catch this case because in `handleEnforceTCBAttr()` the function doesn't yet know that it's a redeclaration. I think this case is more important to catch than the straightforward case (because it's very easy to make this mistake), so i'll try to find a better place to emit the error. Is it ok if this goes into a follow-up patch?
> I was about to comment on the code in SemaDeclAttr.cpp but skipped down here to see if you had a redeclaration test first, and found this comment instead. :-D I think you need to follow the `mergeFooAttr` pattern to enforce this properly (and I think it probably should be done in the initial patch, but it shouldn't be too painful hopefully). The basic idea is that you change `mergeDeclAttribute()` in SemaDecl.cpp to call `S.mergeEnforceTCBAttr()` that checks whether there are conflicting attributes or not. Then in `handleEnforceTCBAttr()` in SemaDeclAttr.cpp, you can call `mergeEnforceTCBAttr()` to handle the straightforward case.
Found it, thanks!!


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

https://reviews.llvm.org/D91898