This is an archive of the discontinued LLVM Phabricator instance.

Support ObjC Method call in enforce_tcb checks
ClosedPublic

Authored by pdherbemont on Mar 23 2022, 12:39 PM.

Details

Summary
Before this change, calling an ObjC method from a C function marked with
the enforce_tcb attribute would not produce a warning.

After this change, we'll get a proper TCB warning when calling an ObjC method from a TCB. Also, this change enables ObjC methods to participate in a TCB.

  1. Allow ObjC Methods to be decorated with the enforce_tcb attribute
  2. Change the CheckTCBEnforcement() function to accept a NamedDecl (instead of the more specialized FunctionDecl) and a SourceLocation (instead of a CallExpr).
  3. Call CheckTCBEnforcement() from CheckObjCMethodCall()
  4. Add a test.

Diff Detail

Event Timeline

pdherbemont created this revision.Mar 23 2022, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 12:39 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
pdherbemont requested review of this revision.Mar 23 2022, 12:39 PM
NoQ edited reviewers, added: NoQ, t-rasmud; removed: dergachev.a, vsavchenko.Mar 23 2022, 1:37 PM
NoQ added a comment.Mar 23 2022, 1:40 PM

Looks great! It might make sense to add tests to test/Sema/attr-enforce-tcb-errors.cpp to ensure that attribute conflicts are resolved correctly on Objective-C methods but also it's not like we've changed anything there. I have only one tiny nitpick.

clang/lib/Sema/SemaChecking.cpp
17371

This should be a plain cast<> because we already know that the object is of the right type so there's nothing dynamic about it.

Removed the useless dyn_cast.

pdherbemont marked an inline comment as done.Mar 23 2022, 2:02 PM

Added clang/test/Sema/attr-enforce-tcb-errors.m based on clang/test/Sema/attr-enforce-tcb-errors.cpp.

pdherbemont added a comment.EditedMar 23 2022, 3:55 PM

Looks great! It might make sense to add tests to test/Sema/attr-enforce-tcb-errors.cpp to ensure that attribute conflicts are resolved correctly on Objective-C methods but also it's not like we've changed anything there. I have only one tiny nitpick.

Thanks for the review! I added a dedicated test/Sema/attr-enforce-tcb-errors.m. I thought it might be better to keep ObjC aside. Let me know if you want a different approach.

NoQ accepted this revision.Mar 23 2022, 4:22 PM

Looks great, thanks! I'll wait a bit to see if other folks have something to say about this and then commit the patch for you.

This revision is now accepted and ready to land.Mar 23 2022, 4:22 PM

Just some suggestions, feel free to ignore them.

clang/include/clang/Sema/Sema.h
12968–12969

Maybe Call[Expr]Loc or InvocationLoc so that we're clear what the expression is?

clang/lib/Sema/SemaChecking.cpp
5500

You could check for builtins here, then the cast down there wouldn't be needed.

17370–17372

I'd suggest to hoist that check into CheckFunctionCall, then we don't need to cast at all. But feel free to keep it as is.

aaron.ballman accepted this revision.Mar 24 2022, 5:10 AM

LGTM, though I agree with the suggestions from @aaronpuchert.

Integrated Aaron review feedbacks:

  • Use more expressive name CallExprLoc.
  • Moved the builtin check one level up to avoid casting.

Thank you Aaron!

pdherbemont marked 2 inline comments as done.Mar 24 2022, 7:56 AM

Fixed clang-format.

aaronpuchert accepted this revision.Mar 24 2022, 1:02 PM

Feel free to fix that on commit.

clang/include/clang/Sema/Sema.h
12968–12969

The source file calls this CallExprLoc, I think you forgot to rename it here.

Updated CheckTCBEnforcement definition.

pdherbemont marked an inline comment as done.Mar 24 2022, 4:46 PM
pdherbemont added inline comments.
clang/include/clang/Sema/Sema.h
12968–12969

The source file calls this CallExprLoc, I think you forgot to rename it here.

indeed!

pdherbemont marked an inline comment as done.Mar 24 2022, 4:47 PM
NoQ added a comment.Mar 28 2022, 1:30 PM

Great! I'll commit.

Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 3:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript