Skip to content

disambiguate two exprts with same ID #4479

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

Conversation

owen-mc-diffblue
Copy link
Contributor

The first commit is cherry-picked from #2646. I thought it deserved to be merged, even if the rest of that PR is not being worked on. I also implemented a suggestion from @smowton on that PR to make a new class is_invalid_pointer_exprt. The same could be done for many other pointer predicates, but I do not have time right now.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • 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).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Looks good except one minor correction

@@ -217,7 +217,7 @@ IREP_ID_ONE(invalid)
IREP_ID_TWO(C_invalid_object, #invalid_object)
IREP_ID_ONE(pointer_offset)
IREP_ID_ONE(pointer_object)
IREP_ID_TWO(is_invalid_pointer, is-invalid-pointer)
IREP_ID_TWO(is_invalid_pointer, is_invalid_pointer)
Copy link
Contributor

Choose a reason for hiding this comment

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

IREP_ID_ONE

@owen-mc-diffblue owen-mc-diffblue force-pushed the fix/disambiguate-two-exprts-with-same-id branch from 092802b to a6b6ddb Compare April 2, 2019 16:06
klaas and others added 2 commits April 2, 2019 21:59
This commit resolves an issue where ID_dynamic_object was used to
label two semantically distinct exprts.

One, with a single operand, was a boolean expression meaning that
the operand is a pointer to a dynamic object. This has been renamed
to ID_is_dynamic_object.

The second, with two operands, is an exprt representing a dynamic
object itself. This has stayed ID_dynamic_object.

Disambiguating which meaning was intended in each case was relatively
easy, as uses of these exprts frequently come with assertions
about the number of operands, and so this was used to inform which
instances of ID_dynamic_object should be changed and which should
be left the same.
@owen-mc-diffblue owen-mc-diffblue force-pushed the fix/disambiguate-two-exprts-with-same-id branch from a6b6ddb to e4d3ac6 Compare April 2, 2019 21:00
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: e4d3ac6).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/106816492

@tautschnig
Copy link
Collaborator

The first commit is cherry-picked from #2646. I thought it deserved to be merged, even if the rest of that PR is not being worked on.

Thank you! FWIW, those patches and related PRs will all see attention as @karkhaz is bringing them back to life (starting from #4332).

@@ -12,6 +12,7 @@ Author: Daniel Kroening, [email protected]
#ifndef CPROVER_UTIL_POINTER_PREDICATES_H
#define CPROVER_UTIL_POINTER_PREDICATES_H

#include "std_expr.h"
#define SYMEX_DYNAMIC_PREFIX "symex_dynamic::"

class exprt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of those forward declarations will now be unnecessary given the added include.

@@ -33,7 +33,6 @@ exprt good_pointer_def(const exprt &pointer, const namespacet &);
exprt null_object(const exprt &pointer);
exprt null_pointer(const exprt &pointer);
exprt integer_address(const exprt &pointer);
exprt is_invalid_pointer(const exprt &pointer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that's ok, though generally we should first mark such functions as deprecated.

@owen-mc-diffblue owen-mc-diffblue force-pushed the fix/disambiguate-two-exprts-with-same-id branch from e4d3ac6 to a9774ca Compare April 3, 2019 09:08
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: a9774ca).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/106877160

@owen-mc-diffblue owen-mc-diffblue merged commit 23af75f into diffblue:develop Apr 3, 2019
@owen-mc-diffblue owen-mc-diffblue deleted the fix/disambiguate-two-exprts-with-same-id branch April 3, 2019 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants