Page MenuHomePhabricator

[Diagnostics] Implement -Wsizeof-pointer-div
ClosedPublic

Authored by xbolva00 on Oct 5 2018, 2:41 PM.

Details

Summary

void test(int *arr) {

int arr_len = sizeof(arr) / sizeof(*arr);  // warn, incorrect way to compute number of array elements

}

Enabled under -Wall (same behaviour as GCC)

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Oct 5 2018, 2:41 PM
xbolva00 updated this revision to Diff 168539.Oct 5 2018, 2:47 PM
xbolva00 added inline comments.Oct 5 2018, 2:50 PM
test/Sema/div-sizeof-ptr.c
9 ↗(On Diff #168539)

GCC warns also in this case, but it is weird...

xbolva00 updated this revision to Diff 168546.Oct 5 2018, 3:23 PM
  • small code improvements

It looks like you ran clang-format on all of lib/Sema/SemaExpr.cpp and changed many lines that are irrelevant to your patch. Can you undo that, please?

xbolva00 updated this revision to Diff 168562.Oct 6 2018, 12:56 AM
  • fixed formatting noise
xbolva00 updated this revision to Diff 168564.Oct 6 2018, 12:59 AM
xbolva00 updated this revision to Diff 168569.Oct 6 2018, 4:13 AM
  • added DefaultIgnore
jfb added a comment.Oct 9 2018, 9:05 PM

Can you add tests with arrays?
The error message doesn't really say what to do instead. What's wrong? What's correct instead?
In C++17 and later we should suggest using std::size for some cases instead.

In D52949#1259947, @jfb wrote:

Can you add tests with arrays?

Yes :)

The error message doesn't really say what to do instead. What's wrong? What's correct instead?

What do you suggest? "division produces the incorrect number of array elements; pass the length of array as a function argument"?

In C++17 and later we should suggest using std::size for some cases instead.

Ok, good idea.

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

@rsmith?

MTC added a subscriber: MTC.Oct 23 2018, 9:19 AM

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

IMHO, a clang-tidy checker is more suitable than clang static analyzer to indicate to the developers that we should prefer std::size to sizeof(data)/sizeof(*data).

What I want to know most is whether we should emit a warning for the code below. I haven't been able to test the patch yet, but intuitively, this patch will emit a warning.

int foo(int *p) {
    int d = sizeof(p) / sizeof(char); // Should we emit a warning here?
    return d;
}

GCC keep silent about this case, and I agree with it, see https://gcc.godbolt.org/z/ZToFqq. What do you think about this case?

No warning for your case.

xbolva00 updated this revision to Diff 170696.EditedOct 23 2018, 10:45 AM
  • Added MTC's example case to test file

Any ideas for better warning message? Except for the warning text, I see this patch as ready.

xbolva00 updated this revision to Diff 171920.Oct 31 2018, 7:55 AM
xbolva00 added a reviewer: MTC.
  • New warning text, added another test line
MTC added a comment.Nov 1 2018, 4:03 AM

Sorry for the long delay for this patch! The implementation is fine for me. However, I'm the newbie on clang diagnostics and have no further insight into this checker. @aaron.ballman may have more valuable insights into this checker.

test/Sema/div-sizeof-ptr.c
13 ↗(On Diff #171920)

My bad! I didn't read the code carefully.

aaron.ballman added inline comments.Nov 1 2018, 5:56 AM
include/clang/Basic/DiagnosticGroups.td
147 ↗(On Diff #171920)

Do you really need the explicit diagnostic group? Given that there's only one diagnostic here, I would lower this into the diag (see below).

include/clang/Basic/DiagnosticSemaKinds.td
3299 ↗(On Diff #171920)

InGroup<DiagGroup<"sizeof-pointer-div">>

lib/Sema/SemaExpr.cpp
8729 ↗(On Diff #171920)

const Expr *

8731–8733 ↗(On Diff #171920)

Can use const auto * here as the type is spelled out in the initialization.

Under what circumstances would you expect to get a null LHS or RHS? I suspect this should be using dyn_cast instead.

8746–8750 ↗(On Diff #171920)

Elide braces.

test/Sema/div-sizeof-ptr.c
1 ↗(On Diff #171920)

This is missing tests for the positive behavior -- can you add some tests involving arrays rather than pointers? Here are some other fun test cases as well:

int a[10][12];
sizeof(a) / sizeof(*a); // Should this warn?
template <typename Ty, int N>
int f(Ty (&Array)[N]) {
  return sizeof(Array) / sizeof(Ty); // Shouldn't warn
}
9 ↗(On Diff #168539)

I think it's reasonable to not diagnose here, but I don't have strong feelings.

xbolva00 updated this revision to Diff 172117.Nov 1 2018, 6:43 AM
xbolva00 marked 5 inline comments as done.
  • Addressed review comments.

We match the GCC behaviour here, except the case:
int b1 = sizeof(int *) / sizeof(int);

But I think it is ok now.

lib/Sema/SemaExpr.cpp
8729 ↗(On Diff #171920)

if const, i have build issues with "<< LHS"

aaron.ballman accepted this revision.Nov 1 2018, 7:44 AM

LGTM with a minor formatting nit.

lib/Sema/SemaExpr.cpp
8729 ↗(On Diff #171920)

That sounds like a bug with the diagnostic builder, but it can be addressed separately.

test/Sema/div-sizeof-ptr.cpp
1 ↗(On Diff #172117)

Please run this file through clang-format as the formatting appears to be off.

27 ↗(On Diff #172117)

GCC does not warn here, but I question whether we should. This evaluates to 10, but is that what the user expects it to evaluate to, as opposed to 120?

Regardless, I think it's reasonable to not warn on this right now (it would need different diagnostic text).

This revision is now accepted and ready to land.Nov 1 2018, 7:44 AM
This revision was automatically updated to reflect the committed changes.