[PATCH] D158561: [-Wunsafe-buffer-usage] Add AST info to the unclaimed DRE debug notes for analysis

NoQ accepted this revision. NoQ added a comment. Thanks! Looks great now, let's try to land this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158561/new/ https://reviews.llvm.org/D158561 ___ cfe-commits mailing list

[PATCH] D136811: [-Wunsafe-buffer-usage] WIP: RFC: NFC: User documentation.

NoQ added a comment. I'll move this PR to github, and I'll update it to reflect the current state of things, with the aim to have it in good shape (and, possibly, land) before the Dev Meeting. There weren't any major course corrections, but I'll need to spell out what are the things that we've

[PATCH] D158561: [-Wunsafe-buffer-usage] Add AST info to the unclaimed DRE debug notes for analysis

NoQ added inline comments. Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-debug-unclaimed/lit.local.cfg:6 + +config.substitutions.append(('PYTHON_EXE', python_executable)); I might be overthinking/cargo-culting, but folks seem to never expose the

[PATCH] D158561: [-Wunsafe-buffer-usage] Add AST info to the unclaimed DRE debug notes for analysis

NoQ added a comment. I like where this is going, and I appreciate the test! Tests for tiny debug utilities aren't critical for the compiler, but they're a great way to demonstrate and document how to invoke the script and how it's supposed to work. Comment at:

[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks amazing now. Thanks for splitting it up and fighting hard against complexity! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2586 #endif it =

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Everything looks great now, let's commit! Again, thank you Ziqing for simplifying and splitting up the patch. It looks like we're able to keep code complexity from exploding. That makes me feel

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ added inline comments. Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:256 - foo(f(p, , a, a)[1]); // expected-warning{{unsafe buffer access}} - // FIXME: expected note@-1{{in instantiation of function template specialization 'f'

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ updated this revision to Diff 552560. NoQ added a comment. Rebase. Make warnings a bit more verbose than before. This plays nicely with our attempts to categorize unemitted fixits via `-mllvm -debug-only=SafeBuffers`, but this time it doesn't make sense to make the extra information

[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

NoQ added a comment. In D157441#4575220 , @ziqingluo-90 wrote: >>> Extend PointerAssignmentGadget and PointerInitGadget to generate fix-its >>> for cases where the left-hand side >>> has won't fix strategy and the right-hand side has std::span

[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code

NoQ accepted this revision. NoQ added a comment. LGTM! I have minor suggestions for comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1791-1794 // For a `VarDecl` of the form `T * var (= Init)?`, this -// function generates a fix-it for the declaration, which

[PATCH] D158071: [clang] Remove rdar links; NFC

NoQ added a comment. Uh-oh looks like I missed 3 links that were spelled as `radar://`. I confirm that they can be safely removed as I looked into those internal bug reports and found no useful information to add. I'll take a brief look at the links in static analyzer tests later today or

[PATCH] D156474: [-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code

NoQ accepted this revision. NoQ added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156474/new/ https://reviews.llvm.org/D156474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. It doesn't have to be in `core` just because it's dealing with "core features" of the language. The `core` package is for checks without which path-sensitive analysis becomes //so incredibly incorrect// that we don't want to support such configuration, we don't want our

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(), +

[PATCH] D156188: [-Wunsafe-buffer-usage] Refactor and improve for parameter fix-its

NoQ added a comment. Btw, how does this work with `int **`? Given that the pointee type `int *` isn't a simple identifier. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156188/new/ https://reviews.llvm.org/D156188 ___ cfe-commits mailing

[PATCH] D156188: [-Wunsafe-buffer-usage] Refactor and improve for parameter fix-its

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Very nice, everything looks great! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1777-1778 + + StringRef SpanOpen = "std::span<"; + StringRef SpanClose = ">"; +

[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I see you've started to address the big comment in D157441 , so, LGTM, thanks a lot for splitting stuff up! I have one tiny stylistic nitpick.

[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

NoQ added a comment. Ooo nice!! > Extend PointerAssignmentGadget and PointerInitGadget to generate fix-its for > cases where the left-hand side has won't fix strategy and the right-hand side > has std::span strategy. Hmm, is this intertwined with other changes, or is this completely separate?

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. Ok I think this looks great and almost good to go. I love how the core change in the grouping algorithm is actually tiny and simple! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892 + for (unsigned i = 0; i < NumParms; i++) { +if

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. OOo down to ±300, I love this!! I'll take a closer look tomorrow. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892 + for (unsigned i = 0; i < NumParms; i++) { +if (!ParmsMask[i]) + continue; + Speaking of [[

[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2219 + // cannot be fixed... + eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr); + // Now `FixItsForVariable` gets further reduced: a variable is in NoQ wrote: >

[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2219 + // cannot be fixed... + eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr); + // Now `FixItsForVariable` gets further reduced: a variable is in Architecturally

[PATCH] D155814: Fix the linting problems which causes `clang/utils/ci/run-buildbot check-format` to return 1.

NoQ added a comment. Looks like rG910450a28ba9c has independently addressed both issues. But at least we had a chance to deal with the injustice! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

NoQ added a comment. I really like this! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2178 + // `FixItsForVariable` will map each variable to a set of fix-its directly + // associated to the variable itself. Fix-its of distict variables in + // `FixItsForVariable`

[PATCH] D156474: [-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1143 + // order is deterministic: + CompareNode> + byVar; t-rasmud wrote: > NoQ wrote: > > ziqingluo-90 wrote: > > > To make the order of variables in every

[PATCH] D156474: [-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1143 + // order is deterministic: + CompareNode> + byVar; ziqingluo-90 wrote: > To make the order of variables in every group deterministic. Would it make

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

NoQ added a comment. > (1) UBOR is only triggered when the constant folding performed by the Clang > Static Analyzer engine determines that the value of a binary operator > expression is undefined Yes I wholeheartedly agree, these checks are pre-condition checks, they should have never been

[PATCH] D154880: [-Wunsafe-buffer-usage] Add a facility for debugging low fixit coverage.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM! I'm excited to learn what this new facility discovers! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1730-1735 +#ifndef NDEBUG +#define DEBUG_NOTE_DECL_FAIL(D, Msg)

[PATCH] D154880: [-Wunsafe-buffer-usage] Add a facility for debugging low fixit coverage.

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1730-1731 +#define DEBUG_NOTE_DECL_FAIL(D, Msg) \ +Handler.addDebugNoteForVar((D), (D)->getBeginLoc(), "failed to produce fixit for declaration '" + D->getNameAsString() + "'" + Msg) +

[PATCH] D154838: [analyzer] Add check for null pointer passed to %p of printf family

NoQ added a comment. > Should I cover non-standard (i.e., non ISO C standard) functions from the > `printf_s` family? Should I cover non-standard functions like `dprintf`? The static analyzer, unlike the compiler proper, isn't required to treat all code fairly. It's ok to have different

[PATCH] D155524: [-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fixed at an earlier stage

NoQ accepted this revision. NoQ added a comment. LGTM, thanks!! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1095-1102 // Gadgets "claim" variables they're responsible for. Once this loop finishes, // the tracker will only track DREs that weren't claimed by any

[PATCH] D156192: [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM! Yeah I agree that we can eventually support a lot of these incrementally, but it's great to establish a safe baseline first. > If there are other specifiers involved, they may be

[PATCH] D150338: [-Wunsafe-buffer-usage] Improving insertion of the [[clang::unsafe_buffer_usage]] attribute

NoQ added a comment. Looks like the patch has landed, let's close the revision? 樂 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150338/new/ https://reviews.llvm.org/D150338 ___ cfe-commits mailing list

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. I just want to add to this, that the NFC part is actually insanely valuable (despite technically not doing anything). This patch is so complex primarily because `UnsafeBufferUsage.cpp` already has 1300 lines of code in unstructured static functions - that's more than half

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. Alright, I think I managed to fully chew through this patch and I think it's beautiful and everything makes sense to me! I have a tiny complaint though: It is very large though, 500 lines of code is very hard to digest all at once. Because we aren't coming in with all the

[PATCH] D155667: [-Wunsafe-buffer-usage] Check source location validity before using `TypeLoc`s

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155667/new/ https://reviews.llvm.org/D155667 ___

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2248-2249 +#ifndef NDEBUG +// FIXME: F->getBaseStmt() should never be null! +// (Or we should build a better interface for this.) +Handler.addDebugNoteForVar(

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ added a comment. Awesome!! Did you try running it on some real code? Does this actually cover most cases? (I suspect that (1.) is going to be the most popular case, but that's also the easiest case to diagnose visually. We might still want a note if we wanted to prioritize among

[PATCH] D155304: [clang][docs] Update LibASTMatchersReference.html

NoQ added a subscriber: ccotter. NoQ added a comment. @ccotter: now your matcher is properly documented in https://clang.llvm.org/docs/LibASTMatchersReference.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155304/new/

[PATCH] D155641: [-Wunsafe-buffer-usage] Do not assert that function parameters have names.

NoQ added a comment. I nitpicked to comments, but other than that LGTM, thanks! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1890-1891 + // A parameter of a function definition has no name. + // FIXME: We should create a name for the parameter as part of the

[PATCH] D155524: [-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fixed at an earlier stage

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1095-1102 // Gadgets "claim" variables they're responsible for. Once this loop finishes, // the tracker will only track DREs that weren't claimed by any gadgets, // i.e. not understood by

[PATCH] D155524: [-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fixed at an earlier stage

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2249 } UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); Maybe early-return here when there are no gadgets? CHANGES SINCE LAST ACTION

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ updated this revision to Diff 538862. NoQ added a comment. Add the other missing base statement. Debugger was acting weirdly so I thought we had bigger problems, but it's just the other thing missing. It's likely that we ultimately want a better interface for that anyway. CHANGES SINCE

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ updated this revision to Diff 538852. NoQ added a comment. `private:` => `public:` (for some reason it didn't complain until I did a full rebuild) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154880/new/ https://reviews.llvm.org/D154880 Files:

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:578 - virtual const Stmt *getBaseStmt() const override { return nullptr; } + virtual const Stmt *getBaseStmt() const override { return PtrInitRHS; } I changed this to make

[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

NoQ created this revision. NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak. Herald added subscribers: steakhal, martong. Herald added a project: All. NoQ requested review of this revision. Herald added a subscriber: wangpc. This patch adds extra notes to

[PATCH] D150338: [-Wunsafe-buffer-usage] Improving insertion of the [[clang::unsafe_buffer_usage]] attribute

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks good to me! This doesn't take care of the GNU syntax (`__attribute__((unsafe_buffer_usage))`), but this probably isn't a big deal as people can simply use the modern C++ syntax in any code

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293 +// search of connected components. +if (!ParmsNeedFix.empty()) { + auto First = ParmsNeedFix.begin(), Last = First; ziqingluo-90 wrote: > NoQ wrote: > >

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293 +// search of connected components. +if (!ParmsNeedFix.empty()) { + auto First = ParmsNeedFix.begin(), Last = First; ziqingluo-90 wrote: > NoQ wrote: > >

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293 +// search of connected components. +if (!ParmsNeedFix.empty()) { + auto First = ParmsNeedFix.begin(), Last = First; ziqingluo-90 wrote: > NoQ wrote: > > What

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. > In this case function `fileno` returns -1 because of failure, but this is not > indicated in a `NoteTag`. This is a correct result, only the note is missing. > This problem can be solved if a note is displayed on every branch ("case") of > the standard C functions. But

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. Uh-oh, looks like I'm not paying nearly enough attention to this discussion (sorry about that!!) I'm somewhat skeptical of the decision made in D151225 because the entire reason I originally implemented `StdCLibraryFunctions` was to deal

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. Ok whew that's a big patch! I *think* I found a couple of bugs. In any case, this code is quickly becoming very complicated, and the functions we're editing are rapidly growing out of control. Chopping them up into smaller functions with clear separation of concerns would

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2077 + for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { FixItsForVariable[VD] = There's a bug in variable naming: `FixablesForUnsafeVars`actually contains

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

NoQ added a comment. I'll just leave this tiny bit of analysis here, that we did on the whiteboard a while ago. > The "intermediate" overload `void f(std::span, int *)` stays there but > usually is not useful. If there are more than two parameters need to be > fixed, there will be more such

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

NoQ added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:1945 + auto DRE = + DeclRefExpr::Create(*Context, {}, {}, VD, false, SourceLocation(), + VD->getType(), VK_PRValue); tbaeder wrote: > NoQ wrote: > > Is

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

NoQ added a comment. I think this is the right solution and I'm happy to see it! We have a few "direct" CFG tests in `test/Analysis/cfg.c` etc.; your patch doesn't seem to be specific to thread safety analysis so it's probably a good idea to add a few direct tests. Comment

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

NoQ added a comment. > By the way, I'm fed up with the hack that ElementRegion is used for three > separate things ("real" array indexing, casts and pointer arithmetic). To fix > this I'm thinking about introducing a subclass hierarchy where a base class > `ElementLikeRegion` has three

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

NoQ added a comment. I totally support this! There's no reason why the region wouldn't carry such information. This patch is surprisingly tiny, but this matches my grep observations: almost nothing in the static analyzer treats `CXXTempObjectRegion` specially. Even in case of live

[PATCH] D145739: [-Wunsafe-buffer-usage] Group variables associated by pointer assignments

NoQ added a comment. Thank you Sergei for catching this! Thank you Rashmi for fixing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145739/new/ https://reviews.llvm.org/D145739 ___ cfe-commits

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786 + "%select{" +"unsafe operation on raw pointer in expression|" +"unsafe arithmetic on raw pointer|" NoQ wrote: > malavikasamak wrote: > > NoQ wrote: > > > The

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 523601. NoQ added a comment. Rebase *correctly*! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146669/new/ https://reviews.llvm.org/D146669 Files: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 523595. NoQ added a comment. Rebase! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146669/new/ https://reviews.llvm.org/D146669 Files: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h clang/include/clang/Basic/DiagnosticOptions.def

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

NoQ added a comment. Static Analyzer changes look great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150411/new/ https://reviews.llvm.org/D150411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786 + "%select{" +"unsafe operation on raw pointer in expression|" +"unsafe arithmetic on raw pointer|" malavikasamak wrote: > NoQ wrote: > > The first mode

[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok looks great to me now! Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2364 + Node->getBeginLoc())) { + UnsafeBufferUsageReporter R(S); +

[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2340 + if (Node->doesThisDeclarationHaveABody()) +checkUnsafeBufferUsage(Node); + return true; This code will grow bigger when more warnings are added, and it's

[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2320-2323 +void traverseTU(const TranslationUnitDecl *TU) { + for (auto I = TU->decls_begin(); I != TU->decls_end(); ++I) +TraverseDecl(*I); +}

[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1478 + + struct CallableFinderCallback : MatchFinder::MatchCallback { +UnsafeBufferUsageHandler I think this entire matcher business can easily live in

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 515110. NoQ added a comment. Before I forget: Update tests that didn't fail due to my patch because they were testing absence of things (warnings or fixits) and my patch only made them more absent. They need to keep testing absence of these things even when the

[PATCH] D144064: [-Wunsafe-buffer-usage] Match unsafe pointers being casted to bool or participating in pointer subtractions

NoQ accepted this revision. NoQ added a comment. LGTM! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:188-191 + allOf(hasLHS(hasPointerType()), + hasRHS(hasPointerType())), + eachOf(hasLHS(InnerMatcher), +

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 510933. NoQ added a comment. - Rebase! (I'll update related revisions soon but not immediately, need to make sense out of them first.) - Eliminate the `EmitFixits` mode as discussed above. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146669/new/

[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp:19 bool a = (bool) [5]; - // CHECK:

[PATCH] D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `[any]` to `()[any]`

NoQ accepted this revision. NoQ added a comment. Looks great! LGTM except there's some dead code. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:570-573 +if (const auto *ArraySubst = +dyn_cast(Node->getSubExpr())) + if (const auto *DRE = +

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ added a comment. So in a nutshell, this is the intended behavior: | code | diagnostic | EmitFixits && EmitSuggestions | !EmitFixits && EmitSuggestions | !EmitFixits && !EmitSuggestions | |

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786 + "%select{" +"unsafe operation on raw pointer in expression|" +"unsafe arithmetic on raw pointer|" The first mode doesn't show up in any tests and it's

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

NoQ created this revision. NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak, aaron.ballman, gribozavr, ymandel, sgatev. Herald added subscribers: steakhal, martong. Herald added a project: All. NoQ requested review of this revision. So far we didn't pay enough attention to

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 507912. NoQ added a comment. Don't recommend enabling suggestions when suggestions are impossible (eg. due to lack of C++20). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146669/new/ https://reviews.llvm.org/D146669 Files:

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792 "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information">; +def note_safe_buffer_usage_suggestions_disabled : Note< + "pass

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 507903. NoQ added a comment. Explain the weird variable name. Confirm that even though `warn_unsafe_buffer_operation` and `note_unsafe_buffer_operation` have a different number of modes, the mismatch doesn't cause problems yet. Add an assert to catch the

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ updated this revision to Diff 507549. NoQ added a comment. Add some nice assertions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146669/new/ https://reviews.llvm.org/D146669 Files: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

NoQ created this revision. NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak, aaron.ballman, gribozavr, ymandel, sgatev. Herald added subscribers: steakhal, martong. Herald added a project: All. NoQ requested review of this revision. Herald added a subscriber: MaskRay. This

[PATCH] D146342: [WIP][-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ added a comment. Yeah this information is probably at least partially unavailable in the fully parsed AST. We have all the instantiations with their AST copied from the original template AST, but we no longer remember *why* we copied it (which is what the `CodeSynthesisContext` stack

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

NoQ added a comment. I love such debugging facilities! They're a massive boost to developer productivity compared to ad-hoc debug prints, and they allow new contributors to study how the tool works. I'm excited to see how this thing turns out. In the static analyzer's path-sensitive engine

[PATCH] D146342: [WIP][-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

NoQ added a subscriber: usama54321. NoQ added a comment. Thank you for a detailed summary, it's very helpful! > Can we move everything in AnalysisBasedWarnings.cpp to Sema? So far > AnalysisBasedWarnings is used to bridge Sema and UnsafeBufferAnalysis so that > the changes are minimal.

[PATCH] D145993: [-Wunsafe-buffer-usage] Reducing non-determinism in diagnostics output stream

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM, let's land asap! Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:65-71 +// Compares AST nodes by source locations. +template struct CompareNode { +

[PATCH] D144064: [-Wunsafe-buffer-usage] Match unsafe pointers being casted to bool or participating in pointer subtractions

NoQ added a comment. Looks great overall! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:156 + // 2. the operand of a cast-to-(Integer or Boolean) operation; or + // 3. the operand of a pointer subtraction operation; or + // 4. the operand of a pointer comparison

[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment

NoQ added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:149-152 +// Matches a `UnaryOperator` whose operator is pre-increment: +AST_MATCHER(UnaryOperator, isPreInc) { + return Node.getOpcode() == UnaryOperator::Opcode::UO_PreInc; +} Oh

[PATCH] D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `[any]` to `(DRE.data() + any)`

NoQ added a comment. In D143128#4167626 , @jkorous wrote: > This is an interesting topic. In the abstract I see the question as: "Should > the Fix-Its prioritize how the code will fit the desired end state > (presumably modern idiomatic C++) or

[PATCH] D143680: [-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok I think this is good to go! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143680/new/ https://reviews.llvm.org/D143680 ___ cfe-commits

[PATCH] D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `[any]` to `(DRE.data() + any)`

NoQ added a comment. In D143128#4108502 , @ziqingluo-90 wrote: > In D143128#4108375 , @NoQ wrote: > >> Why do we prefer `DRE.data() + any` to `()[any]`? It could be much >> less intrusive this way, and the

[PATCH] D144977: [analyzer] Fix of the initialization list parsing.

NoQ added a comment. Hi, looks very interesting! We definitely have troubles with some initializer lists. It looks like you're primarily interested in the alpha/unsupported checker, but your patch probably affects behavior of the default setup as well. If you've found any difference in

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

NoQ added a comment. Yeah looks like I replied without properly reading the patch. `TaintBugReport` is brilliant and we already have a precedent for subclassing `BugReport` in another checker. However I'm somewhat worried that once we start doing more of this, we'll eventually end up with

[PATCH] D144352: Do not emit exception diagnostics from coroutines and coroutine lambdas

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Makes sense to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144352/new/ https://reviews.llvm.org/D144352

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

NoQ added a comment. I completely agree with @steakhal, these should be note tags: - The "visitor way" is to reverse-engineer the exploded graph after the fact. - The "slightly more sophisticated visitor way" is have checker callbacks leave extra hints in the graph to assist reverse

[PATCH] D144003: [clang][analyzer] Improve bug reports of StdLibraryFunctionsChecker.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks like a massive improvement. Yes, the warning text traditionally focuses on what exactly is wrong. Describing why it's wrong is important as well, but usually less important. We're making an

[PATCH] D143680: [WIP][-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers

NoQ added a comment. Looks great! Nitpicks. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:872 + assert(Loc.isValid() && "Expected the source location of the last" + "character of an AST Node is alwasy valid"); + return Loc;

[PATCH] D143128: [-Wunsafe-buffer-usage][WIP] Fix-Its transforming `[any]` to `DRE.data() + any`

NoQ added a comment. Why do we prefer `DRE.data() + any` to `()[any]`? It could be much less intrusive this way, and the safety guarantees are the same. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143128/new/ https://reviews.llvm.org/D143128

[PATCH] D141338: [-Wunsafe-buffer-usage] Filter out conflicting fix-its

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks great! Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:961 +// a fix-it overlaps with macros; or +// a fix-it conflicts with another one +if

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. As far as I'm concerned, I think this is great for the initial implementation! Let's commit as soon as Jan confirms that his problem is addressed. Comment at:

  1   2   3   4   5   6   7   8   9   10   >