Looser check for overflowing_binary_op #91856

ouz-a · 2021-12-13T09:43:12Z

Fix for issue #91636 tight check resulted in ICE, this makes the check a little looser. It seems eq allows comparing of supertype and subtype if lhs = supertype and rhs = subtype but not vice versa, is this intended behavior ?

rust-highfive · 2021-12-13T09:43:15Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-12-13T09:43:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

oli-obk · 2021-12-13T11:50:03Z

Higher kinded lifetimes strike again... I think the real fix would be to use the subtyping infra, but that could be rather expensive for such a common op. Although it short circuits on type equality, so it should be cheap most of the time

ouz-a · 2021-12-13T12:03:41Z

Higher kinded lifetimes strike again... I think the real fix would be to use the subtyping infra, but that could be rather expensive for such a common op. Although it short circuits on type equality, so it should be cheap most of the time

I tried other things to fix this but they broke a lot of stuff 😸 I will look at subtyping infra, thanks!

oli-obk · 2021-12-13T13:02:39Z

I think something like tcx.at(span).sub(rhs.ty, lhs.ty).is_ok() should work

oli-obk · 2021-12-13T13:03:49Z

Or maybe check how mir validation does it, tho I fear it just bails out on such lifetimes

ouz-a · 2021-12-13T13:05:58Z

Or maybe check how mir validation does it, tho I fear it just bails out on such lifetimes

I think because of coercion mir gives up. As I posted in the issue if I use println! program compiles but there are bunch of MIR errors.

oli-obk · 2021-12-13T13:16:21Z

OK, I guess if the subtyping check works here, we should make mir validation use it, too?

ouz-a · 2021-12-13T13:37:33Z

Again for such a small case wouldn't adding an extra layer of subtyping check make it slow ? 😕

ouz-a · 2021-12-13T13:58:25Z

I think something like tcx.at(span).sub(rhs.ty, lhs.ty).is_ok() should work

Also tcx.at(span) doesn't have sub method did you meant infcx ?

oli-obk · 2021-12-13T14:07:03Z

Oh... I did... yea... spawning an infcx is serious overkill...

OK, let's keep the PR as it is, doing the real check is too expensive for CTFE

@RalfJung that ok with you?

r? @RalfJung

RalfJung · 2021-12-13T15:50:06Z

OK, I guess if the subtyping check works here, we should make mir validation use it, too?

I think MIR validation currently doesn't check much about operators (unary and binary alike), so that sounds like a separate PR to me.

RalfJung · 2021-12-13T15:50:49Z

compiler/rustc_const_eval/src/interpret/operator.rs

@@ -330,7 +330,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            _ if left.layout.ty.is_any_ptr() => {
                // The RHS type must be the same *or an integer type* (for `Offset`).


Please update this comment, adding a link to #91636.

RalfJung · 2021-12-13T15:52:11Z

Yeah I am fine with this change. I proposed it. :)

@ouz-a could you add #91636 (comment) add a regression test? Other than that and the comment nit I raised, this looks good to me!

ouz-a · 2021-12-13T16:06:59Z

Going to update the comment and add a regression test. Thanks!

oli-obk · 2021-12-14T08:03:15Z

@bors r+

bors · 2021-12-14T08:03:17Z

📌 Commit b85762f has been approved by oli-obk

oli-obk · 2021-12-14T08:03:27Z

@bors rollup

RalfJung · 2021-12-14T14:20:19Z

compiler/rustc_const_eval/src/interpret/operator.rs

@@ -328,9 +328,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                self.binary_int_op(bin_op, l, left.layout, r, right.layout)
            }
            _ if left.layout.ty.is_any_ptr() => {
-                // The RHS type must be the same *or an integer type* (for `Offset`).
+                // The RHS type must be a `pointer` *or an integer type* (for `Offset`).
+                // (This is workaround for the issue #91636)


Suggested change

// (This is workaround for the issue #91636)

// (Even when both sides are pointers, their type might differ, see issue #91636)

RalfJung · 2021-12-14T14:21:02Z

Comment nit
@bors r-

oli-obk · 2021-12-14T17:08:53Z

@bors r+ rollup

bors · 2021-12-14T17:08:54Z

📌 Commit a5054e3 has been approved by oli-obk

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#89825 (Make split_inclusive() on an empty slice yield an empty output) - rust-lang#91239 (regression test for issue 87490) - rust-lang#91597 (Recover on invalid operators `<>` and `<=>`) - rust-lang#91774 (Fix typo for MutVisitor) - rust-lang#91786 (Return an error when `eval_rvalue_with_identities` fails) - rust-lang#91798 (Avoid suggest adding `self` in visibility spec) - rust-lang#91856 (Looser check for overflowing_binary_op) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

add regression test Adds a regression test for rust-lang/rust#91636 (which was fixed by rust-lang/rust#91856)

Looser check for binary_op_overflow

01948e2

rust-highfive assigned cjgillot Dec 13, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2021

This comment has been minimized.

Sign in to view

formatting

a01b13d

rust-highfive assigned RalfJung and unassigned cjgillot Dec 13, 2021

RalfJung reviewed Dec 13, 2021

View reviewed changes

Add regression test and comment

b6c8098

This comment has been minimized.

Sign in to view

test should pass 😅

b85762f

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2021

RalfJung reviewed Dec 14, 2021

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2021

comment update

a5054e3

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2021

matthiaskrgr mentioned this pull request Dec 14, 2021

Rollup of 7 pull requests #91933

Merged

bors merged commit bae9270 into rust-lang:master Dec 15, 2021

rustbot added this to the 1.59.0 milestone Dec 15, 2021

This was referenced Dec 15, 2021

Compiler panics when comparing functions #91636

Closed

add regression test rust-lang/miri#1938

Merged

bors added a commit to rust-lang/miri that referenced this pull request Dec 15, 2021

Auto merge of #1938 - RalfJung:test, r=RalfJung

7f0e652

add regression test Adds a regression test for rust-lang/rust#91636 (which was fixed by rust-lang/rust#91856)

bors added a commit to rust-lang/miri that referenced this pull request Dec 15, 2021

Auto merge of #1938 - RalfJung:test, r=RalfJung

51e9630

add regression test Adds a regression test for rust-lang/rust#91636 (which was fixed by rust-lang/rust#91856)

bors added a commit to rust-lang/miri that referenced this pull request Dec 15, 2021

Auto merge of #1938 - RalfJung:test, r=RalfJung

2eb3b37

add regression test Adds a regression test for rust-lang/rust#91636 (which was fixed by rust-lang/rust#91856)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Looser check for overflowing_binary_op #91856

Looser check for overflowing_binary_op #91856

ouz-a commented Dec 13, 2021

rust-highfive commented Dec 13, 2021

rust-highfive commented Dec 13, 2021

This comment has been minimized.

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021 •

edited

Loading

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

RalfJung commented Dec 13, 2021

RalfJung Dec 13, 2021

RalfJung commented Dec 13, 2021

ouz-a commented Dec 13, 2021

This comment has been minimized.

oli-obk commented Dec 14, 2021

bors commented Dec 14, 2021

oli-obk commented Dec 14, 2021

RalfJung Dec 14, 2021

RalfJung commented Dec 14, 2021

oli-obk commented Dec 14, 2021

bors commented Dec 14, 2021

		@@ -330,7 +330,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
		_ if left.layout.ty.is_any_ptr() => {
		// The RHS type must be the same or an integer type (for `Offset`).

	// (This is workaround for the issue #91636)
	// (Even when both sides are pointers, their type might differ, see issue #91636)

Looser check for overflowing_binary_op #91856

Looser check for overflowing_binary_op #91856

Conversation

ouz-a commented Dec 13, 2021

rust-highfive commented Dec 13, 2021

rust-highfive commented Dec 13, 2021

This comment has been minimized.

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

ouz-a commented Dec 13, 2021 • edited Loading

ouz-a commented Dec 13, 2021

oli-obk commented Dec 13, 2021

RalfJung commented Dec 13, 2021

RalfJung Dec 13, 2021

Choose a reason for hiding this comment

RalfJung commented Dec 13, 2021

ouz-a commented Dec 13, 2021

This comment has been minimized.

oli-obk commented Dec 14, 2021

bors commented Dec 14, 2021

oli-obk commented Dec 14, 2021

RalfJung Dec 14, 2021

Choose a reason for hiding this comment

RalfJung commented Dec 14, 2021

oli-obk commented Dec 14, 2021

bors commented Dec 14, 2021

ouz-a commented Dec 13, 2021 •

edited

Loading