Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempted fix to issue #63551 #63906

Closed
wants to merge 3 commits into from

Conversation

TheOregonTrail
Copy link
Contributor

Well this is my first PR so hopefully I didn't do too bad.

As said I'm attempting to fix issue #63551 and remove the unnecessary check for is_unit.

would recommend removing let is_unit = input_ty.is_unit() unless there is another use.

@rust-highfive
Copy link
Collaborator

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (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.
2019-08-26T02:14:21.2411493Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-26T02:14:21.2610545Z ##[command]git config gc.auto 0
2019-08-26T02:14:21.2684906Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-26T02:14:21.2738554Z ##[command]git config --get-all http.proxy
2019-08-26T02:14:21.2872112Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63906/merge:refs/remotes/pull/63906/merge
---
2019-08-26T02:14:56.0302018Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-26T02:14:56.0302053Z 
2019-08-26T02:14:56.0302329Z   git checkout -b <new-branch-name>
2019-08-26T02:14:56.0302648Z 
2019-08-26T02:14:56.0302723Z HEAD is now at 608fcfb35 Merge 8bb577ff9e907400314029a87ebf49072ca1338f into 521d78407471cb78e9bbf47160f6aa23047ac499
2019-08-26T02:14:56.0489012Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-26T02:14:56.0492274Z ==============================================================================
2019-08-26T02:14:56.0492650Z Task         : Bash
2019-08-26T02:14:56.0492741Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-26T02:21:34.0035329Z    Compiling serde_json v1.0.40
2019-08-26T02:21:35.8555794Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-26T02:21:46.7754332Z     Finished release [optimized] target(s) in 1m 30s
2019-08-26T02:21:46.7836354Z tidy check
2019-08-26T02:21:46.9976993Z tidy error: /checkout/src/librustc_codegen_ssa/mir/rvalue.rs:589: trailing whitespace
2019-08-26T02:21:48.9713099Z some tidy checks failed
2019-08-26T02:21:48.9714239Z 
2019-08-26T02:21:48.9714239Z 
2019-08-26T02:21:48.9715247Z 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"
2019-08-26T02:21:48.9715422Z 
2019-08-26T02:21:48.9715449Z 
2019-08-26T02:21:48.9715782Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-26T02:21:48.9715849Z Build completed unsuccessfully in 0:01:33
2019-08-26T02:21:48.9715849Z Build completed unsuccessfully in 0:01:33
2019-08-26T02:21:48.9759982Z == clock drift check ==
2019-08-26T02:21:48.9775282Z   local time: Mon Aug 26 02:21:48 UTC 2019
2019-08-26T02:21:49.1331822Z   network time: Mon, 26 Aug 2019 02:21:49 GMT
2019-08-26T02:21:49.1335991Z == end clock drift check ==
2019-08-26T02:21:50.4824763Z ##[error]Bash exited with code '1'.
2019-08-26T02:21:50.4858164Z ##[section]Starting: Checkout
2019-08-26T02:21:50.4860250Z ==============================================================================
2019-08-26T02:21:50.4860314Z Task         : Get sources
2019-08-26T02:21:50.4860368Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (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.
2019-08-26T04:11:15.3229162Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-26T04:11:16.2175925Z ##[command]git config gc.auto 0
2019-08-26T04:11:16.2180610Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-26T04:11:16.2183097Z ##[command]git config --get-all http.proxy
2019-08-26T04:11:16.2187942Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63906/merge:refs/remotes/pull/63906/merge
---
2019-08-26T04:11:52.2661544Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-26T04:11:52.2661567Z 
2019-08-26T04:11:52.2661749Z   git checkout -b <new-branch-name>
2019-08-26T04:11:52.2662803Z 
2019-08-26T04:11:52.2663588Z HEAD is now at 019ba58cc Merge 4ab65c6a993e82a85c8ce50aa52a7215e53a5b41 into 4c58535d09d1261d21569df0036b974811544256
2019-08-26T04:11:52.2816432Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-26T04:11:52.2819474Z ==============================================================================
2019-08-26T04:11:52.2819696Z Task         : Bash
2019-08-26T04:11:52.2819865Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-26T04:42:46.3754255Z    Compiling semver v0.9.0
2019-08-26T04:42:47.8045566Z error: failed to run custom build command for `libc v0.2.61`
2019-08-26T04:42:47.8045677Z 
2019-08-26T04:42:47.8045727Z Caused by:
2019-08-26T04:42:47.8046283Z   process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/build/libc-b24c65ec356e6fe9/build-script-build` (signal: 11, SIGSEGV: invalid memory reference)
2019-08-26T04:42:48.1971967Z error: build failed
2019-08-26T04:42:48.2007703Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2019-08-26T04:42:48.2008046Z expected success, got: exit code: 101
2019-08-26T04:42:48.2017509Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-26T04:42:48.2017509Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-26T04:42:48.2017586Z Build completed unsuccessfully in 0:24:38
2019-08-26T04:42:48.2070109Z == clock drift check ==
2019-08-26T04:42:48.2085072Z   local time: Mon Aug 26 04:42:48 UTC 2019
2019-08-26T04:42:48.3578557Z   network time: Mon, 26 Aug 2019 04:42:48 GMT
2019-08-26T04:42:48.3581470Z == end clock drift check ==
2019-08-26T04:42:49.5777899Z ##[error]Bash exited with code '1'.
2019-08-26T04:42:49.5842012Z ##[section]Starting: Checkout
2019-08-26T04:42:49.5844336Z ==============================================================================
2019-08-26T04:42:49.5844408Z Task         : Get sources
2019-08-26T04:42:49.5844456Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@bjorn3
Copy link
Member

bjorn3 commented Aug 26, 2019

Could you please remove the rustfmt commit? While it is ok to format the lines you are working with, fornatting the rest gives too much noise.

@petrochenkov
Copy link
Contributor

r? @bjorn3
cc @eddyb

@rust-highfive rust-highfive assigned bjorn3 and unassigned petrochenkov Aug 26, 2019
@petrochenkov
Copy link
Contributor

@bors delegate=bjorn3
(Not sure all assignable people have r+ rights.)

@bors
Copy link
Contributor

bors commented Aug 26, 2019

✌️ @bjorn3 can now approve this pull request

@bjorn3
Copy link
Member

bjorn3 commented Aug 26, 2019

@petrochenkov I am not part of the rust team.

r? @eddyb (does that work?)

@rust-highfive rust-highfive assigned eddyb and unassigned bjorn3 Aug 26, 2019
@petrochenkov
Copy link
Contributor

@bjorn3

I am not part of the rust team.

(That's not too important, I know that you worked on rustc_codegen_ssa and familiar with the code, so you can review.)

@bjorn3
Copy link
Member

bjorn3 commented Aug 26, 2019

(That's not too important, I know that you worked on rustc_codegen_ssa and familiar with the code, so you can review.)

Didn't know that. Is that written down somewhere as policy?

@Kixiron Kixiron added A-codegen Area: Code generation 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2019
@Kixiron
Copy link
Member

Kixiron commented Aug 26, 2019

@luigisHat Tests are not currently passing

@TheOregonTrail
Copy link
Contributor Author

@Kixiron and @bjorn3 Take another look please I think I did what you wanted.

@@ -546,7 +546,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
) -> Bx::Value {
let is_float = input_ty.is_floating_point();
let is_signed = input_ty.is_signed();
let is_unit = input_ty.is_unit();
// No need for this anymore ISSUE:#63551
let _is_unit = input_ty.is_unit();
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line and the one above.

})
} else if is_float {
mir::BinOp::Eq | mir::BinOp::Le | mir::BinOp::Ge => {
if is_float {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the { on the line above and merge this line into the one above.

@@ -600,10 +596,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
base::bin_op_to_icmp_predicate(op.to_hir_binop(), is_signed),
lhs, rhs
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line.

}
}
}


Copy link
Member

Choose a reason for hiding this comment

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

Please remove this newline.

@bjorn3
Copy link
Member

bjorn3 commented Aug 28, 2019

Please squash your comments after fixing the review comments above. See for example http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html for how to squash.

@TheOregonTrail
Copy link
Contributor Author

Sorry Having trouble with git rebase I have a merge conflict with rvalue.rs and It's giving me a headache.

@TheOregonTrail
Copy link
Contributor Author

@bjorn3 I tried to git squash it but I think I messed it up. When I finish with git rebase I keep getting thrown back a commit and am required to pull and henceforth get stuck pushing two commits at once ("Added two commits six days ago"). Help would be appreciated

@bjorn3
Copy link
Member

bjorn3 commented Sep 1, 2019

Maybe reset your branch back to master and perform you changes again.

$ git fetch origin # get latest version of rust
$ git reset --hard origin/master # reset current branch back to latest version of rust (throws all changes away)
$ # perform changes and commit
$ git push --force # push your changes to your fork

Hope that helps. Over time you will probably get better at using git, but at first it can be hard.

@JohnCSimon
Copy link
Member

Ping from triage:
@luigisHat can you please address the comments here? thanks!

@TheOregonTrail
Copy link
Contributor Author

@JohnCSimon @bjorn3 Sorry about the late action I've had problems with my computer and just got my self repairs done today. Thanks for being patient 👍

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@bjorn3
Copy link
Member

bjorn3 commented Sep 18, 2019

@luigisHat It seems some tests changed their error order. Please bless them. (./x.py test --bless)

@bjorn3
Copy link
Member

bjorn3 commented Sep 18, 2019

I should have checked the PR ci run before r+ing this.

@TheOregonTrail
Copy link
Contributor Author

@bjorn3 Do you know why it says Alex Crichton canceled the build ?

@bjorn3
Copy link
Member

bjorn3 commented Sep 20, 2019

No, where are you seeing that?

@mati865
Copy link
Contributor

mati865 commented Sep 20, 2019

Do you know why it says Alex Crichton canceled the build ?

When one of the builds fails bot cancels other jobs to save time by letting other jobs run.
I think Alex configured this using his personal account and that's why his name appears. It doesn't matter since this is legitimate failure.

@oli-obk oli-obk 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2019
@oli-obk oli-obk assigned bjorn3 and unassigned eddyb Sep 21, 2019
@TheOregonTrail
Copy link
Contributor Author

@bjorn3 x.py test --bless leaves me with 82 errors concerning for the test suite [debuginfo-gdb+lldb]. I'm also getting "error: gdb failed to execute". What is gdb's role in the compiler ?

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2019

gdb is used in tests testing debuginfo. It should be fine to ignore those tests.

@TheOregonTrail
Copy link
Contributor Author

TheOregonTrail commented Sep 27, 2019

@bjorn3 what should I look for in failed-doctest-output.rs ? Or in general ?

@bjorn3
Copy link
Member

bjorn3 commented Sep 27, 2019

Running ./x.py --bless src/test/rustdoc-ui should fix all rustdoc tests, while skipping the tests requiring gdb.

@TheOregonTrail
Copy link
Contributor Author

Ok I ran ./x.py test --stage 1 src/test/rustdoc-ui --bless. The Tests all passed but I can't add anything to a new commit because nothing changed. @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented Oct 1, 2019

Maybe try rebasing this PR. If CI succeeds, this PR will be ready to go.

@JohnCSimon
Copy link
Member

Ping from triage - this hasn't been touched in 5 days.
@luigisHat @bjorn3 seems that CI has failed
Thanks.

@Centril
Copy link
Contributor

Centril commented Oct 8, 2019

@bors r-

@TheOregonTrail
Copy link
Contributor Author

@bjorn3 Could we try testing this commit with whatever we did before ?

@bjorn3
Copy link
Member

bjorn3 commented Oct 10, 2019

I think you messed up the rebase again. If I have to believe the PR CI, it should work fine now though. Do you want to try fixing the rebase? Or should I try?

@TheOregonTrail
Copy link
Contributor Author

@bjorn3 If you have the time could you please fix it and tell me what you did ? Thanks so much for your patience you really have been a big help stepping me through this.

@bjorn3
Copy link
Member

bjorn3 commented Oct 19, 2019

I decided to make a new branch with your changes:

$ git checkout master
$ git pull # Get latest changes
$ git checkout -b fix_63906 # Make new branch to apply changes

# Fetch all your commits
$ git remote add luigishat https://github.com/luigishat/rust.git
$ git fetch luigishat

# Take the commit with the actual changes and apply it to the current branch
$ git cherry-pick b139849bf1842c7cf7ea0cc78e3ddc362cb2ce02

# Change the commit message to something nicer
$ git commit --amend -m "Remove unreachable unit tuple compare binop codegen"

$ git push -u my fix_63906 # Push branch to my fork

Normally you would do something like:

$ git fetch origin # Assuming origin refers to https://github.com/rust-lang/rust
$ git rebase origin/master
$ git push --force-with-lease # Push rebased version (--force-with-lease instead of --force prevents forcing when your fork changed between the last time you pulled and the push)

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 20, 2019
Remove unreachable unit tuple compare binop codegen

Closes rust-lang#63906
Fixes rust-lang#63551

This is based on rust-lang#63906 by @luigisHat, who had trouble with rebasing his PR.
@bors bors closed this in 4f74fd7 Oct 20, 2019
@TheOregonTrail TheOregonTrail deleted the cmp_fix branch October 22, 2019 01:42
@TheOregonTrail TheOregonTrail restored the cmp_fix branch October 22, 2019 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet