Skip to content

Fix simplification of pointer-object comparison [blocks: #4628] #4627

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

Merged

Conversation

tautschnig
Copy link
Collaborator

  1. pointer_object((T1 *)NULL) equals pointer_object((T2 *)NULL) for any
    types T1, T2. Previously, this would return false, unless T1==T2.
  2. Do not restrict the above to NULL, but instead let the existing logic
    in simplify_inequality fully simplify this.
  3. Add a unit test of this code, which highlighted further bugs and
    limitations: the unit test previously did not set the instance of the
    desired dynamic object, and address-of inequalities over dynamic objects
    can also be simplified.
  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@tautschnig tautschnig changed the title Fix simplification of pointer-object comparison Fix simplification of pointer-object comparison [blocks: #4628] May 8, 2019
@tautschnig tautschnig force-pushed the fix-pointer-object-simplification branch from 68d1fdc to fbf7968 Compare May 8, 2019 14:09
@tautschnig tautschnig force-pushed the fix-pointer-object-simplification branch from 3c9f1b1 to c2a516a Compare May 8, 2019 15:29
@smowton
Copy link
Contributor

smowton commented May 8, 2019

The unit test doesn't currently cover all the added behaviour (e.g. the line @owen-jones-diffblue suggested you change)

1) pointer_object((T1 *)NULL) equals pointer_object((T2 *)NULL) for any
types T1, T2. Previously, this would return false, unless T1==T2.
2) Do not restrict the above to NULL, but instead let the existing logic
in simplify_inequality fully simplify this.
3) Add a unit test of this code, which highlighted further bugs and
limitations: the unit test previously did not set the instance of the
desired dynamic object, and address-of inequalities over dynamic objects
can also be simplified.
@tautschnig tautschnig force-pushed the fix-pointer-object-simplification branch from c2a516a to 359a063 Compare May 12, 2019 17:46
@tautschnig
Copy link
Collaborator Author

Unit test extended (and locally tested that it does fail without @owen-jones-diffblue's fix).

{
new_inequality_ops.push_back(typecast_exprt::conditional_cast(
op, new_inequality_ops.front().type()));
simplify_node(new_inequality_ops.back());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought for the future: in this case I think we have a very particular simplification in mind -- presumably that nulls are typed so the cast might be folded, and that (T*)(S*)x can become (T*)x. However bluntly using the simplifier always risks that a much more expensive transformation might be attempted. This suggests factoring out particular simplifications as utility methods, used both during simplification and as the routine way of creating a pointer cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That certainly is something to review all the simplifier code for. I actually have some patches in a branch already, but it's not quite a systematic review yet. Will work on this in a separate PR.

@tautschnig tautschnig merged commit 80c47c9 into diffblue:develop May 13, 2019
@tautschnig tautschnig deleted the fix-pointer-object-simplification branch May 13, 2019 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants