[PATCH] D93822: [clang][Sema] Add diagnostics for implicit widening of multiplication result

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: rtrieu.
dblaikie added a comment.

In D93822#2474355 <https://reviews.llvm.org/D93822#2474355>, @lebedev.ri wrote:

> Thank you for taking a look!
>
> In D93822#2474236 <https://reviews.llvm.org/D93822#2474236>, @dblaikie wrote:
>
>> Haven't reviewed the code in detail, but the idea seems sound.
>
> That's reassuring!
>
>> Is there any prior art in other compilers you can draw data/experiences from?
>
> As far as i can tell, no other compiler diagnoses these cases,
> however e.g. GitHub's CodeQL static analysis tool complains about this by default:
> https://github.com/darktable-org/rawspeed/security/code-scanning/7?query=342efe49c0bef4bf2450051586d899f9d303ae62
>
>> & having some data from some reasonably sized codebases on false positive
>> (how often the correct thing is to suppress the warning, versus fixing the warning)
>> would probably be useful.
>
> I don't have those numbers presently (other than the long long diffs of fixes for true-positives i have already linked),
> but as a rule of thumb, iff a particular project in question
> never allocates more than `UINT_MAX` bytes / `UINT_MAX / sizeof(element)` elements,
> *all* issued diagnostics for said project *will* be false-positives by definition.
> So YMMV.
>
>> I would guess it doesn't meet the on-by-default bar we would prefer for Clang,
>
> Yeah, based on my initial observations, it's pretty chatty.
>
>> but might still be OK.
>
> I'm open to input here, but it would be good to have this at least in `-Wextra` (`-Weverything` always being a last resort option).

Yeah, not sure how @rsmith, @rtrieu, or @aaron.ballman feel about what the bar for warnings is these days - used to be a bit more hardcore about the "if it can't be on by default it shouldn't be in clang" than we are these days, I think. I'd guess -Wextra might be suitable.



================
Comment at: clang/docs/ReleaseNotes.rst:64-74
+  - ``-Wimplicit-widening-of-pointer-offset``:
+
+    .. code-block:: c++
+
+      char* ptr_add(char *base, int a, int b) {
+        return base + a * b; // expected-warning {{result of multiplication in type 'int' is used as a pointer offset after an implicit widening conversion to type 'ssize_t'}}
+      }
----------------
lebedev.ri wrote:
> dblaikie wrote:
> > Does this only trigger when the `sizeof(char*) > sizeof(int)`? (judging by the test coverage I think that's the case)
> > 
> > (ultimately, might be worth committing the two diagnostics separately - usual sort of reasons, separation of concerns, etc)
> > Does this only trigger when the sizeof(char*) > sizeof(int)? (judging by the test coverage I think that's the case)
> 
> That should be the case, since `sizeof(size_t) == sizeof(char*)` and
> `// RUN: %clang_cc1 -triple i686-linux-gnu   -fsyntax-only -Wimplicit-widening-of-pointer-offset -verify=silent %s`
> 
> > (ultimately, might be worth committing the two diagnostics separately - usual sort of reasons, separation of concerns, etc)
> 
> Should i split this into two reviews to begin with?
> Should i split this into two reviews to begin with?
Nah, doubt it's worth splitting up right now - see how a few folks feel about the idea in general, and then if there's enough mechanical details to discuss might be worth splitting out one on top of the other.


================
Comment at: clang/test/Sema/implicit-widening-of-pointer-offset-in-array-subscript-expression.c:24
+void t1(char *base, int a, int b) {
+  // FIXME: test `[a * b]base` pattern?
+}
----------------
lebedev.ri wrote:
> dblaikie wrote:
> > Question is unclear - is there uncertainty about whether this should be tested? Or is this a false negative case? In which case I'd probably include the test showing no diagnostic and mention it could be fixed/improved?
> I may be misremembering things, but IIRC `a[b]` and `b[a]` is the same thing,
> but i'm not sure how to exercise the second spelling.
> I.e. i'm just not sure how to write a test for it.
> I may be misremembering things, but IIRC a[b] and b[a] is the same thing,
Yep, that's the case - `a[b]` where one of them is a pointer and teh other is an integer, is equivalent to `*(a + b)`, so that means it's the same as `b[a]`.

I guess you'd write it the same as t0, but with the expressions reversed? `return *(a * b)[base];`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822