Page MenuHomePhabricator

[Diagnostics] Add -Wsizeof-array-div
ClosedPublic

Authored by xbolva00 on Fri, Sep 6, 9:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Fri, Sep 6, 9:41 AM
xbolva00 updated this revision to Diff 219175.Fri, Sep 6, 2:18 PM

Addressed comments by @rsmith

xbolva00 marked 3 inline comments as done.Fri, Sep 6, 2:24 PM
xbolva00 added inline comments.
lib/Sema/SemaExpr.cpp
9169 ↗(On Diff #219175)

If we dont skip isDependentType, compiler crashes later
Unknown builtin type!
UNREACHABLE executed at /home/xbolva00/LLVM/llvm/tools/clang/lib/AST/ASTContext.cpp:1855!
Stack dump:
0. Program arguments: /home/xbolva00/LLVM/build/bin/clang -cc1 -internal-isystem /home/xbolva00/LLVM/build/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -verify -std=c++11 /home/xbolva00/LLVM/llvm/tools/clang/test/SemaTemplate/instantiate-init.cpp

Even if skip hasSimilarType call, the warning message is unfriendly like "element type is "int", not "<dependent type>"

Ok to skip?

9174 ↗(On Diff #219175)

fails to build with const Expr *, so workaround.

test/Sema/div-sizeof-array.cpp
16 ↗(On Diff #219175)

We could warn here, but the code is working fine iff sizeof(unsigned long long) == sizeof(int *).

Maybe just ignore this case for now?

rsmith added inline comments.Tue, Sep 10, 11:43 AM
lib/Sema/SemaExpr.cpp
9169 ↗(On Diff #219175)

Yes, we can't compute the size of a dependent type. You should also check this for ArrayElemTy.

9170 ↗(On Diff #219175)

You don't need this check if you're also checking sizes: similar types always have the same size.

9171 ↗(On Diff #219175)

And we know that these types are complete or dependent because we already checked the formation of a sizeof expression on them? OK.

9174 ↗(On Diff #219175)

Do not include pretty-printed expressions in diagnostics like this. If the LHS is a DeclRefExpr, you can print its name here, but otherwise you should just underline the expression rather than printing it out.

xbolva00 updated this revision to Diff 219591.Tue, Sep 10, 1:08 PM

Addressed review notes

xbolva00 marked 4 inline comments as done.Tue, Sep 10, 1:08 PM
xbolva00 marked an inline comment as done.
xbolva00 added inline comments.
lib/Sema/SemaExpr.cpp
9174 ↗(On Diff #219175)

Ok, done.

Implemented a bit different than suggested, maybe ok too?

Just one thing I missed before, otherwise this looks good. Thanks!

lib/Sema/SemaExpr.cpp
9171 ↗(On Diff #219591)

Use Context.getAsArrayType here to properly deal with type sugar.

xbolva00 updated this revision to Diff 219597.Tue, Sep 10, 1:40 PM
  • use S.Context.getAsArrayType

Thanks for review/advices!

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Sep 11, 3:58 AM
Closed by commit rL371605: [Diagnostics] Add -Wsizeof-array-div (authored by xbolva00, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 11, 3:58 AM

Did you collect metrics on number of true/false positives of this warning?

Thanks for review/advices!

It seems you committed in a hurry. This adds a new enabled-by-default warning which can break people's builds if they have -Werror. I don't know who will be broken but I think you probably needs a thumbs-up before committing.

cfe/trunk/test/Sema/div-sizeof-array.cpp
1

For enabled-by-default warnings, I guess it may be nice to have another RUN line without -Wsizeof-array-div.

Can sou show me a false positive case?

xbolva00 added a comment.EditedWed, Sep 11, 5:25 AM

This adds a new enabled-by-default warning which can break people's builds if they have -Werror.

Sorry, but why a LLVM/Clang contributor should care if user’s -Werror builds may break just because a true positive?

If it breaks a build and finds a bug, then great, no?

gcc devs going to implement it too (inspired by this patch ;-))

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91741

FWIW, we did find a false positive with this warning.

https://github.com/bluerise/openbsd/blob/664a06af7e423ff06d46383c669d33c7bea7d234/lib/libutil/bcrypt_pbkdf.c#L81

uint32_t cdata[BCRYPT_WORDS];
...
blf_enc(&state, cdata, sizeof(cdata) / sizeof(uint64_t));

Looks like a bug indeed, until we look at the documentation for blf_enc, https://man.openbsd.org/blowfish, which says that the function indeed accepts 64-bit-sized blocks. However, for unclear reasons, it accepts the buffer as u_int32_t*. So the callers must have a buffer of u_int32_t, but compute its size in uint64_ts.

So, is it a false positive? Yes. However, the code is indeed questionable.

Thanks, ugh, that’s weird. Yes, very questionable code. I would probably change it to

unsigned blockSize = sizeof(uint64_t);
sizeof(cdata) / blockSize;

and add a code comment that interface requires such blocks. Warning will go away and code readability will be better.

@xbolva00

As mentioned in an inline comment, the test only works if the sizeof(int) != sizeof(int *) and sizeof(unsigned long long) == sizeof(int)

The test then also runs clang -cc1 with the default target. This means the test asserts that all clang targets match the assertions above.

My team supports an embedded ARM default LLVM product, and this test is now failing on our end because int is 4 bytes, unsigned long long is 8 bytes, and int * is 4 bytes.
This test needs to either work for all targets (via some macro hackery) or the target needs to be made explicit, which I believe would be preferable because the warning itself is not reliant upon the target, only the type sizes.

For instance, adding -triple x86_64-unknown-linux-gnu fixes the test on our embedded ARM toolchain.

I added that triple yesterday, failing bot is green with it. Did you pull latest sources?

Ah! My apologies. I had not pulled anything new down, as our process tends to hard-stop until an issue is resolved (something I'm thinking needs to be a little more flexible).

I'll pull the fix down then, thanks!

Not sure if we're still mentioning false positives here, but if we have something like a multidimensional array of a specific type and we want to find the number of elements inside it, this warning would also appear:

int arr[][5] = {
  {1, 2, 3, 4, 5},
  {6, 7, 8, 9, 0},
};

void func() {
  unsigned num_elems = sizeof(arr) / sizeof(int);  // Warns here
  assert(num_elems == 10);
}

Although one could argue that sizeof(arr) / sizeof(**arr) would be better while not producing the warning.

xbolva00 added a comment.EditedFri, Sep 13, 4:25 PM

Hm, yes, I will disable this warning if the element type is an array I think.

Or just wait and see how gcc devs will implement this and whether they will restrict it or not.