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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
