[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/AttrDocs.td:5681
+  trusted compute base (TCB) does not call out of the TCB. This generates a
+  warning everytime a function not marked with an enforce_tcb attribute is
+  called from a function with the enforce_tcb attribute. A function may be a
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5682
+  warning everytime a function not marked with an enforce_tcb attribute is
+  called from a function with the enforce_tcb attribute. A function may be a
+  part of multiple TCBs. Invocations through function pointers are currently
----------------



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


================
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() {}
----------------
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.


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

https://reviews.llvm.org/D91898