rustc_codegen_ssa: fix range check in codegen_get_discr. #62584
Conversation
rust-highfive
added
the
S-waiting-on-review
|
This looks good, r=me once the test has been added (for the case where the niche would not be able to represent the uninhabited variants if they were actually assigned distinct discriminant values). |
| bx.cx().const_uint_big(niche_llty, niche_start) | ||
| bx.cx().const_uint(niche_llty, relative_max as u64) | ||
| }; | ||
| bx.icmp(IntPredicate::IntULE, relative_discr, relative_max) |
There was a problem hiding this comment.
If relative_discr is re-projected to 0..=n, doesn’t this actually compute is_proper_variant rather than is_niche (the variable name this gets assigned to)? That is, I would expect any niche discriminant to be in n+1..niche_llty::max_value() and this appears to check 0..=n <= n.
This appears to be reinforced by the bx.select below, which selects niche_discr when is_niche == 0.
There was a problem hiding this comment.
select(c, a, b) is c ? a : b I'm pretty sure.
In 0..=n, n is the number of niche(-using) variants, i.e. relative_discr == 0 is the first niche-using variant, and it goes up from there.
eddyb
force-pushed
the
circular-math-is-hard
branch
from
42d19df to
c063057
Compare
|
@bors r=pnkfelix |
|
📌 Commit c063057 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 rollup=never -- I want this tested alone since it is sensitive (soundness) and I want bisection to be easier. |
rustc_codegen_ssa: fix range check in codegen_get_discr. Fixes #61696, see #61696 (comment) for more details. In short, I had wanted to use `x - a <= b - a` to check whether `x` is in `a..=b` (as it's 1 comparison instead of 2 *and* `b - a` is guaranteed to fit in the same data type, while `b` itself might not), but I ended up with `x - a + c <= b - a + c` instead, because `x - a + c` was the final value needed. That latter comparison is equivalent to checking that `x` is in `(a - c)..=b`, i.e. it also includes `(a - c)..a`, not just `a..=b`, so if `c` is not `0`, it will cause false positives. This presented itself as the non-niche ("dataful") variant sometimes being treated like a niche variant, in the presence of uninhabited variants (which made `c`, aka the index of the first niche variant, arbitrarily large). r? @nagisa, @rkruppe or @oli-obk
|
@bors p=5 |
|
☀️ Test successful - checks-azure, checks-travis, status-appveyor |
Fixes #61696, see #61696 (comment) for more details.
In short, I had wanted to use
x - a <= b - ato check whetherxis ina..=b(as it's 1 comparison instead of 2 andb - ais guaranteed to fit in the same data type, whilebitself might not), but I ended up withx - a + c <= b - a + cinstead, becausex - a + cwas the final value needed.That latter comparison is equivalent to checking that
xis in(a - c)..=b, i.e. it also includes(a - c)..a, not justa..=b, so ifcis not0, it will cause false positives.This presented itself as the non-niche ("dataful") variant sometimes being treated like a niche variant, in the presence of uninhabited variants (which made
c, aka the index of the first niche variant, arbitrarily large).r? @nagisa, @rkruppe or @oli-obk