Fix bug in integer range matching #57978

varkor · 2019-01-29T22:22:29Z

Fixes #57894.

rust-highfive · 2019-01-29T22:22:40Z

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

varkor · 2019-01-29T22:23:03Z

r? @oli-obk

src/librustc_mir/build/matches/simplify.rs

oli-obk · 2019-01-30T14:00:11Z

src/librustc_mir/build/matches/simplify.rs

                };
                if let Some((min, max, sz)) = range {
                    if let (Some(lo), Some(hi)) = (lo.val.try_to_bits(sz), hi.val.try_to_bits(sz)) {
+                        let (lo, hi) = (lo ^ bias, hi ^ bias);


So this basically makes sure that unsigned comparison of signed numbers still works out?

Yeah. I'll add a comment.

rust-highfive · 2019-01-31T00:58:59Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0d7d7338:start=1548894141300430345,finish=1548894246135799271,duration=104835368926
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:04:03] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:03] tidy error: /checkout/src/librustc_mir/build/matches/simplify.rs:131: line longer than 100 chars
[00:04:03] tidy error: /checkout/src/librustc_mir/build/matches/simplify.rs:132: line longer than 100 chars
[00:04:03] tidy error: /checkout/src/librustc_mir/build/matches/simplify.rs:133: line longer than 100 chars
[00:04:04] some tidy checks failed
[00:04:04] 
[00:04:04] 
[00:04:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:04] 
[00:04:04] 
[00:04:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:04] Build completed unsuccessfully in 0:00:49
[00:04:04] Build completed unsuccessfully in 0:00:49
[00:04:04] Makefile:68: recipe for target 'tidy' failed
[00:04:04] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05ee4d34
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Jan 31 00:28:19 UTC 2019
---
travis_time:end:00e0f77c:start=1548894500722019038,finish=1548894500727289008,duration=5269970
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:101b966b
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0125cf82
travis_time:start:0125cf82
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0245f820
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

oli-obk · 2019-01-31T12:58:53Z

@bors r+

matthewjasper · 2019-01-31T12:58:26Z

src/librustc_mir/build/matches/simplify.rs

                };
                if let Some((min, max, sz)) = range {
                    if let (Some(lo), Some(hi)) = (lo.val.try_to_bits(sz), hi.val.try_to_bits(sz)) {
+                        // We want to compare ranges numerically, but the order of signed floating-


Signed integers

bors · 2019-01-31T12:58:54Z

📌 Commit 441355d7c2fe99b1e666d3c30ee9b6c7996547bf has been approved by oli-obk

oli-obk · 2019-01-31T12:59:26Z

@bors r-

r=me with the comment nit addressed

pnkfelix · 2019-02-01T13:22:34Z

@varkor your update to the comment (890198445cde6603ea58c67a3177d38c36da4ede) continues to refer to floating-point numbers rather than signed integers (which is what I had interpreted @matthewjasper 's comment nit as referring to).

So now I am inferring that you did in fact mean to reference floating-point numbers in your comment.

I haven't attempted to validate the reasoning in the comment/code when applied to floating-point numbers.

(I would be somewhat surprised if such a simple expression actually did suffice for comparing the binary representations of arbitrary floating-point values, but it has been a while since I've been in the depths of software FP implementation...)

But, both your own code here and the referenced code in pattern/_match.rs, is only going to look at ranges constructed from one of the types {char, iN, uN} ... nothing with floating-point type.

... so ... is there some way that floating point values can arise in these code paths? Even the underlying representation in the MIR interpreter, as far as I can tell, avoids using floating-point values...

pnkfelix · 2019-02-01T13:23:46Z

(If in fact floating-point values can arise ... I guess I'd expect to see a unit test illustrating that scenario...)

varkor · 2019-02-01T14:38:17Z

Sorry, I must have been distracted when editing the comment; it should not refer to floating-point numbers at all. I think I was conflating it with another problem I've been thinking about recently. Floating-point range patterns are not permitted in this location and do not need to be handled. I shall update the comment (and squash all of the edits) once I get back to my computer. Sorry for the confusion.

varkor · 2019-02-01T20:02:48Z

@bors r=oli-obk

bors · 2019-02-01T20:02:49Z

📌 Commit cd1047e has been approved by oli-obk

…oli-obk Fix bug in integer range matching Fixes #57894.

bors · 2019-02-01T20:57:44Z

⌛ Testing commit cd1047e with merge 852701a...

bors · 2019-02-01T23:43:28Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 852701a to master...

@ghost

[beta] Rollup backports Cherry-picked: * #58008: Pass correct arguments to places_conflict * #58007: Don't panic when accessing enum variant ctor using `Self` in match * #57978: Fix bug in integer range matching * #57862: Build the standard library for thumbv7neon-unknown-linux-gnueabihf in CI * #57659: Fix release manifest generation r? @ghost

rust-highfive assigned michaelwoerister Jan 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2019

rust-highfive assigned oli-obk and unassigned michaelwoerister Jan 29, 2019

Centril reviewed Jan 29, 2019

View reviewed changes

src/librustc_mir/build/matches/simplify.rs Show resolved Hide resolved

oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 30, 2019

oli-obk reviewed Jan 30, 2019

View reviewed changes

matthewjasper reviewed Jan 31, 2019

View reviewed changes

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 Jan 31, 2019

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 Jan 31, 2019

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 31, 2019

Fix bug in integer range matching

cd1047e

varkor force-pushed the fix-irrefutable-integer-range-match branch from 8901984 to cd1047e Compare February 1, 2019 20:02

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 Feb 1, 2019

bors added a commit that referenced this pull request Feb 1, 2019

Auto merge of #57978 - varkor:fix-irrefutable-integer-range-match, r=…

852701a

…oli-obk Fix bug in integer range matching Fixes #57894.

bors merged commit cd1047e into rust-lang:master Feb 1, 2019

varkor deleted the fix-irrefutable-integer-range-match branch February 2, 2019 00:02

Centril mentioned this pull request Feb 2, 2019

Beta miscompiles match when a pattern has a range including signed minimum #58067

Closed

pietroalbini mentioned this pull request Feb 4, 2019

[beta] Rollup backports #58159

Merged

pietroalbini removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Feb 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in integer range matching #57978

Fix bug in integer range matching #57978

varkor commented Jan 29, 2019 •

edited by pnkfelix

Loading

rust-highfive commented Jan 29, 2019

varkor commented Jan 29, 2019

oli-obk Jan 30, 2019

varkor Jan 30, 2019

rust-highfive commented Jan 31, 2019

oli-obk commented Jan 31, 2019

matthewjasper Jan 31, 2019

bors commented Jan 31, 2019

oli-obk commented Jan 31, 2019

pnkfelix commented Feb 1, 2019 •

edited

Loading

pnkfelix commented Feb 1, 2019

varkor commented Feb 1, 2019

varkor commented Feb 1, 2019

bors commented Feb 1, 2019

bors commented Feb 1, 2019

bors commented Feb 1, 2019

Fix bug in integer range matching #57978

Fix bug in integer range matching #57978

Conversation

varkor commented Jan 29, 2019 • edited by pnkfelix Loading

rust-highfive commented Jan 29, 2019

varkor commented Jan 29, 2019

oli-obk Jan 30, 2019

Choose a reason for hiding this comment

varkor Jan 30, 2019

Choose a reason for hiding this comment

rust-highfive commented Jan 31, 2019

oli-obk commented Jan 31, 2019

matthewjasper Jan 31, 2019

Choose a reason for hiding this comment

bors commented Jan 31, 2019

oli-obk commented Jan 31, 2019

pnkfelix commented Feb 1, 2019 • edited Loading

pnkfelix commented Feb 1, 2019

varkor commented Feb 1, 2019

varkor commented Feb 1, 2019

bors commented Feb 1, 2019

bors commented Feb 1, 2019

bors commented Feb 1, 2019

varkor commented Jan 29, 2019 •

edited by pnkfelix

Loading

pnkfelix commented Feb 1, 2019 •

edited

Loading