aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup<DiagGroup<"alloca">>, DefaultIgnore; ---------------- george.burgess.iv wrote: > aaron.ballman wrote: > > george.burgess.iv wrote: > > > aaron.ballman wrote: > > > > george.burgess.iv wrote: > > > > > nit: I'd just say "use of function '%0'" here; "builtin" doesn't > > > > > really add much. > > > > > > > > > > I also wonder if we should be saying anything more than "we found a > > > > > use of this function." Looks like GCC doesn't > > > > > (https://godbolt.org/z/sYs_8G), but since this warning is sort of > > > > > opinionated in itself, might it be better to expand this to "use of > > > > > '%0' is discouraged"? > > > > > > > > > > WDYT, Aaron? > > > > What is the purpose to this diagnostic, aside from GCC compatibility? > > > > What does it protect against? > > > > > > > > If there's a reason users should not use alloc(), it would be better > > > > for the diagnostic to spell it out. > > > > > > > > Btw, I'm okay with this being default-off because the GCC warning is as > > > > well. I'm mostly hoping we can do better with our diagnostic wording. > > > > I'm mostly hoping we can do better with our diagnostic wording > > > > > > +1 > > > > > > > What is the purpose to this diagnostic? > > > > > > I think the intent boils down to that `alloca` is easily misused, and > > > leads to e.g., > > > https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . Since its > > > use often boils down to nothing but a tiny micro-optimization, some > > > parties would like to discourage its use. > > > > > > Both glibc and bionic recommend against the use of `alloca` in their > > > documentation, though glibc's docs are less assertive than bionic's, and > > > explicitly call out "[alloca's] use can improve efficiency compared to > > > the use of malloc plus free." > > > > > > Greping a codebase and investigating the first 15 results: > > > - all of them look like microoptimizations; many of them also sit close > > > to other `malloc`/`new` ops, so allocating on these paths presumably > > > isn't prohibitively expensive > > > - all but two of the uses of `alloca` have no logic to fall back to the > > > heap `malloc` if the array they want to allocate passes a certain > > > threshold. Some of the uses make it look *really* easy for the array to > > > grow very large. > > > - one of the uses compares the result of `alloca` to `NULL`. Since > > > `alloca`'s behavior is undefined if it fails, ... > > > > > > I'm having trouble putting this into a concise and actionable diagnostic > > > message, though. The best I can come up with at the moment is something > > > along the lines of "use of function %0 is subtle; consider using heap > > > allocation instead." > > Okay, that's along the lines of what I was thinking. > > > > Part of me thinks that this should not diagnose calls to `alloca()` that > > are given a constant value that we can test for sanity at compile time. > > e.g., calling `alloca(10)` is highly unlikely to be a problem, but calling > > `alloca(1000000)` certainly could be, while `alloca(x)` is a different > > class of problem without good static analysis. > > > > That said, perhaps we could get away with `use of function %0 is > > discouraged; there is no way to check for failure but failure may still > > occur, resulting in a possibly exploitable security vulnerability` or > > something along those lines? > Yeah, GCC has a similar `-Walloca-larger-than=N` that does roughly what you > described. The icky part is exactly what you said. GCC will warn about > unknown values, but considers control flow when doing so: > https://godbolt.org/z/J3pGiT > > It looks like it's the same "we apply optimizations and then see what > happens" behavior as similar diagnostics: https://godbolt.org/z/lLyteP > > WRT the diag we emit here, maybe we could use notes to break it up and imply > things? e.g. > > warning: use of function %0 is discouraged, due to its security implications > note: 'malloc' or 'new' are suggested alternatives, since they have > well-defined behavior on failure > > ...not sold on the idea, but it's a thought. > > If we don't want to break it to pieces, I'm fine with your suggestion I'm not certain the note adds value because it will always be printed on the same line as the warning. A note would make sense if we had a secondary location to annotate though. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
