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

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

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

LGTM, let's land asap!

Looks great overall!

In D144905#4179108, @jkorous wrote:

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

In D142795#4165331, @ymandel wrote:

In D142795#4148991, @NoQ wrote:

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

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

Aha, looks correct now!

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 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)?"

Ok I think this is good to go!

In D143128#4108502, @ziqingluo-90 wrote:

In D143128#4108375, @NoQ wrote:

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?

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

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.

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.

How do we preserve MatchResult long enough?

@ymandel I think we should definitely try it.

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

Yeah looks like I replied without properly reading the patch.

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

Makes sense to me!

Aha ok LGTM!

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

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.

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.

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

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.

Looks great! Nitpicks.

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!

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.

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

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

Ooo, this looks amazing.

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.

Looks great!

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.

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.

Looks great!

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.

Nice! I have some nitpicks.

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

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

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

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

In D142354#4079643, @Szelethus wrote:

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

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

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?

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

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

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

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.

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?

Aha looks great!

Looks awesome!

LGTM! I like the new diagnostics a lot better!

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

In D139737#4037455, @ziqingluo-90 wrote:

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.

Looks awesome!

In D138940#4036211, @erichkeane wrote:

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?

In D138940#4031366, @erichkeane wrote:

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.

In D138329#4027490, @thakis wrote:

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.

LGTM!

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

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

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

Ooo thanks I'll fix asap.

Great!

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.

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.

LGTM!

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

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

In D140062#4000094, @jkorous wrote:

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.

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.