Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove custom octagon analysis, make Apron Analysis answer MustBeEqual, MayBeEqual, and MayBeLess queries #500

Merged
merged 16 commits into from
Dec 17, 2021

Conversation

jerhard
Copy link
Member

@jerhard jerhard commented Dec 16, 2021

As we have working Apron bindings, the custom implemented octagon analysis is now superfluous. Also, the OctagonAnalysis did not take different ikinds of integers into account, and was coupled with some parts of IntDomain, making changes there more costly to do.

Part of #460.

We have a working binding to Apron that offers octagons among some other relational domains.
The octagon implementation that is removed here was not designed with
handling different ikinds correctly, instead it used an int domain with
some default ikind.
… not used by partitioned arrays (j is the partitioning expression)
@jerhard jerhard added the cleanup Refactoring, clean-up label Dec 16, 2021
@jerhard
Copy link
Member Author

jerhard commented Dec 16, 2021

The test 23/13-advantage_for_last.c for was annotated to use octagons but didn't need them, so I simply removed the option annotation.

There were two other tests for the octagon analysis together with partitioned arrays ("22/10 array_octagon", "23/10 array_octagon" ) and copies of these for the precision adaption ("42/14 22_10-array_octagon", "42/20 23_10-array_octagon"). I changed the test cases to use the Apron analysis instead of the Octagon analysis, but these test cases fail now due to imprecision.

@jerhard
Copy link
Member Author

jerhard commented Dec 16, 2021

Interestingly, the assert in line 56 fails, while the asserts in lines 54, 55 succeed, and the partitioned array knows that it is partitioned by j, and that the left part of the array has value 1, and the middle part has value 2. Maybe the apron analysis is somehow not queried by the partitioned arrays/the apron analysis does not answer?

z = j;
// Values that may be read are 1 or 2
assert(a[z] == 1); //UNKNOWN
assert(a[z] == 2); //UNKNOWN
assert(z >= 0);
assert(z <= j);
assert(a[z] == 0); //FAIL

@jerhard
Copy link
Member Author

jerhard commented Dec 16, 2021

I would suggest to annotate the tests in question with SKIP and create a separate issue about these. Any objections?

@jerhard jerhard changed the title Remove old octagon analysis Remove custom octagon analysis Dec 16, 2021
@sim642
Copy link
Member

sim642 commented Dec 16, 2021

I think the octagon analysis answers queries like MustBeEqual which partitioned arrays use, but the apron analysis doesn't implement them. It shouldn't be too hard to add support for them though.

@jerhard jerhard changed the title Remove custom octagon analysis Remove custom octagon analysis, make Apron Analysis answer MustBeEqual, MayBeEqual, and MayBeLess queries Dec 16, 2021
@jerhard jerhard requested a review from sim642 December 16, 2021 12:17
@jerhard
Copy link
Member Author

jerhard commented Dec 16, 2021

@sim642 I extended the apron analysis to also answer the MustBeEqual, MayBeEqual and MayBeLess queries.
Most asserts in the both test cases now work, except for a few. And some asserts now can be shown that previously couldn't.

@sim642
Copy link
Member

sim642 commented Dec 16, 2021

Maybe @michael-schwarz knows what aspects the octagon analysis had that the apron analysis doesn't that it's not precise enough for that. Did the octagon analysis have something very specific for the partitioned arrays or maybe it was just more precise because it was unsound?

@michael-schwarz
Copy link
Member

michael-schwarz commented Dec 16, 2021

Did the octagon analysis have something very specific for the partitioned arrays or maybe it was just more precise because it was unsound?

No, it did not have anything very specific. I think it completely ignored overflows, so maybe that is why?

@sim642 sim642 self-requested a review December 17, 2021 07:58
The folder 36-apron would otherwise have 100 testcases, causing problems with the script that claims there would be a duplicate tests case 36/10
@jerhard jerhard merged commit 1e9fc97 into master Dec 17, 2021
@jerhard jerhard deleted the remove_old_octagon branch December 17, 2021 12:19
@sim642 sim642 added this to the v2.0.0 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactoring, clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants