Page MenuHomePhabricator

Add -Wpointer-compare
ClosedPublic

Authored by ziyig on Aug 1 2019, 11:12 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

ziyig created this revision.Aug 1 2019, 11:12 AM

+1, useful check.

what about:
char *a...
(char) 0 == a

?

Is this off by default in GCC? I would like to see it -Wall/-Wextra..

aaron.ballman added inline comments.Aug 1 2019, 12:02 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3291 ↗(On Diff #212859)

I think this would read a bit better as comparing a pointer to a null character constant; did you mean to compare to %select{nullptr|NULL|0}?

I'm a bit unsure about NULL vs 0 in the diagnostic, but I kind of feel like we should prefer nullptr if it's enabled, NULL if it's available, and 0 only as a last-ditch fallback.

3292 ↗(On Diff #212859)

Why is this disabled by default?

clang/include/clang/Sema/Sema.h
9994 ↗(On Diff #212859)

Should be CheckNullCharToNullptr() to fit with the surrounding naming conventions.

clang/lib/Sema/SemaExpr.cpp
10436 ↗(On Diff #212859)

I'd drop this comment as it doesn't add much value.

10441 ↗(On Diff #212859)

Rather than do isa<> followed by a dyn_cast<>, this should be:

if (const auto *RCL = dyn_cast<CharacterLiteral>(E.get())) {
}
10443 ↗(On Diff #212859)

I think it would be more clear to compare against 0 as an integer literal here.

clang/test/Sema/warn-nullchar-nullptr.c
9 ↗(On Diff #212859)

I'd appreciate tests for the other character literals as well. L'\0', u8'\0', etc.

ziyig marked 8 inline comments as done.Aug 1 2019, 2:03 PM

+1, useful check.

what about:
char *a...
(char) 0 == a

?

Thanks for the suggestion, I added the test.

Is this off by default in GCC? I would like to see it -Wall/-Wextra..

This is not off by default in GCC, changed.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3292 ↗(On Diff #212859)

I changed this to not off by default.

ziyig updated this revision to Diff 212901.Aug 1 2019, 2:03 PM
ziyig marked an inline comment as done.
aaron.ballman added inline comments.Aug 2 2019, 6:14 AM
clang/include/clang/Sema/Sema.h
9994 ↗(On Diff #212901)

I'd drop the comment here, it doesn't add much value (and it's over the 80 col limit).

clang/lib/Sema/SemaExpr.cpp
10447 ↗(On Diff #212901)

This logic is incorrect (and it won't pick from three different options provided by the diagnostic).

10448–10449 ↗(On Diff #212901)

Formatting is off here -- you should run the patch through clang-format, if you don't already do that.

10451 ↗(On Diff #212901)

This should probably be looking at the canonical type so that we strip off all the sugar. You should add a test like:

typedef char my_char;

int func(int *a) {
  return a == (my_char)0; 
}
10452 ↗(On Diff #212901)

I see that GCC only warns on char types, but it seems equally plausible (through macros or typedefs) to get a wchar_t or other kind of character type. Should we warn on those as well?

clang/test/Sema/warn-nullchar-nullptr.c
4 ↗(On Diff #212901)

We definitely should not be suggesting nullptr in C code. :-)

Maybe emit fixit with NULL/nullptr too?

ziyig updated this revision to Diff 213088.Aug 2 2019, 10:50 AM
ziyig marked 6 inline comments as done.
ziyig added inline comments.
clang/lib/Sema/SemaExpr.cpp
10452 ↗(On Diff #212901)

I don't have any opinion about this. Which one do you think is better?

xbolva00 added inline comments.Aug 2 2019, 10:58 AM
clang/lib/Sema/SemaExpr.cpp
10440 ↗(On Diff #213088)

StringRef?

10461 ↗(On Diff #213088)

You want to replace it with NullValue, not NullNotes.

xbolva00 added inline comments.Aug 2 2019, 11:00 AM
clang/test/Sema/warn-nullchar-nullptr.c
5 ↗(On Diff #213088)

Please add test for:

a != ‘\0’

ziyig updated this revision to Diff 213103.Aug 2 2019, 11:44 AM
ziyig marked 2 inline comments as done.
ziyig retitled this revision from Add -Wpointer-compare as an optional warning to Add -Wpointer-compare.
ziyig marked 2 inline comments as done.
ziyig added inline comments.
clang/lib/Sema/SemaExpr.cpp
10452 ↗(On Diff #212901)

It works for typedefs and MACRO now. Is this what you expected?

xbolva00 added inline comments.Aug 2 2019, 11:51 AM
clang/lib/Sema/SemaExpr.cpp
10460 ↗(On Diff #213103)

You can ditch SelectValue and use %0 in diag msg. Then ‘<< NullValue’.

Can you add tests for C++? E.g C++03 and C++11?

Other than that, I have no comments - looks fine.

ziyig updated this revision to Diff 213107.Aug 2 2019, 12:02 PM
ziyig marked an inline comment as done.
ziyig added a comment.EditedAug 2 2019, 12:05 PM

Can you add tests for C++? E.g C++03 and C++11?

C++ doesn't support comparison between pointer and character, so I guess it's not necessary?
Here is an example of comparing pointer and character in C++:
int foo(int *a) {

return a == '\0';

}

#1 with x86-64 clang (trunk)
<source>:2:14: error: comparison between pointer and integer ('int *' and 'char')

return a == '\0';
       ~ ^  ~~~~

1 error generated.
Compiler returned: 1

So

if (C mode only) {
CheckNullCharToNullptr(LHS, RHS);
CheckNullCharToNullptr(RHS, LHS);
}

?

Drop if (cpp11) condition in the function?

Rename it please. ‘CheckPtrComparisonWithNullChar’ ?

ziyig updated this revision to Diff 213113.Aug 2 2019, 12:50 PM
xbolva00 added inline comments.Aug 4 2019, 3:45 AM
clang/lib/Sema/SemaExpr.cpp
10497 ↗(On Diff #213113)

!getLangOpts().CPlusPlus ?

aaron.ballman added inline comments.Aug 5 2019, 5:50 AM
clang/lib/Sema/SemaExpr.cpp
10452 ↗(On Diff #212901)

Not quite -- I wasn't very clear. I was thinking more about a scenario like this:

#define CHECK_VAL(x, y) (x != y)
#ifdef NARROW
#define TYPE char
#else
#define TYPE wchar_t
#endif

void func(int *ip) {
  if (CHECK_VAL(ip, (TYPE)0))
    ;
}

When NARROW is defined, your check will trigger, but when it's not defined, there is no diagnostic.

I can't think of a situation where we'd want to warn on a cast involving char but not wchar_t (for instance), so I think you should check for isAnyCharacterType() rather than just CharTy below.

10439–10443 ↗(On Diff #213113)

I would rather see this done using a %select in the diagnostic, since both of the strings are statically known at compile time.

10453 ↗(On Diff #213113)

Formatting is still off here (the else binds to the } above).

10459 ↗(On Diff #213113)

When switching to using %select above, this can become NullValue ? "NULL" : "(void *)0"

10497 ↗(On Diff #213113)

Agreed -- that's the canonical way to check for any C mode.

@aaron.ballman, Should we also consider address spaces? Or exotic NULL as non-zero bit pattern?

@aaron.ballman, Should we also consider address spaces? Or exotic NULL as non-zero bit pattern?

I don't think address spaces would have any impact here, and unless LLVM supports a platform with a non-zero NULL bit pattern, I don't think it would be all that beneficial to consider it (we could always do that in a follow-up patch though).

Right, thanks for info.

ziyig updated this revision to Diff 213384.Aug 5 2019, 9:40 AM
ziyig marked 7 inline comments as done.
ziyig added inline comments.
clang/lib/Sema/SemaExpr.cpp
10452 ↗(On Diff #212901)

I changed the check to isAnyCharacterType(), but I don't think there's any way to test it in C cause wchar_t will be treated as an interger in C, so I didn't add tests for this.

a > (char) 0 deserves other diagnostic message.. gcc emits: ordered comparison of pointer with integer zero.

This warning should handle ==, != only, I think.

Pleae add test for a > ‘\0’ to be sure we dont suggest a > NULL.
Then a follow up patch could diagnose ordered comparison with integer zero.

aaron.ballman added inline comments.Aug 5 2019, 10:35 AM
clang/lib/Sema/SemaExpr.cpp
10452 ↗(On Diff #212901)

Ugh, so that explains why this part of the check only makes sense for char types. I forgot that wchar_t has an underlying integer type in C. I guess you can go back to just checking CharTy because the same holds true for char16_t and char32_t in C. Sorry about the churn!

10439–10443 ↗(On Diff #213113)
int NullValue = PP.isMacroDefined("NULL") ? 0 : 1;
ziyig updated this revision to Diff 213404.Aug 5 2019, 10:53 AM
ziyig marked 3 inline comments as done.
xbolva00 accepted this revision.Aug 5 2019, 11:05 AM
This revision is now accepted and ready to land.Aug 5 2019, 11:05 AM
aaron.ballman accepted this revision.Aug 5 2019, 11:09 AM

LGTM, thank you for the patch!

Closed by commit rL367940: [Sema] Add -Wpointer-compare (authored by gbiv, committed by ). · Explain WhyAug 5 2019, 3:15 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 3:15 PM