Skip to content

Fix shadow memory missing aggregation on non-compound bitvector types #7935

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

esteffin
Copy link
Contributor

@esteffin esteffin commented Oct 3, 2023

Fix shadow memory missing max aggregation when the type is a non-compound bitvector of size > 1 byte.

Fix bug in duplicate_per_byte when the init_expr has signed type.

Fix wrong regression and unit tests.

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

@esteffin esteffin requested a review from a team October 3, 2023 15:29
@esteffin esteffin force-pushed the esteffin/sm-aggregate-on-non-byte-bv branch 2 times, most recently from 3d6294e to aa39c40 Compare October 3, 2023 17:03
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (c435f43) 78.62% compared to head (3251ce4) 78.62%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7935      +/-   ##
===========================================
- Coverage    78.62%   78.62%   -0.01%     
===========================================
  Files         1701     1701              
  Lines       195987   195975      -12     
===========================================
- Hits        154097   154087      -10     
+ Misses       41890    41888       -2     
Files Coverage Δ
src/goto-symex/shadow_memory_util.cpp 92.19% <100.00%> (-0.30%) ⬇️
src/util/expr_initializer.cpp 84.21% <100.00%> (+0.24%) ⬆️
unit/util/expr_initializer.cpp 99.74% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


// Iterator for the other elements in the collection in the interval denoted
// by `expr_iterator` and `end`.
std::vector<exprt>::const_iterator expr_to_compare_to = expr_iterator + 1;
Copy link
Member

Choose a reason for hiding this comment

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

std::next ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

member_exprt(expr, component), field_type, ns, log, is_union, max);
}
return max;
// TODO: sure unsigned_long_int_type()?
Copy link
Member

Choose a reason for hiding this comment

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

yes! char won't be enough

/// \param field_type the type of the shadow memory field to return
/// \param ns the namespace to perform type-lookups into
/// \return the aggregated max byte-sized value contained in expr
/// Note that the expr type size must be known at compile time.
Copy link
Member

Choose a reason for hiding this comment

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

❓ Where is this checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an invariant of boolbv_widtht::operator() that we use to get the size here.

It would be nice to avoid the invariant by doing a check earlier, but unfortunately that's harder to do as it would involve a recursive check on the type structure.

If necessary it is possible to add it in, but I would defer it to a future PR

__CPROVER_set_field(a + 1, "uninitialized", 1);

assert(__CPROVER_get_field(a, "uninitialized") == 0);
assert(__CPROVER_get_field(a + 1, "uninitialized") == 1);
Copy link
Member

Choose a reason for hiding this comment

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

I'd check the remaining bytes too that they are 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

When using duplicate_per_byte with signed bv, if the bv was casted to a
bigger unsigned bv the sign-extension was performed, so the obtained bv was
non-zero on the least significant part and interfeering with the replicated
value.
@NlightNFotis NlightNFotis force-pushed the esteffin/sm-aggregate-on-non-byte-bv branch from aa39c40 to 5e0547e Compare October 4, 2023 10:10
@esteffin esteffin force-pushed the esteffin/sm-aggregate-on-non-byte-bv branch from 5e0547e to 055621d Compare October 4, 2023 10:52
Fix missing aggregation when the shadow memory element type is a
non-compound bitvector of size > 1 byte.
@esteffin esteffin force-pushed the esteffin/sm-aggregate-on-non-byte-bv branch from 055621d to e555d0d Compare October 4, 2023 11:12
Enrico Steffinlongo added 2 commits October 4, 2023 13:12
Fix shadow memory regression tests that was not aggregating correctly
when the type of the element is a non-compound bit vector of size > 1
byte.

Also added extra checks on the regression test.
@esteffin esteffin force-pushed the esteffin/sm-aggregate-on-non-byte-bv branch from e555d0d to 3251ce4 Compare October 4, 2023 12:12
@NlightNFotis NlightNFotis merged commit f33b96a into diffblue:develop Oct 4, 2023
@esteffin esteffin deleted the esteffin/sm-aggregate-on-non-byte-bv branch October 4, 2023 13:54
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.

3 participants