This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Reduce null checking of C++ object pointers (PR27581)
ClosedPublic

Authored by vsk on Feb 3 2017, 6:57 PM.

Details

Summary

Given a load of a member variable from an instance method ('this->x'),
ubsan inserts a null check for 'this', and another null check for
'&this->x', before allowing the load to occur. Both of these checks are
redundant, because 'this' must have been null-checked before the method
is called.

Similarly, given a call to a method from another method bound to the
same instance ('this->foo()'), ubsan inserts a redundant null check for
'this'. There is also a redundant null check in the case where the
object pointer is a reference ('Ref.foo()').

This patch teaches ubsan to remove the redundant null checks identified
above. I'm not sure I've gone about this in the way suggested in PR27581,
and would appreciate any advice/corrections.

Testing: check-clang and check-ubsan. I also compiled X86FastISel.cpp
with -fsanitize=null using patched/unpatched clangs based on r293572.
Here are the number of null checks emitted in various setups:

Setup# of null checks
unpatched, -O021767
patched, -O010758

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Feb 3 2017, 6:57 PM

I feel like you should have at least one statement in the test that uses explicit 'this' to access a field/method.

Does your patch handle fields/methods in base classes as well? I think 'this' might be implicitly converted in the base of the member expression.

Also, what about cases like this one:

struct A {
  int x;

  void methodNotConst() const {
    const_cast<A*>(this)->x = 0;
  }
};
vsk updated this revision to Diff 87334.Feb 6 2017, 4:31 PM
vsk edited the summary of this revision. (Show Details)

@arphaman thank you for the feedback!

Per Alex's comments:

  • Add test for explicit use of the 'this' pointer.
  • Handle cases where the 'this' pointer may be casted (implicitly, or otherwise).

We're able to eliminate more null checks with this version of the patch. See the updated patch summary for the X86FastISel.cpp numbers.

arphaman added a comment.EditedFeb 9 2017, 4:30 AM

Thanks!

I guess for the sake of completeness it might be useful for handle 'this' in parens as well, since it could come up in macros:

#define MEMBER(x) (x)->y
...
MEMBER(this)
...

Btw, I was curious if we could do a similar optimization in Objective-C, but 'self' can be set to null there inside the body of a method. I guess maybe it would be possible to avoid the null check on self if we can prove it wasn't modified, but that's out of scope of this patch.

lib/CodeGen/CGExprCXX.cpp
294 ↗(On Diff #87334)

You can avoid the '{' '}' here.

Btw, you mentioned that 'this' must have been null-checked before the method is called. But what if it's called from some part of code that was compiled without -fsanitize=null? Wouldn't we still want at least one check to see if 'this' is null in a method?

vsk added a comment.Feb 10 2017, 10:18 AM

Ah, I did miss ParenExpr. Maybe it would be better to use Expr::isImplicitCXXThis, since it handles this case and some more. Later, we can go back and see if it's feasible to handle static/const casts in isImplicitCXXThis to catch more cases.

Btw, you mentioned that 'this' must have been null-checked before the method is called. But what if it's called from some part of code that was compiled without -fsanitize=null? Wouldn't we still want at least one check to see if 'this' is null in a method?

That's fair, I think that would address the concerns about the partial sanitization use case brought up in the original PR.

vsk updated this revision to Diff 88075.Feb 10 2017, 7:08 PM
vsk edited the summary of this revision. (Show Details)
vsk added a reviewer: arphaman.
  • Check 'this' once per method. This supports the partial sanitization use-case.
  • Flesh out the predicate for avoiding null checks on object pointers. I couldn't use isImplicitCXXThis like I'd wanted, because we'd like to catch explicit this exprs as well.
arphaman added inline comments.Feb 13 2017, 10:54 AM
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
8 ↗(On Diff #88075)

I think this kind of check would've passed even when this patch is not applied. I understand that you need the first check for the single 'this' check, but it should be rewritten to ensure that it couldn't be the check for the return foo. Maybe you could use a C label before return and then check that the 'this' check is performed before the label?

vsk marked 2 inline comments as done.Feb 13 2017, 2:23 PM
vsk added inline comments.
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
8 ↗(On Diff #88075)

Yes, this check should be tighter. This just checks that the number of checks in the method goes from 2 to 1. The label idea is great.

vsk updated this revision to Diff 88259.Feb 13 2017, 2:24 PM
vsk marked an inline comment as done.
  • Tighten up the tests per Alex's suggestion.
This revision is now accepted and ready to land.Feb 16 2017, 9:41 AM
This revision was automatically updated to reflect the committed changes.