Page MenuHomePhabricator

Produce warning for performing pointer arithmetic on a null pointer.
ClosedPublic

Authored by jamieschmeiser on Mar 17 2021, 11:06 AM.

Details

Summary

Test and produce warning for subtracting a pointer from null or subtracting null from a pointer. Reuse existing warning that this is undefined behaviour. Also add unit test for both warnings.

Diff Detail

Event Timeline

jamieschmeiser requested review of this revision.Mar 17 2021, 11:06 AM
jamieschmeiser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2021, 11:06 AM

I used formatting similar to the existing code, which is not what clang-format is expecting.

jamieschmeiser retitled this revision from Produce waring for performing pointer arithmetic on a null pointer. to Produce warning for performing pointer arithmetic on a null pointer..Mar 29 2021, 6:29 AM

Reformat to satisfy clang-format

Please add a test for (char*)0-(char*)0.

Respond to review comments: add requested test.

efriedma added inline comments.Apr 9 2021, 10:58 AM
clang/lib/Sema/SemaExpr.cpp
10783

IgnoreParenCasts() seems a little weird here; e.g. this code thinks (char*)(char)256 isn't null? Not sure how much it practically matters, though, given this is just a warning.

clang/test/Sema/pointer-addition.c
34

This is what I was afraid would happen.

C++ has a specific carveout in the standard that "null-null" isn't undefined. C doesn't, but I'm not sure warning is practically useful there.

In any case, printing the same warning twice isn't useful.

jamieschmeiser added inline comments.Apr 9 2021, 11:35 AM
clang/test/Sema/pointer-addition.c
34

Yes, I see that [expr.add] p 7 says nullptr-nullptr is defined to be 0. I will suppress the warning for this.

jamieschmeiser added inline comments.Apr 9 2021, 2:08 PM
clang/test/Sema/pointer-addition.c
34

I disagree with the comment that the two warnings are not useful. While it is the same warning, the section of code indicated by each warning is different and both need to be corrected to clear the code of warnings. A single warning would likely be more confusing in that a user would see the same warning and not notice that a different section of code is indicated after fixing the indicated problem. I would expect the typical response to be the user assuming that they had made a mistake and removing the initial fix and fixing the second, only to again receive a same warning again with a different section of code. The different code sections indicated would likely be overlooked, leading to frustration whereas two warnings clearly indicates there are 2 problems.

Respond to review comments: Do not issue warning for nullptr - nullptr in C++.

Fix indenting.

xbolva00 added a subscriber: xbolva00.

Maybe Nick could try this patch with linux kernel?

Maybe Nick could try this patch with linux kernel?

Yes! Thank you of thinking of me for this; we're currently sorting issues identified by this warning (as implemented today, for addition only IIUC) in:
https://lore.kernel.org/lkml/20210430111641.1911207-1-schnelle@linux.ibm.com/

I ran out of time today, but I'll give this a shot tomorrow and see if this catches anything that looks like a false positive.

I didn't see any instances in quick testing on the Linux kernel: x86_64 defconfig, aarch64 defconfig, arm defconfig, x86_64 allmodconfig. So I guess that's a good thing (for Linux)!

clang/lib/Sema/SemaExpr.cpp
10782–10793

Might it be better to move the C++ check to the top; have all of this within a if (!getLangOpts().CPlusPlus) { block? Perhaps I'm misunderstanding what the || is doing?

efriedma added inline comments.Mon, May 3, 5:45 PM
clang/lib/Sema/SemaExpr.cpp
10782–10793

The getLangOpts().CPlusPlus check is specifically so we don't warn on null-null, which the C++ standard allows.

clang/test/Sema/pointer-addition.c
34

I see what you mean about the two warnings; printing twice is probably okay.

In terms of whether we actually want the null-null warning in C, I could go either way. I usually like to avoid differences between C and C++ where possible. But the standards are actually written differently here. And it's unlikely that explicitly written null-null shows up in practice, so probably nobody will notice either way.

Needs a testcase for the C++ behavior.

Respond to review comments: add C++ test.

efriedma accepted this revision.Thu, May 6, 1:06 PM

LGTM

This revision is now accepted and ready to land.Thu, May 6, 1:06 PM