Page MenuHomePhabricator

NoQ (Artem Dergachev)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 3 2015, 9:16 AM (393 w, 4 d)

Recent Activity

Today

NoQ accepted D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.
Mon, Mar 20, 2:48 PM · Restricted Project

Thu, Mar 16

NoQ added a comment to D143676: [-Wunsafe-buffer-usage] FixableGadget for handling pointer references under UPC..

Other than my usual complaint about declRefExpr(to(varDecl())), LGTM!

Thu, Mar 16, 3:16 PM · Restricted Project
NoQ added inline comments to D143206: [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.
Thu, Mar 16, 2:54 PM · Restricted Project
NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

Other than the to(varDecl()) discussion, LGTM!

Thu, Mar 16, 1:51 PM · Restricted Project

Mon, Mar 13

NoQ added a comment to D145739: [-Wunsafe-buffer-usage][WIP] Group variables associated by pointer assignments.

I guess we can abandon D143133 now in favor of this patch?

Mon, Mar 13, 3:51 PM · Restricted Project
NoQ accepted D145993: [-Wunsafe-buffer-usage] Reducing non-determinism in diagnostics output stream.

LGTM, let's land asap!

Mon, Mar 13, 3:26 PM · Restricted Project, Restricted Project

Wed, Mar 8

NoQ added a comment to D144064: [-Wunsafe-buffer-usage] Match unsafe pointers being casted to bool or participating in pointer subtractions.

Looks great overall!

Wed, Mar 8, 2:27 PM · Restricted Project, Restricted Project
NoQ added a comment to D144905: [-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages.

I think you meant "warnings aren't emitted against UNEVALUATED code"

Wed, Mar 8, 1:08 PM · Restricted Project

Tue, Mar 7

NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

@ymandel I think we should definitely try it.

I don't think our FixableGadgets correspond nicely to RewriteRules. Our story is that we gather *global* understanding of the entire function by collecting gadgets inside it, which is then used for discovering the best "strategy" for transforming the function, which may activate certain (not necessarily all) fixable gadgets in a specific manner. For instance, once D143133 gets fully developed, it'll involve fixpoint iteration over all fixables to discover variables that need to be transformed simultaneously. So I suspect that RewriteRule is too high-level / restrictive for our purposes.

This makes a lot of sense. We've actually moved in the same direction ourself, away from one-shot rules (though we still use them for simpler tasks) towards analysis (or even, an analysis pipeline) followed by application of edits. I think the RewriteRule concept needs to expand to encompass this approach and would hope that feedback from your use case could help drive that. In the meantime, we've found taken two approaches:

  1. Embed the analysis in the matchers and thread that state to the edit generator. So, the rule looks something like makeRule(matcherThatAnchorsAnalysis(SharedState), generatorFromState(SharedState)). Sometimes the generator is a custom function, but we can often use the standard combinators.
  2. Generate _metadata_ from the rewrite rule, and don't generate edits. Then, followup passes can consume the metadata and turn them into edits (using EditGenerators or otherwise). The metadata can also just bundle edits directly, but in a form suited to followup processing rather than the standard output from rules that's intended for direct application.

Regardless, I'd love to work together to find a format that works for these use cases.

Tue, Mar 7, 5:27 PM · Restricted Project
NoQ added a comment to D144905: [-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages.

(these tests can also demonstrate that your fix for _Generic is correct!)

Tue, Mar 7, 3:29 PM · Restricted Project
NoQ added a comment to D144905: [-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages.

Aha, looks correct now!

Tue, Mar 7, 3:28 PM · Restricted Project
NoQ added inline comments to D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment.
Tue, Mar 7, 3:24 PM · Restricted Project, Restricted Project
NoQ added a comment to D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `(DRE.data() + any)`.

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 carefully respect the state of the code as is now?"

The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
"Would you like to change &DRE.data()[any] to (DRE.data() + any)?"

Tue, Mar 7, 3:06 PM · Restricted Project, Restricted Project
NoQ accepted D143680: [-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers.

Ok I think this is good to go!

Tue, Mar 7, 2:50 PM · Restricted Project, Restricted Project

Thu, Mar 2

NoQ added a comment to D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `(DRE.data() + any)`.

Why do we prefer DRE.data() + any to &DRE.data()[any]? It could be much less intrusive this way, and the safety guarantees are the same.

It is actually (DRE.data() + any) versus &DRE.data()[any]. Are they quite the same in terms of being intrusive?

Thu, Mar 2, 5:10 PM · Restricted Project, Restricted Project

Wed, Mar 1

NoQ added a comment to D144977: [analyzer] Fix of the initialization list parsing..

Hi, looks very interesting! We definitely have troubles with some initializer lists.

Wed, Mar 1, 12:33 PM · Restricted Project, Restricted Project

Mon, Feb 27

NoQ added inline comments to D144905: [-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages.
Mon, Feb 27, 6:08 PM · Restricted Project

Thu, Feb 23

NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

Also, @ymandel would you mind if we commit this patch as-is and experiment with Transformers in a follow-up patch? 'Cause we have like 6 patches waiting for this one to land, and we'd rather avoid large-scale rebasing as we fight to keep our backlogs short.

Thu, Feb 23, 5:56 PM · Restricted Project
NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

It also sounds like this is going to be the first use of libClangTransformers in clang proper, so we'll have to link the clang binary to it.

Thu, Feb 23, 5:51 PM · Restricted Project
NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

How do we preserve MatchResult long enough?

Thu, Feb 23, 5:16 PM · Restricted Project
NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

@ymandel I think we should definitely try it.

Thu, Feb 23, 4:54 PM · Restricted Project
NoQ added a comment to D143628: [-Wunsafe-buffer-usage][PoC][WIP] Add #include <span> to emitted Fix-Its.

https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc3/clang/include/clang/Tooling/Transformer/RewriteRule.h#L38 hmmm.

Thu, Feb 23, 2:21 PM · Restricted Project
NoQ added a comment to D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place.

Yeah looks like I replied without properly reading the patch.

Thu, Feb 23, 2:04 PM · Restricted Project, Restricted Project

Wed, Feb 22

NoQ accepted D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.

Looks amazing now! Thank you for getting to the bottom of this and building this elaborate fix for the root cause!

Wed, Feb 22, 5:23 PM · Restricted Project, Restricted Project
NoQ accepted D144352: Do not emit exception diagnostics from coroutines and coroutine lambdas.

Makes sense to me!

Wed, Feb 22, 5:20 PM · Restricted Project, Restricted Project
NoQ added inline comments to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.
Wed, Feb 22, 5:17 PM · Restricted Project
NoQ added inline comments to D143206: [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.
Wed, Feb 22, 5:11 PM · Restricted Project
NoQ accepted D142794: [-Wunsafe-buffer-usage] Fixits for assignment to array subscript expr.

Aha ok LGTM!

Wed, Feb 22, 4:24 PM · Restricted Project, Restricted Project

Tue, Feb 21

NoQ added inline comments to D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.
Tue, Feb 21, 6:37 PM · Restricted Project, Restricted Project
NoQ added a comment to D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place.

I completely agree with @steakhal, these should be note tags:

Tue, Feb 21, 4:16 PM · Restricted Project, Restricted Project
NoQ added inline comments to D143206: [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.
Tue, Feb 21, 1:48 PM · Restricted Project

Feb 16 2023

NoQ added inline comments to D143206: [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.
Feb 16 2023, 6:23 PM · Restricted Project
NoQ added inline comments to D143206: [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.
Feb 16 2023, 6:21 PM · Restricted Project
NoQ added inline comments to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.
Feb 16 2023, 6:11 PM · Restricted Project
NoQ accepted D144003: [clang][analyzer] Improve bug reports of StdLibraryFunctionsChecker..

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 extraordinary statement that the program is wrong, so warning with path notes should look like a proof of that:

"Indeed, let's assume 'x' is greater than 0, Then the program takes true branch on this if-statement. Then it assigns value '0' to pointer 'p'. Which brings us to the statement '*p = 1' three lines below. Pointer 'p' is null, so the program will crash. // Therefore your code is broken, Q.E.D.

It should not end with a general recommendation:

"Indeed, let's assume 'x' is greater than 0. Then the program takes true branch on this if-statement. Then it assigns value '0' to pointer 'p'. Which brings us to the statement '*p = 1' three lines below. Null pointers should not be dereferenced." // I already know that, so what?

This is especially important because people read reports from bottom to top: "Ok, if this happens it's indeed bad, now why do you think this happens?". So if they don't understand what happens (in your case, what value *is* there), they can't even start asking this question. So this is why I think this patch is amazing and it should have been like this from the start.

Feb 16 2023, 5:05 PM · Restricted Project, Restricted Project
NoQ accepted D143697: [-Wunsafe-buffer-usage] Create Fix-Its only if they are emitted.

This is probably good to go either way, we can address the note problem separately. The patch is NFC, purely a performance thing, so no tests are needed.

Feb 16 2023, 3:06 PM · Restricted Project, Restricted Project
NoQ added a comment to D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.

Dude, this is beautiful. I have like one nitpick, and I think we can land.

Feb 16 2023, 12:57 PM · Restricted Project, Restricted Project

Feb 15 2023

NoQ added a comment to D143133: [-Wunsafe-buffer-usage][WIP] Add fixit for variables that have an assignment from another spanified pointer.

We've noticed a subtle problem offline yesterday: sounds like if fixits for both sides are desired (like in the primary test case) but one of them can't be emitted (eg., due to unsupported variable use) then the other fixit alone will be incorrect: it'll act as if both p and q are spanified, so it'll emit no change for p = q. Also generally, we really don't want users to apply fixits for p and q independently; that won't lead to correct code. So it sounds like we cannot land this patch until we learn how to group related variables together.

Feb 15 2023, 6:53 PM · Restricted Project
NoQ added inline comments to D143697: [-Wunsafe-buffer-usage] Create Fix-Its only if they are emitted.
Feb 15 2023, 3:57 PM · Restricted Project, Restricted Project

Feb 14 2023

NoQ added a comment to D143680: [-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers.

Looks great! Nitpicks.

Feb 14 2023, 5:56 PM · Restricted Project, Restricted Project

Feb 9 2023

NoQ added inline comments to D143676: [-Wunsafe-buffer-usage] FixableGadget for handling pointer references under UPC..
Feb 9 2023, 6:27 PM · Restricted Project
NoQ added a comment to D143133: [-Wunsafe-buffer-usage][WIP] Add fixit for variables that have an assignment from another spanified pointer.

Aha, great! Looks like our fancy ownership model couldn't handle shared ownership for multi-variable gadgets, so we're back to raw pointers. Works for me, it's not like they're buffers or something. I have minor nitpicks and then I think we can land!

Feb 9 2023, 4:08 PM · Restricted Project

Feb 8 2023

NoQ added a comment to D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.

CallEvent also needs this information because CallEvent::getProgramPoint() creates implicit call related program points, so we need to modify every call site of CallEvent::CallEvent() to pass this information.

Feb 8 2023, 8:33 PM · Restricted Project, Restricted Project

Feb 7 2023

NoQ accepted D143455: [-Wunsafe-buffer-usage] Emit Fix-Its only for C++20 and later standards.

Yeah that's important! Do we need to cherry-pick this to llvm-16 release candidate?

Feb 7 2023, 7:03 PM · Restricted Project, Restricted Project
NoQ added a comment to D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.

When reviewing please check the history and review the previous solution too (that's only 1 if statement).

Feb 7 2023, 4:59 PM · Restricted Project, Restricted Project
NoQ added a comment to D143328: [analyzer] Remove the loop from the exploded graph caused by missing information in program points.

Ooo, this looks amazing.

Feb 7 2023, 4:52 PM · Restricted Project, Restricted Project
NoQ added a comment to D143133: [-Wunsafe-buffer-usage][WIP] Add fixit for variables that have an assignment from another spanified pointer.

Wait, I completely misread, you *were* trying to mutate the strategy. I think it makes sense to split this work the way I thought it's already split: first get fixits when the strategy is already set (which this patch almost has), then try to figure out how to mutate and fixpoint-iterate-over the strategy.

Feb 7 2023, 3:53 PM · Restricted Project

Feb 6 2023

NoQ added a comment to D142794: [-Wunsafe-buffer-usage] Fixits for assignment to array subscript expr.

Looks great!

Feb 6 2023, 3:53 PM · Restricted Project, Restricted Project
NoQ added a comment to D143133: [-Wunsafe-buffer-usage][WIP] Add fixit for variables that have an assignment from another spanified pointer.

Ooo, that's a big one. Our first fixable that can connect multiple variables together. IIUC this patch doesn't yet teach the strategy machine how to pick related strategies for related variables, it just teaches the fixit machine what to do when the strategy is already picked.

Feb 6 2023, 3:52 PM · Restricted Project
NoQ added a comment to D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `(DRE.data() + any)`.

Why do we prefer DRE.data() + any to &DRE.data()[any]? It could be much less intrusive this way, and the safety guarantees are the same.

Feb 6 2023, 3:46 PM · Restricted Project, Restricted Project

Feb 3 2023

NoQ accepted D141338: [-Wunsafe-buffer-usage] Filter out conflicting fix-its.

Looks great!

Feb 3 2023, 1:45 PM · Restricted Project, Restricted Project
NoQ accepted D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.

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.

Feb 3 2023, 1:43 PM · Restricted Project, Restricted Project

Jan 31 2023

NoQ added inline comments to D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.
Jan 31 2023, 5:08 PM · Restricted Project, Restricted Project

Jan 30 2023

NoQ added a comment to D142795: [-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic.

Nice! I have some nitpicks.

Jan 30 2023, 1:32 PM · Restricted Project
NoQ accepted D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

As for me, this looks like it's good to go! Thanks Rashmi for addressing all the comments!

Jan 30 2023, 12:23 PM · Restricted Project, Restricted Project

Jan 27 2023

NoQ added a comment to D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.

Thank you!! I think we're almost ready to commit but this concern is still hanging:

Jan 27 2023, 5:49 PM · Restricted Project, Restricted Project

Jan 26 2023

NoQ added inline comments to D140489: Add builtin_elementwise_log.
Jan 26 2023, 2:50 PM · Restricted Project, Restricted Project
NoQ accepted D140179: [-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.

Ok I think I'm happy with the patch! Let's give other folks a few days to comment and then land?

Jan 26 2023, 1:00 PM · Restricted Project, Restricted Project
NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

Ok, let's add a virtual method / inheritance test and IMHO we're good to go!

Jan 26 2023, 12:41 PM · Restricted Project, Restricted Project

Jan 25 2023

NoQ added a comment to D142354: [analyzer] Create a stub for an std::variant checker.

In the latter case, have you explored the possibility of force-inlining the class instead, like I suggested in the thread?

I have done some tests then, and figured that the analyzer can't really follow what happens in std::variant. I admit, now that I've taken a more thorough look, I was wrong! Here are some examples.

std::variant<int, std::string> v = 0;
int i = std::get<int>(v);
clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
10 / i; // FIXME: This should warn for division by zero!
std::variant<int, std::string> v = 0;
std::string *sptr = std::get_if<std::string>(&v);
clang_analyzer_eval(sptr == nullptr); // expected-warning{{TRUE}}
*sptr = "Alma!"; // FIXME: Should warn, sptr is a null pointer!
void g(std::variant<int, std::string> v) {
  if (!std::get_if<std::string>(&v))
    return;
  int *iptr = std::get_if<int>(&v);
  clang_analyzer_eval(iptr == nullptr); // expected-warning{{TRUE}}
  *iptr = 5; // FIXME: Should warn, iptr is a null pointer!
}

In neither of these cases was a warning emitted, but that was a result of bug report suppression, because the exploded graph indicates that the errors were found.

We will need to teach these suppression strategies in these cases, the heuristic is too conservative, and I wouldn't mind some NoteTags to tell the user something along the lines of "Assuming this variant holds and std::string value".

Jan 25 2023, 7:32 PM · Restricted Project, Restricted Project

Jan 24 2023

NoQ added a comment to D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.

Awesome, we're getting closer to producing our first fix!

Jan 24 2023, 4:07 PM · Restricted Project, Restricted Project
NoQ added a comment to D142354: [analyzer] Create a stub for an std::variant checker.

Interesting, what specific goals do you have here? Are you planning to find specific bugs (eg. force-unwrap to a wrong type) or just to model the semantics? In the latter case, have you explored the possibility of force-inlining the class instead, like I suggested in the thread? Have you found a reasonable amount of code that uses std::variant, to test your checker on?

Jan 24 2023, 2:32 PM · Restricted Project, Restricted Project

Jan 19 2023

NoQ added inline comments to D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.
Jan 19 2023, 4:34 PM · Restricted Project, Restricted Project
NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

At some point we'd want to add this to release notes just like we did with D135851. But we probably want to do that for the entire -Wunsafe-buffer-usage feature, not for individual elements like this attribute. I think I'll add a release note to D136811 so that it landed at the same time as we start considering the feature is "usable" (for that we need to have an initial implementation of all the enforcement and opt-out elements - warnings, pragmas (D140179), the attribute, but not necessarily fixits).

Jan 19 2023, 3:57 PM · Restricted Project, Restricted Project
NoQ added inline comments to D140179: [-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.
Jan 19 2023, 3:51 PM · Restricted Project, Restricted Project
NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

Ok with the new documentation I think this patch is mostly good to go! @erichkeane WDYT, are we missing anything?

Jan 19 2023, 3:44 PM · Restricted Project, Restricted Project
NoQ added a comment to D137800: [llvm-driver] Reinvoke clang as described by llvm driver extra args.

The ccc-analyzer part looks good to me, thanks for taking care of this!

Jan 19 2023, 3:07 PM · Restricted Project, Restricted Project, Restricted Project

Jan 18 2023

NoQ added inline comments to D140179: [-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.
Jan 18 2023, 4:59 PM · Restricted Project, Restricted Project
NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

Aha, great, you reused a chunk of D136811 to document the attribute. I have a few suggestions how to make it make sense to the reader who didn't read the documentation in D136811, because even when D136811 lands there will still be people who learn about the attribute for the first time from https://clang.llvm.org/docs/AttributeReference.html and not from the document in D136811.

Jan 18 2023, 4:45 PM · Restricted Project, Restricted Project
NoQ added inline comments to D141868: [Clang] [Sema] Removed a fix-it for system headers.
Jan 18 2023, 1:43 PM · Restricted Project, Restricted Project

Jan 17 2023

NoQ added a comment to D141868: [Clang] [Sema] Removed a fix-it for system headers.

Looks great! Sounds like you're looking for a more permanent fix, I guess ConversionFixItGenerator could try to avoid adding fixits to system header functions?

Jan 17 2023, 5:03 PM · Restricted Project, Restricted Project
NoQ accepted D141333: [-Wunsafe-buffer-usage][NFC] Refactor checkUnsafeBufferUsage.

Aha looks great!

Jan 17 2023, 2:54 PM · Restricted Project, Restricted Project

Jan 12 2023

NoQ added a comment to D141338: [-Wunsafe-buffer-usage] Filter out conflicting fix-its.

Looks awesome!

Jan 12 2023, 9:32 PM · Restricted Project, Restricted Project
NoQ accepted D141356: [-Wunsafe-buffer-usage] Group diagnostics by variable.

LGTM! I like the new diagnostics a lot better!

Jan 12 2023, 9:21 PM · Restricted Project, Restricted Project
NoQ added inline comments to D141353: [-Wunsafe-buffer-usage] Emit warnings about unsafe operations on arrays.
Jan 12 2023, 1:17 PM · Restricted Project

Jan 10 2023

NoQ added a comment to D141333: [-Wunsafe-buffer-usage][NFC] Refactor checkUnsafeBufferUsage.

I like this, this is much easier to work with than my original speculative spaghetti!

Jan 10 2023, 6:11 PM · Restricted Project, Restricted Project
NoQ added inline comments to D141353: [-Wunsafe-buffer-usage] Emit warnings about unsafe operations on arrays.
Jan 10 2023, 5:57 PM · Restricted Project
NoQ added a comment to D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations.

Since we do not have any FixableGadget to trigger fix-its at this point, I let fix-its of local variable declarations always be emitted for the purpose of testing.

Jan 10 2023, 5:54 PM · Restricted Project, Restricted Project
NoQ added a comment to D141338: [-Wunsafe-buffer-usage] Filter out conflicting fix-its.

Looks awesome!

Jan 10 2023, 5:43 PM · Restricted Project, Restricted Project
NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

What is NOT acceptable to me is that once an attribute form is written, that it becomes ill-formed in some way. So allowing the attribute in the empty-form on every function (limited to functions, right?), then it cannot EVER become not-allowed/diagnosed for that. If you allow a parameter to it, that form can never be disallowed, etc.

My suggestion was to only add it to functions that you KNOW you can diagnose in the exact form you know you can diagnose, so you don't get stuck in a place where you cannot make something illegal. I believe this is WORSE in cases where you allow parameters without giving immediate meaning to them, but is worth making sure you DO want to someday allow this in the same form on every function. So, are you ok with this being on C variadics, template variadics, prototypeless functions, etc?

Jan 10 2023, 2:25 PM · Restricted Project, Restricted Project

Jan 6 2023

NoQ added a comment to D138940: [-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute.

Can you add more details to that proposal? This is a 'devil in the details' sorta thing for me. Else, feel free to start implementing stage-1 (I suspect you've proposing what I commented on earlier), and I can comment on it there.

Jan 6 2023, 5:17 PM · Restricted Project, Restricted Project

Jan 4 2023

NoQ added a comment to D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check.

The clang binary should not depend on ASTMatchers.

I see this isn't new in this patch, but ASTMatchers should only be used from clang-tidy, not from the compiler itself.

Jan 4 2023, 6:02 PM · Restricted Project, Restricted Project

Jan 3 2023

NoQ accepted D139233: [-Wunsafe-buffer-usage] Add an unsafe gadget for pointer-arithmetic operations.

LGTM!

Jan 3 2023, 1:24 PM · Restricted Project, Restricted Project
NoQ accepted D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check.

Ok LGTM! I hope we'll get a chance to implement a reusable solution later🤞

Jan 3 2023, 1:22 PM · Restricted Project, Restricted Project

Dec 20 2022

NoQ added a comment to D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming..

I suppressed the assertion in rG8fd62e7. It looks like we'll need a deeper investigation into this.

Dec 20 2022, 4:06 PM · Restricted Project, Restricted Project

Dec 19 2022

NoQ added a comment to D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming..

Also if you're compiling your code with -Weverything by default, I do recommend explicitly disabling -Wunsafe-buffer-usage for at least a month or so, until we land all the basic functionality and you can make an informed decision whether you actually need it (more reading in https://discourse.llvm.org/t/rfc-c-buffer-hardening/65734 and D136811).

Dec 19 2022, 2:24 PM · Restricted Project, Restricted Project
NoQ added a comment to D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming..

Ooo thanks I'll fix asap.

Dec 19 2022, 2:11 PM · Restricted Project, Restricted Project

Dec 16 2022

NoQ accepted D138318: [-Wunsafe-buffer-usage] Improve pointer match pattern.

Great!

Dec 16 2022, 4:46 PM · Restricted Project, Restricted Project

Dec 15 2022

NoQ added a comment to D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming..

Because we're approaching a stack of ~15 patches and it's starting to become unmanageable, I'm really interested in landing these older patches quickly. I think I addressed the feedback, and I'm definitely committed to addressing more feedback as it comes in, but it's likely that I'd ask for a chance to address it in follow-up patches anyway. Because otherwise the rest of our team will have to undergo annoying rebases, and it's probably also super confusing to review when all patches in the stack are out of sync from each other. So I'll try to land this tomorrow.

Dec 15 2022, 7:09 PM · Restricted Project, Restricted Project
NoQ added a comment to D137348: [-Wunsafe-buffer-usage] Introduce an abstraction for fixable code patterns..

Because we're approaching a stack of ~15 patches and it's starting to become unmanageable, I'm really interested in landing these older patches quickly. I think I addressed the feedback, and I'm definitely committed to addressing more feedback as it comes in, but it's likely that I'd ask for a chance to address it in follow-up patches anyway. Because otherwise the rest of our team will have to undergo annoying rebases, and it's probably also super confusing to review when all patches in the stack are out of sync from each other. So I'll try to land this tomorrow.

Dec 15 2022, 7:09 PM · Restricted Project, Restricted Project
NoQ accepted D138321: [-Wunsafe-buffer-usage] Ignore array subscript on literal zero.

LGTM!

Dec 15 2022, 7:03 PM · Restricted Project, Restricted Project
NoQ accepted D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations.

LGTM! Let's land once I land the previous patches.

Dec 15 2022, 6:59 PM · Restricted Project, Restricted Project
NoQ added inline comments to D140179: [-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.
Dec 15 2022, 6:57 PM · Restricted Project, Restricted Project
NoQ retitled D140062: [-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets from [WIP] Safe-buffers re-architecture to introduce Fixable gadgets to [WIP][-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets.
Dec 15 2022, 6:15 PM · Restricted Project, Restricted Project
NoQ added a comment to D140062: [-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets.

Looks great to me as well! I have a few nitpicks.

Dec 15 2022, 6:07 PM · Restricted Project, Restricted Project
NoQ added a comment to D140062: [-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets.

Let me add one more thought to our motivation here.
@malavikasamak has pointed out that replacing arr with arr.data() when its type changes from T* to std::span<T> is not a universally correct fix. A counter-example is LHS of an assignment as arr.data() is not an LValue:

arr = nullptr;
=>
arr.data() = nullptr; // error: expression is not assignable

Which means that not only would gadgets that ignore relevant context be of lower quality but in some cases these would also be incorrect.

Dec 15 2022, 5:57 PM · Restricted Project, Restricted Project
NoQ added a comment to D139534: [analyzer] Don't escape local static memregions on bind.

Ok screw it, my static example is still broken. There's technically still a leak in it because there's no time for the other code to kick in between p = (int *)malloc(sizeof(int)); and p = 0; to read the value from p.

Dec 15 2022, 1:53 PM · Restricted Project, Restricted Project
NoQ added inline comments to D139534: [analyzer] Don't escape local static memregions on bind.
Dec 15 2022, 1:32 PM · Restricted Project, Restricted Project
NoQ added inline comments to D139534: [analyzer] Don't escape local static memregions on bind.
Dec 15 2022, 1:25 PM · Restricted Project, Restricted Project
NoQ added inline comments to D139534: [analyzer] Don't escape local static memregions on bind.
Dec 15 2022, 1:19 PM · Restricted Project, Restricted Project