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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 16 inline comments as done.
NoQ added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5497
+  called from a function with the enforce_tcb attribute. A function may be
+  a part of multiple TCBs. Invocations of function pointers and C++ methods
+  are not checked. Builtins are considered to a part of every TCB.
----------------
aaron.ballman wrote:
> Are there plans to support this on function pointers or other kinds of callable things?
> 
> Also, it seems rather odd that C++ methods are not checked. I could somewhat understand not checking virtual functions, but why not member functions where the target is known? Why not static member functions?
Yup, sounds like there are plans for function pointers. We're likely to have a follow-up patch for that. I'll relax the statement to "currently".

With C++ we basically didn't make up our mind yet with respect to what we want. Like, it's kind of strange to isolate individual members of the class into a TCB, it sounds like it'd make a lot more sense on per-class basis but we'll have to see. Also as a matter of fact, the current implementation does check (and emit warnings for) C++ methods. I'll add a test and a TODO to figure out what exactly do we want and drop that part of the documentation.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1246
+
+def TCBEnforcement : DiagGroup<"tcb-enforcement">;
----------------
aaron.ballman wrote:
> Are you planning on adding more diagnostics under this group? If not, I'd suggest defining it inline (see below).
Aha, ok, not yet but i guess we can always bring the group back if we need more diagnostics.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16079-16084
+  for (const auto *Attr : Callee->specific_attrs<EnforceTCBAttr>()) {
+    CalleeTCBs.insert(Attr->getTCBName());
+  }
+  for (const auto *Attr : Callee->specific_attrs<EnforceTCBLeafAttr>()) {
+    CalleeTCBs.insert(Attr->getTCBName());
+  }
----------------
aaron.ballman wrote:
> Pretty sure you can remove the manual loops here with something like this.
`std::inserter` doesn't seem to work with `llvm::StringSet` but `llvm::for_each` works and seems to be more compact(?)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16086
+
+  // Go through the TCBs the caller is a part of and emit warnings if Caller is in a TCB that the Callee is not
+  for (const auto *Attr : Caller->specific_attrs<EnforceTCBAttr>()) {
----------------
aaron.ballman wrote:
> Can you be sure to run the patch through clang-format, this looks like it's over the 80 col limit.
Umm yeah indeed thanks!


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16090-16092
+    if (CalleeTCBs.count(CallerTCB) == 0) {
+      Diag(TheCall->getExprLoc(), diag::warn_tcb_enforcement_violation) << CallerTCB << Callee->getName();
+    }
----------------
aaron.ballman wrote:
> 
TIL that `<< Callee` adds quotes automatically. I should use clang diagnostic builders more often :)


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

https://reviews.llvm.org/D91898