Skip to content

Conversation

MathiasVP
Copy link
Contributor

Consider this snippet:

void notequal_refinement(short n) {
  if (n < 0) {
    range(n);
    return;
  }
  ...
}

the n in n < 0 has an implicit short-to-int conversion which means that the left-hand side of < is not actually a LoadInstruction of n, but instead a ConvertInstruction of the result of a LoadInstruction.

We could fix up all of these by doing a transitive closure from the LoadInstruction, through all "safe conversions" and figure out if we reached the operand of <. This is a bit ugly though, and we'd have to do this in multiple places.

Instead, this PR uses the new-ish QlBuiltins::EquivalenceRelation feature to define a new type that unifies unconverted instructions with their safe conversions. This means that the n in n < 0 really is the LoadInstruction (because the LoadInstruction and the ConvertInstruction is mapped to the same Expr).

@MathiasVP MathiasVP requested a review from a team as a code owner March 21, 2023 16:45
@github-actions github-actions bot added the C++ label Mar 21, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Mar 21, 2023
@MathiasVP MathiasVP requested a review from rdmarsh2 March 21, 2023 16:47
@MathiasVP
Copy link
Contributor Author

CI failure is unrelated (It's complaining about autoformatting in a couple of Java queries).

@MathiasVP MathiasVP force-pushed the skip-safe-conversions-in-range-analysis branch 3 times, most recently from 3d865d7 to 4dcf5cb Compare March 22, 2023 13:24
@MathiasVP MathiasVP force-pushed the skip-safe-conversions-in-range-analysis branch from 4dcf5cb to cd7ba7c Compare March 22, 2023 16:51
@MathiasVP MathiasVP force-pushed the skip-safe-conversions-in-range-analysis branch from 176b28b to 61bafd3 Compare March 23, 2023 13:27
@MathiasVP
Copy link
Contributor Author

Looks like this is crashing the QL compiler 😭. It only crashes the compiler with assertions enabled, however. So I was still able to run DCA (which looks fantastic now 🎉).

@MathiasVP MathiasVP force-pushed the skip-safe-conversions-in-range-analysis branch from 29835ba to 61bac63 Compare March 27, 2023 20:24
@MathiasVP MathiasVP marked this pull request as draft March 27, 2023 22:08
@MathiasVP MathiasVP force-pushed the skip-safe-conversions-in-range-analysis branch from 61bac63 to 724d97e Compare March 27, 2023 22:14
@MathiasVP MathiasVP marked this pull request as ready for review March 27, 2023 22:15
Comment on lines 25 to 35
exists(IR::ConvertInstruction convert |
this = convert and
tFrom = getSemanticType(convert.getUnary().getResultIRType()) and
tTo = getSemanticType(convert.getResultIRType())
)
or
exists(IR::CopyValueInstruction copy |
this = copy and
tFrom = getSemanticType(copy.getUnary().getResultIRType()) and
tTo = getSemanticType(copy.getResultIRType())
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style point, but I don't see why this can't just be

        tFrom = getSemanticType(this.getUnary().getResultIRType()) and
        tTo = getSemanticType(this.getResultIRType())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Fixed in 4602a8a.

rank[n + 1](IR::Instruction instr, int i, IR::IRBlock block |
this = Equiv::getEquivalenceClass(instr) and block.getInstruction(i) = instr
|
instr order by i
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be a situation where a Conversion or CopyValue appears in a different block than its operand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never encountered such a case, no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this query:

from ConvertInstruction instr
where instr.getUnary().getBlock() != instr.getBlock()
select instr

did produce up about ~30 results on Wireshark (all related to the ternary operator). I've added d9b2a72 to ensure that we only collapse instructions in the same block.

That was a good catch!

@rdmarsh2 rdmarsh2 merged commit d03dd49 into github:main Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants