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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
