User Details
- User Since
- Sep 3 2015, 9:16 AM (393 w, 4 d)
Today
Thu, Mar 16
Other than my usual complaint about declRefExpr(to(varDecl())), LGTM!
Other than the to(varDecl()) discussion, LGTM!
Mon, Mar 13
I guess we can abandon D143133 now in favor of this patch?
LGTM, let's land asap!
Wed, Mar 8
Looks great overall!
Tue, Mar 7
(these tests can also demonstrate that your fix for _Generic is correct!)
Aha, looks correct now!
Ok I think this is good to go!
Thu, Mar 2
Wed, Mar 1
Hi, looks very interesting! We definitely have troubles with some initializer lists.
Mon, Feb 27
Thu, Feb 23
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.
Yeah looks like I replied without properly reading the patch.
Wed, Feb 22
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!
Tue, Feb 21
I completely agree with @steakhal, these should be note tags:
Feb 16 2023
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.
Feb 15 2023
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 14 2023
Looks great! Nitpicks.
Feb 9 2023
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 8 2023
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 7 2023
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.
Feb 6 2023
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.
Feb 3 2023
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.
Jan 31 2023
Jan 30 2023
Nice! I have some nitpicks.
As for me, this looks like it's good to go! Thanks Rashmi for addressing all the comments!
Jan 27 2023
Thank you!! I think we're almost ready to commit but this concern is still hanging:
Jan 26 2023
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!
Jan 25 2023
Jan 24 2023
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?
Jan 19 2023
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!
Jan 18 2023
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 17 2023
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!
Jan 12 2023
Looks awesome!
LGTM! I like the new diagnostics a lot better!
Jan 10 2023
I like this, this is much easier to work with than my original speculative spaghetti!
Looks awesome!
Jan 6 2023
Jan 4 2023
Jan 3 2023
LGTM!
Ok LGTM! I hope we'll get a chance to implement a reusable solution later🤞
Dec 20 2022
I suppressed the assertion in rG8fd62e7. It looks like we'll need a deeper investigation into this.
Dec 19 2022
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.
Dec 16 2022
Great!
Dec 15 2022
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.
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.