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

Added graphviz visualization for obligation forests. #54486

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

orium
Copy link
Member

@orium orium commented Sep 22, 2018

This can be a big help when debugging the trait resolver.

@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 @varkor (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 Sep 22, 2018
@orium
Copy link
Member Author

orium commented Sep 22, 2018

This is something I did to help me debug issue #40827 (CC #53255) and I think can be helpful in the future for other people debugging trait resolution.

/// * `sed 's,std::[a-z]*::,,g'` — Deletes the `std::<package>::` prefix of paths.
/// * `sed 's,"Binder(TraitPredicate(<\(.*\)>)) (\([^)]*\))","\1 (\2)",'` — Transforms
/// `Binder(TraitPredicate(<predicate>))` into just `<predicate>`.
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is not used. The idea is that people can temporarily call it while debugging stuff.

@varkor
Copy link
Member

varkor commented Sep 24, 2018

I've not worked with obligation_forest before, so I think it'd be more helpful for someone who has to judge whether there are any ways this could be more useful. r? @arielb1

@rust-highfive rust-highfive assigned arielb1 and unassigned varkor Sep 24, 2018
@arielb1
Copy link
Contributor

arielb1 commented Sep 24, 2018

Hi @orium - are you trying to debug something in particular?

@memoryruins
Copy link
Contributor

memoryruins commented Sep 24, 2018

@arielb1

(I'm glad I added graphviz to the obligations forest. It make things much easier to understand. I will open a PR soon with that.)

#53255 (comment)

edit: I see you already replied ^^

@arielb1
Copy link
Contributor

arielb1 commented Sep 25, 2018

This looks like an OK idea, but I don't know the graphviz API that well - or what's the best way to integrate tooling like that into the compiler - r? @nikomatsakis for that.

@bors
Copy link
Contributor

bors commented Sep 26, 2018

☔ The latest upstream changes (presumably #53824) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

I have no objection to landing this code, though I don't expect the obligation forest to live much longer.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2018

📌 Commit dde82aa3d78115533136aa58a6da7a8b0a468cbd has been approved by nikomatsakis

@bors 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 Oct 4, 2018
@nikomatsakis
Copy link
Contributor

The code itself seems fine :)

@bors
Copy link
Contributor

bors commented Oct 4, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout obligation-forest-graphviz (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self obligation-forest-graphviz --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 2791 and retry the command.
Auto-merging src/librustc_data_structures/obligation_forest/mod.rs
Auto-merging src/librustc_data_structures/lib.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors 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 Oct 4, 2018
@orium orium force-pushed the obligation-forest-graphviz branch from dde82aa to bb2c805 Compare October 5, 2018 01:04
@orium
Copy link
Member Author

orium commented Oct 5, 2018

@nikomatsakis rebased :)

@nikomatsakis
Copy link
Contributor

@bors r

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 9, 2018

📌 Commit bb2c8052cb410195b1e5b9f5b4a0522cdecd27b2 has been approved by nikomatsakis

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 9, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 9, 2018
@bors
Copy link
Contributor

bors commented Oct 9, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout obligation-forest-graphviz (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self obligation-forest-graphviz --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors 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 Oct 9, 2018
@orium orium force-pushed the obligation-forest-graphviz branch 2 times, most recently from 272ef52 to d9fc4e2 Compare October 11, 2018 09:39
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.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.
######################################################################## 100.0%
[00:02:16] extracting /checkout/obj/build/cache/2018-09-23/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:16] warning: the cargo feature `edition` is now stable and is no longer necessary to be listed in the manifest
[00:02:17]     Updating crates.io index
[00:02:21] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:21] Build completed unsuccessfully in 0:00:34
[00:02:21] make: *** [prepare] Error 1
[00:02:21] Makefile:81: recipe for target 'prepare' failed
[00:02:22] Command failed. Attempt 2/5:
[00:02:22] Command failed. Attempt 2/5:
[00:02:22] warning: the cargo feature `edition` is now stable and is no longer necessary to be listed in the manifest
[00:02:22] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:22] Build completed unsuccessfully in 0:00:00
[00:02:22] make: *** [prepare] Error 1
[00:02:22] Makefile:81: recipe for target 'prepare' failed
[00:02:24] Command failed. Attempt 3/5:
[00:02:24] Command failed. Attempt 3/5:
[00:02:24] warning: the cargo feature `edition` is now stable and is no longer necessary to be listed in the manifest
[00:02:24] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:24] Build completed unsuccessfully in 0:00:00
[00:02:24] make: *** [prepare] Error 1
[00:02:24] Makefile:81: recipe for target 'prepare' failed
[00:02:27] Command failed. Attempt 4/5:
[00:02:27] Command failed. Attempt 4/5:
[00:02:28] warning: the cargo feature `edition` is now stable and is no longer necessary to be listed in the manifest
[00:02:28] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:28] Build completed unsuccessfully in 0:00:00
[00:02:28] make: *** [prepare] Error 1
[00:02:28] Makefile:81: recipe for target 'prepare' failed
[00:02:32] Command failed. Attempt 5/5:
[00:02:32] Command failed. Attempt 5/5:
[00:02:32] warning: the cargo feature `edition` is now stable and is no longer necessary to be listed in the manifest
[00:02:32] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:32] Build completed unsuccessfully in 0:00:00
[00:02:32] make: *** [prepare] Error 1
[00:02:32] Makefile:81: recipe for target 'prepare' failed
[00:02:32] The command has failed after 5 attempts.
---
travis_time:end:076218ba:start=1539251045610849976,finish=1539251045615404233,duration=4554257
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1209290c
$ 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:0baf9bf4
travis_time:start:0baf9bf4
$ 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:113ab0d6
$ 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)

@orium orium force-pushed the obligation-forest-graphviz branch 2 times, most recently from 1923275 to 34fbef7 Compare October 13, 2018 14:44
@orium
Copy link
Member Author

orium commented Oct 13, 2018

@nikomatsakis Should be mergeable now. Btw, is there any way to avoid or reduce merge conflicts because of Cargo.lock?

This can be a big help when debugging the trait resolver.
@nikomatsakis
Copy link
Contributor

@bors r+

@orium not that I know of

@bors
Copy link
Contributor

bors commented Oct 16, 2018

📌 Commit 3fc275d has been approved by nikomatsakis

@bors 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 Oct 16, 2018
@bors
Copy link
Contributor

bors commented Oct 16, 2018

⌛ Testing commit 3fc275d with merge bef62cc...

bors added a commit that referenced this pull request Oct 16, 2018
Added graphviz visualization for obligation forests.

This can be a big help when debugging the trait resolver.
@bors
Copy link
Contributor

bors commented Oct 16, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing bef62cc to master...

@bors bors merged commit 3fc275d into rust-lang:master Oct 16, 2018
@orium orium deleted the obligation-forest-graphviz branch October 16, 2018 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants