Skip to content
This repository has been archived by the owner on Aug 22, 2024. It is now read-only.

Fix issues around Z3 sorts and floating-point flags. #1253

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

Zeta611
Copy link
Contributor

@Zeta611 Zeta611 commented Nov 18, 2023

Description

  1. Properly set the floating-point flag when get_as_numeric_z3_ast meets the Expression::Transmute case, by checking the target_type and dispatching to get_as_numeric_z3_ast.
  2. Fix the Z3 sort of the return type to be BitVector, not Boolean, in bv_overflows.

Fixes #1252.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • API change with a documentation update
  • Additional test coverage
  • Code cleanup or just keeping up with the latest Rustc nightly

How Has This Been Tested?

Analyzing libm failed previously. Now it works.

Checklist:

I have run validate.sh.

  • Fork the repo and create your branch from main.
  • If you've added code that should be tested, add tests.
  • If you've changed APIs, update the documentation.
  • Ensure the test suite passes.
  • Make sure your code lints.
  • If you haven't already, complete your CLA here: https://code.facebook.com/cla

Fixes issues with integer binary operations with floating point numbers
transmuted to integers. Fixes facebookexperimental#1252 (comment)
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 18, 2023
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a23ff7e) 74% compared to head (e49cedd) 74%.

Files Patch % Lines
checker/src/z3_solver.rs 37% 5 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1253   +/-   ##
=====================================
- Coverage     74%     74%   -1%     
=====================================
  Files         23      23           
  Lines      16795   16800    +5     
=====================================
- Hits       12592   12587    -5     
- Misses      4203    4213   +10     

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

Expression::Transmute { operand, .. } => {
self.get_as_numeric_z3_ast(&operand.expression)
Expression::Transmute { operand, target_type } => {
if target_type.is_integer() || target_type.is_floating_point_number() {
Copy link
Contributor

Choose a reason for hiding this comment

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

numeric_cast has logic for target_type == ExpressionType::Char, so it probably makes sense to use it for that case as well. I can imagine that transmuting a bit vector to a char might just happen somewhere, someday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you implying that simply dispatching to numeric_cast would be enough in the case of Transmute, without the conditional to fall back to get_as_numeric_z3_ast? I can see that it will fall back to get_as_numeric_z3_ast anyways. If you confirm, I will make a new commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll still need the fall back. The idea would be to also use numeric_cast only in the case where target_type == ExpressionType::Char. I imagine that it might be interesting to write a test case for this.

Copy link
Contributor Author

@Zeta611 Zeta611 Nov 19, 2023

Choose a reason for hiding this comment

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

But if I add Char type test like:

Expression::Transmute { operand, target_type } => {
    if target_type.is_integer() || target_type.is_floating_point_number() || target_type == ExpressionType::Char {
        self.numeric_cast(&operand.expression, *target_type)
    } else {
        self.get_as_numeric_z3_ast(&operand.expression)
    }
}

it would be the same as

Expression::Transmute { operand, target_type } => self.numeric_cast(&operand.expression, *target_type),

because numeric_cast basically falls back to calling get_as_numeric_z3_ast internally.

Or perhaps I misunderstood your intent!

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind then. I'll look into this separately when I get a chance.

@hermanventer
Copy link
Contributor

Thanks for doing this!

@hermanventer hermanventer merged commit a94a8c7 into facebookexperimental:main Nov 19, 2023
8 of 9 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic while compiling libm
3 participants