User Details
- User Since
- Mar 17 2020, 10:23 AM (120 w, 1 d)
Tue, Jul 5
@aganea Thank you for fixing this.
TestValues structure impies to hold a set of values which can do some kind of convertions including truncations. This is what tests are about. That's true, it may happen that some test cases don't need some values. You can carry them out to some TestValues2 structure and use them instead. That also would work. So it's up to you. I'm OK with both solutions.
Mon, Jun 27
@martong Just FYI. I've been working on reworking this solution to using EquivalenceClasses for several weeks. It turned out that this is an extremely hard task to acomplish. There a lot of cast cases like: (int8)x==y, (uint16)a==(int64)b, (uint8)y == b, Merging and inferring all of this without going beyond the complexity O(n) is really tricky.
May 30 2022
Now I see. Thanks. I also wouldn't mind if you add this explanation to the summary.
May 27 2022
May 19 2022
May 13 2022
Thank you, folk, for taking you time. I'll surely make corresponding changes according to your suggestions and notify you then. Sorry, @martong, for the late response. I'm pretty loaded recent times.
Apr 26 2022
@martong thank you for the idea. I've tried to implement it. Could you look at the patch once again, please? I've also described a new solution in the Summary.
Apr 22 2022
Thank you for the review @martong! Your work is not less hard than mine. I'll rework and update the revision ASAP.
Apr 19 2022
Loaded with comment updateds according to the remarks.
@martong thank you for your time for the review!
Apr 15 2022
Updated according to suggestions.
@martong thank you for the review.
Apr 14 2022
Apr 12 2022
Thanks! LGTM.
Apr 11 2022
LGTM
I checked the tests file on the latest sources. It passes even without your changes. Maybe this patch is already outdated.
Apr 7 2022
Here are my remarks.
Mar 23 2022
Ping. If there is somebody interested in this? :)
Ping.
Jan 27 2022
Paused for a while.
Jan 20 2022
Jan 18 2022
Jan 13 2022
Thank you @martong.
I'll load it ASAP. It's great to see symcasts is closer.
Jan 12 2022
Jan 11 2022
Improved the checker. Reworked tests.
Jan 10 2022
Starting producing SymbolCast you should be careful. It'll be helpfull for you to pay attention on my revisions D105340 and D103096 before doing this. I don't want you walk through the things I fought with. At least we've discussed to introduce a new flag support-symbolic-integer-casts to turn on/off producing the casts.
Dec 22 2021
Added more tests for records (classes, structs, unions). Improved AccessInferrer. Removed support of cast kinds such as DerivedToBase and BaseToDerived, since they are not implied in C++20 7.2.1 p11 paragraph.
TODO: White tests for multi-level casts, e.g.:
T1 a; // a auto* b = (T2*)&a; // {a, T2} auto* c = (T3*)b; // {{a, T2}, T3} auto* d = (T4*)c; // {{{a, T2}, T3}, T4} auto e = *d;
TODO: Fix case with loc::ConcreteInt, e.g.:
T1 *a = nullptr; // 0 loc auto* b = (T2*)&a; // 0 loc auto c = *d; // We are not able to detect an original type and aliased type from ConcreteInt.
Dec 21 2021
Just a ping. I'd like to have this patch stack loaded somewhen :)
Dec 20 2021
Improved AccessInferrer::isOppositeSign. Reworked tests.
TODO: Write more synthetic and real-world tests.
Dec 17 2021
@steakhal Thanks! I appreciate your comprehensive comment! I'll take everything you suggested into account.
Closed with rGda8bd972a33ad642d6237b5b95b31a2a20f46a35
@xazax.hun
Many thanks for the review.
Dec 16 2021
Dec 15 2021
@martong
Thanks for clarification.
TLDR, it is recommended to use the arcanist.
I'm not able to use arcanist. It doesn't work on Windows (at least I've tryed several ways to set up it).
Dec 14 2021
@martong wrote:
Denis, you can see in the Revision Contents that Diff 3 has the baseline commit 63a6348. When I check out 63a6348 then the newly added test file triggers the assertion about BO_Add.
Yes is see it:
I don't get this feature. Is it a parent or something? Please explain how to interpret this table. And how can I use it myself and when?
Changed handler check:: functions. Reworked. Covered more cases.
Dec 13 2021
@steakhal
I don't get this one. I've provided a bunch of tests, even annotated with no-crash comments where we crashed prior to this change.
I wasn't able to catch any crashes with your tests file (symbol-simplification-nonloc-loc.cpp) on the baseline before your patch (D115149#3180005). So I ask you to provide the concrete example you've caught which promted you to do this patch.
Dec 10 2021
Fixed code formatting in the unit test file according to remarks. Ready to load.
Dec 9 2021
@NoQ wrote:
I'm confident that there's a way to get it right, simply because the program under analysis is type-correct. If it's the simplifier, let's fix the simplifier.
I agree with your main thought. But I also believe there definitely are thing to improve everywhere. And this one could wait until we find the root cause and correct it somewhere before.
Dec 8 2021
It seems like your test file passes even before your patch. I've just checked it. My last pull from the baseline was on Nov 15.
Improved dynamic type recognition. Provided -fstrict-aliasing compiler flag dependency.
Still has issues with some cases (see it at the bottom of the test file). WIP.
Fixed unit test.
Dec 7 2021
Passed a CodeGenOptions reference to CheckerManager as well. (Adjusted to D114718)
Dec 3 2021
Nice, assiduous work!
Many thanks for your time! Your work is not less assiduous!
(LHS, RHS) swaps
it doesn't really matter, as the operation is comutative, but, yes, it does matter to depict the particular test case. I'll revise twise LHS-RHS's.
I am going to continue with the next patch in the stack. I recognize that the handling of casts is very important and problems related to not handling them occurs even more frequently as other parts of the engine evolve (e.g. https://reviews.llvm.org/D113753#3167134)
Aha. I saw you patch. I'll join to review it.
Dec 2 2021
@xazax.hun Updated the summary for better undersanding differencies between the Standards.
Nov 30 2021
Nov 29 2021
@rsmith, @aaron.ballman I kindly invite you to join the review process especially in a part of the Standard interpretation. Specifically @rsmith made proposals in this part of the Standard.
Nov 23 2021
Temporary suspended this revision in favor of making a new checker StrictAliasingChecker, which would define an access to values through unpermited types as Undefined Behavior according to certain statements of the current Standard.
Nov 19 2021
Rebased.
Rebased.
Fixed missed part during rebasing in the unit test.
Rebased.
Nov 18 2021
Fixed comments.
Nov 17 2021
@steakhal Please, read the discussion started from here D104285#2943449. It's directly relates to this patch and what we've been arguing about.
If we ever prove that strict aliasing is violated on a given execution path (while being enabled), the ideal thing to do is to terminate the analysis immediately by generating a sink. We can then optionally develop a checker that emits a warning in such cases.
thinking about warnings I assume that the Store will produce UndefinedVals and Undef-related checkers will catch them. But yes, you're right. They wouldn't know anything about the origin of such UndefinedVals.
For the cases where you eliminate possibilities through recognizing strict aliasing, I wonder if a note can be added to the bug report to notify the user that the strict aliasing rule was invoked to add a certain assumption.
I didn't elaborate an idea with a checker yet but, I think, it can be the next step. What about a mechanism which would store the reason of UndefinedVal for existing checkers? It could make any checker more detailed and flexible.
Nov 16 2021
Fixed failed unittest (Syntax error).
Nov 15 2021
Nov 12 2021
Nov 11 2021
@steakhal for the changes.
Please, add to summary several examples of the feature usage.
Nov 10 2021
@steakhal Thank you!
Please clang-format the patch to reflow the touched comment. Given that's done, you are good to go.
The comment is OK. clang-format doesn't complain on it.
Also, wait a couple days before committing to give chance to the rest of the reviewers to have a look.
Sure.
Updated according to comments.
That's fair. Performance of regex really is a weak place.