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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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).



================
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'}}
+      }
----------------
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?


================
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?
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93822