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

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

In D126481#3545350, @martong wrote:

This part of the SValBuilder is responsible for constant folding. We need this constant folding, so the engine can work with less symbols, this way it can be more efficient. Whenever a symbol is constrained with a constant then we substitute the symbol with the corresponding integer. If a symbol is constrained with a range, then the symbol is kept and we fall-back to use the range based constraint manager, which is not that efficient. This patch is the natural extension of the existing constant folding machinery with the support of SymbolCast symbols.

Now I see. Thanks. I also wouldn't mind if you add this explanation to the summary.

@martong As you said, my solution D103096 suppose to pass these and more other tests cases. So how it will help in combination with my solution D103096?
Although, your patch is really simple but it's more like a plug then a real SymbolCast support, isn't it? I don't quite understand the motivation.

In D103096#3502955, @martong wrote:

Ping

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.

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

Thank you for the review @martong! Your work is not less hard than mine. I'll rework and update the revision ASAP.

Loaded with comment updateds according to the remarks.
@martong thank you for your time for the review!

Updated according to suggestions.
@martong thank you for the review.

Thanks! LGTM.

LGTM

I checked the tests file on the latest sources. It passes even without your changes. Maybe this patch is already outdated.

Here are my remarks.

Ping. If there is somebody interested in this? :)

Ping.

Paused for a while.

Thank you @martong.
I'll load it ASAP. It's great to see symcasts is closer.

In D105340#3232671, @NoQ wrote:

This looks great with the option flag. Landing this patch will enable more people to test the new mode and produce feedback on whether the constraint solver keeps working well enough in presence of the new symbols.

Improved the checker. Reworked tests.

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.

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.

Just a ping. I'd like to have this patch stack loaded somewhen :)

Improved AccessInferrer::isOppositeSign. Reworked tests.
TODO: Write more synthetic and real-world tests.

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

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

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

Changed handler check:: functions. Reworked. Covered more cases.

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

Fixed code formatting in the unit test file according to remarks. Ready to load.

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

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.

Passed a CodeGenOptions reference to CheckerManager as well. (Adjusted to D114718)

@martong

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.

@xazax.hun Updated the summary for better undersanding differencies between the Standards.

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

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.

In D114006#3142313, @martong wrote:

LGTM!

Rebased.

Rebased.

Fixed missed part during rebasing in the unit test.

Rebased.

Fixed comments.

@steakhal Please, read the discussion started from here D104285#2943449. It's directly relates to this patch and what we've been arguing about.

In D113588#3137584, @steakhal wrote:

How to address this?

@NoQ

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.

In D114006#3135456, @steakhal wrote:

Sweet!
I think commit titles are not punctuated, even though they start with an uppercase letter.

Fixed failed unittest (Syntax error).

@steakhal

I think I know.

Great! Thank you!

@steakhal for the changes.
Please, add to summary several examples of the feature usage.

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

In D111795#3121851, @steakhal wrote:

We could, but we would definitely pay the price. So, I'm not really a fan of doing that.

That's fair. Performance of regex really is a weak place.