Page MenuHomePhabricator

Improve detection of same value in comparisons
ClosedPublic

Authored by rtrieu on Aug 9 2019, 6:54 PM.

Details

Summary

Self comparison under -Wtautological-compare and -Wtautological-overlap-compare work by detecting that two operands refer to the same value. The easy cases of operands referring to the same Decl are already handled. This change extends the detection to look through variable members, as well as detecting static member access via different routes. Also unify the detection method to one location as previously, each warning had its own.

struct S {

int x;
static int y;

};

bool foo(S s) {

return s.x == 1 && s.x == 2;  // detects this is always false
return S::y == s.y;  // detects this is always true

}

Diff Detail

Repository
rL LLVM

Event Timeline

rtrieu created this revision.Aug 9 2019, 6:54 PM
NoQ added a reviewer: NoQ.Aug 9 2019, 8:15 PM
NoQ added a subscriber: NoQ.

I like where this is going! I'll add myself so that not to forget to review the CFG bits next week.

jfb added a subscriber: jfb.Aug 10 2019, 8:57 AM

Does this work for unions as well?

test/Analysis/array-struct-region.cpp
31 ↗(On Diff #214482)

Is this the only way? Or do casts also disable the diagnostic?

test/SemaCXX/self-comparison.cpp
78 ↗(On Diff #214482)

s1.array[0] == s1.array[0]?

87 ↗(On Diff #214482)

The test only has ==. Do other operators trigger?

rtrieu marked 3 inline comments as done.Aug 12 2019, 3:16 PM
In D66045#1624389, @jfb wrote:

Does this work for unions as well?

This will work for union accesses via the same member name, but not different member names.

union U { int x; int y; } u;
bool b1 = u.x == u.x;  // warn always true
bool b2 = u.x == u.y;  // no warning

It's easy to do when the types are the same, but more difficult when the types are different, so keeping with the same name member access is the safe way for now.

test/Analysis/array-struct-region.cpp
31 ↗(On Diff #214482)

There's other ways to do this. I picked "+ 0" here. Explicit casts, like to void* or const S* would also disable it. I think pragmas would also work. I don't particularly care which way since this is an analyzer test.

test/SemaCXX/self-comparison.cpp
78 ↗(On Diff #214482)

No, array accesses aren't checked yet. But it's a good idea to look into it.

87 ↗(On Diff #214482)

All the standard comparison operators will work here. I'll add tests.

NoQ added inline comments.Aug 12 2019, 3:47 PM
lib/AST/Expr.cpp
3931–3932 ↗(On Diff #214482)

Does this actually happen *after* instantiation? If not - do we even need to emit warnings on templates that aren't instantiated? I guess we still need this branch because isSameComparisonOperand() lives in the Expr class and should always work regardless, but in this case i guess this deserves a unittest(?)

3976 ↗(On Diff #214482)

Mmm, what are other cases when a MemberExpr doesn't have a FieldDecl as its getMemberDecl()? Can this be turned into an assert?

test/Analysis/array-struct-region.cpp
31 ↗(On Diff #214482)

I'd rather disable the newly introduced warning on this test file, because this is a path-sensitive static analysis test and this change may accidentally ruin the original test.

rtrieu marked 2 inline comments as done.Aug 12 2019, 4:40 PM
rtrieu added inline comments.
lib/AST/Expr.cpp
3931–3932 ↗(On Diff #214482)

This happens before instantiation. The test cases are in (test/SemaTemplate/self-comparison.cpp). The basic idea is to allow integral template parameters to be treated as variables.

template<int A>
void foo(int x) {
  bool b1 = A == A;
  bool b2 = x == x;
}

If we treat template parameter A the same as variable x, we can diagnose a few more tautological types, even without instantiation.

3976 ↗(On Diff #214482)

I had to look this one up too. From the docs for this method:

/// Retrieve the member declaration to which this expression refers.
///
/// The returned declaration will be a FieldDecl or (in C++) a VarDecl (for
/// static data members), a CXXMethodDecl, or an EnumConstantDecl.
ValueDecl *getMemberDecl() const { return MemberDecl; }

So, FieldDecl, VarDecl, CXXMethodDecl, and EnumConstantDecl are all valid return types.

rtrieu updated this revision to Diff 214752.Aug 12 2019, 7:24 PM

Check array accesses.

rtrieu marked 3 inline comments as done.Aug 12 2019, 7:26 PM
rtrieu added inline comments.
test/Analysis/array-struct-region.cpp
31 ↗(On Diff #214482)

Makes sense. The warning is now disabled via -Wno flags on the RUN lines.

test/SemaCXX/self-comparison.cpp
78 ↗(On Diff #214482)

A little recursive checking and now we check array accesses.

87 ↗(On Diff #214482)

All operator tests added.

jfb added inline comments.Aug 12 2019, 9:38 PM
test/SemaCXX/self-comparison.cpp
87 ↗(On Diff #214482)

operator<=>?

Is there a separate test for user-defined operators as well? What makes sense in these cases? Technically a user-defined comparison could do anything... but I think this warning makes sense for them as well.

rtrieu updated this revision to Diff 215002.Aug 13 2019, 6:19 PM
rtrieu marked an inline comment as done.Aug 13 2019, 6:30 PM
rtrieu added inline comments.
test/SemaCXX/self-comparison.cpp
87 ↗(On Diff #214482)

operator<=> test was added to test/SemaCXX/compare-cxx2a.cpp because the operator requires header that this test doesn't include.

The warning currently doesn't check user-defined operators and I've include a test below to show that. If a warning for user-defined operators was created, it would not belong in -Wtautological-compare because we would not know the actual result. The best we could do is to say there was a self compare with a user-defined operator. It would also need to hook into the AST at a different point. Builtin compare operators are checked in BinaryOperator nodes. User-defined operators would need to be checked at CallExpr or CXXOperatorCallExpr instead. This is different enough that it should be in a different patch.

jfb added inline comments.Aug 13 2019, 9:33 PM
test/SemaCXX/self-comparison.cpp
87 ↗(On Diff #214482)

Thanks, this looks great!

NoQ accepted this revision.Aug 16 2019, 4:37 PM

Thanks from me as well.

lib/AST/Expr.cpp
4009 ↗(On Diff #215002)

const auto *?

This revision is now accepted and ready to land.Aug 16 2019, 4:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 8:04 PM