Uh oh!
There was an error while loading. Please reload this page.
Add expansion info to crate metadata - #43847
Conversation
There was a problem hiding this comment.
I couldn't find any info on how multiple declarations of thread_local!s behave, so this might need to be moved for the expected behaviour.
There was a problem hiding this comment.
Okay, a lot of symptoms point to having two thread locals being the issue. However, replacing them with one leads to absolutely uncontrolled memory consumption.
So currently, the on-demand loading mechanism for hygiene data is still incorrect (though I have a local fix for that). Moreover, some tests still fail. My current theory is that encoding the hygiene data of a crate doesn't force all of it's dependencies' hygiene data to be decoded first. One could fix this either by forcing a complete decoding procedure for each dependency upon encoding of by simply making decoding of that particular metadata strict (which I'd like to avoid). |
1c29647 to
8ddc088Comparebors
commented
Aug 16, 2017
☔ The latest upstream changes (presumably #43710) made this pull request unmergeable. Please resolve the merge conflicts. |
ibabushkin
commented
Aug 16, 2017
For reference, to reproduce the crash on the backtrace tests in a more comfortable environment: RUST_LOG=debug ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc -g --crate-type=lib src/test/run-pass/backtrace.rsI'm starting my search from there. |
d6e9dee to
3a5c9f1Compareibabushkin
commented
Aug 19, 2017
Okay, the implementation now doesn't cause any crashes and is okay resource-usage-wise. What remains to do now is to remove leftover cruft, and to clean up functionality in other places. For instance, cross-crate macro backtraces would probably benefit from these changes, but the integration remains to be implemented. Finally, some tests might be in order. Given all these points, I think now is a good time to review what we've got so far. |
jseyfried
commented
Aug 21, 2017
Started reviewing, looks good so far. I think we should be able to test using nested |
ibabushkin
commented
Aug 21, 2017
Alright, I'll fiddle something like that together. |
ibabushkin
commented
Aug 25, 2017
Quick update: in the meantime, some recent changes are breaking the functionality again - I'll try to find out what happens. |
bors
commented
Aug 30, 2017
☔ The latest upstream changes (presumably #43968) made this pull request unmergeable. Please resolve the merge conflicts. |
arielb1
commented
Sep 5, 2017
What's the status of this PR? Are you still interested in it @ibabushkin? |
ibabushkin
commented
Sep 5, 2017
via email
On 2017-09-05, Ariel Ben-Yehuda wrote:
What's the status of this PR? Are you still interested in it @ibabushkin?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#43847 (comment) Yes, absolutely. I'm sorry for the stall - some bugs (re?)appeared and
some other things got in the way :/ |
aidanhs
commented
Sep 13, 2017
Hi @ibabushkin, just a friendly ping to make sure this PR doesn't get lost! :) |
arielb1
commented
Sep 19, 2017
What's the status of this PR? Are you still interested in it @ibabushkin? |
ibabushkin
commented
Sep 19, 2017
Yes, absolutely. I'm sorry for the delays piling up - I fear I'll only be able to get back to this early next month :/. Would you prefer me to close the PR in the meantime and reopen it when it is ready? |
carols10cents
commented
Sep 25, 2017
That's up to you @ibabushkin! We're happy to hold off on the reminder pings until October if you want to leave this open :) |
aidanhs
commented
Oct 5, 2017
Hi @ibabushkin, you mentioned early October, so just a friendly ping to make sure you're still aware of this PR! |
ibabushkin
commented
Oct 7, 2017
I'm taking up the work again! :) Thank you all for your patience. |
This change adds an encoded version of hygiene related datastructures to the metadata of a crate. It is then decoded on-demand when spans and AST items from a foreign crate are accessed. Some changes might still be necessary to implement the correct behaviour, however.
Sadly, this isn't the reason the tests fail.
However, now that it behaves as expected, the memory usage is unacceptably high. A solution would be more lazyness during encoding or a pruning scheme.
05a6684 to
909808fCompare| #[macro_use] | ||
| extern crate macro_spans_lib; | ||
| abc!(Name); //~ ERROR recursive type `Name::Name` has infinite size |
There was a problem hiding this comment.
This test failed.
[00:56:33] ---- [compile-fail] compile-fail-fulldeps/macro-spans.rs stdout ----
[00:56:33] [00:56:33] error: /checkout/src/test/compile-fail-fulldeps/macro-spans.rs:16: expected error not found: recursive type `Name::Name` has infinite size
[00:56:33] [00:56:33] error: 0 unexpected errors found, 1 expected errors not found
[00:56:33] status: exit code: 101
[00:56:33] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail-fulldeps/macro-spans.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/macro-spans.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/macro-spans.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux" "-A" "unused"
[00:56:33] not found errors (from test file): [
[00:56:33] Error {
[00:56:33] line_num: 16,
[00:56:33] kind: Some(
[00:56:33] Error
[00:56:33] ),
[00:56:33] msg: "recursive type `Name::Name` has infinite size"
[00:56:33] }
[00:56:33] ]
[00:56:33] [00:56:33] thread '[compile-fail] compile-fail-fulldeps/macro-spans.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:1084:12
[00:56:33] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:56:33] [00:56:33] [00:56:33] failures:
[00:56:33] [compile-fail] compile-fail-fulldeps/macro-spans.rs
[00:56:33] [00:56:33] test result: FAILED. 52 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
ibabushkin
commented
Oct 14, 2017
via email
I am aware :) - fixing it is not quite trivial, working on it. …On 2017-10-14, kennytm wrote:
kennytm commented on this pull request.
> +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:macro_spans_lib.rs
+
+#[macro_use]
+extern crate macro_spans_lib;
+
+abc!(Name); //~ ERROR recursive type `Name::Name` has infinite size
This test failed.
```
[00:56:33] ---- [compile-fail] compile-fail-fulldeps/macro-spans.rs stdout ----
[00:56:33]
[00:56:33] error: /checkout/src/test/compile-fail-fulldeps/macro-spans.rs:16: expected error not found: recursive type `Name::Name` has infinite size
[00:56:33]
[00:56:33] error: 0 unexpected errors found, 1 expected errors not found
[00:56:33] status: exit code: 101
[00:56:33] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail-fulldeps/macro-spans.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/macro-spans.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/macro-spans.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux" "-A" "unused"
[00:56:33] not found errors (from test file): [
[00:56:33] Error {
[00:56:33] line_num: 16,
[00:56:33] kind: Some(
[00:56:33] Error
[00:56:33] ),
[00:56:33] msg: "recursive type `Name::Name` has infinite size"
[00:56:33] }
[00:56:33] ]
[00:56:33]
[00:56:33] thread '[compile-fail] compile-fail-fulldeps/macro-spans.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:1084:12
[00:56:33] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:56:33]
[00:56:33]
[00:56:33] failures:
[00:56:33] [compile-fail] compile-fail-fulldeps/macro-spans.rs
[00:56:33]
[00:56:33] test result: FAILED. 52 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
```
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#43847 (review) |
ibabushkin
commented
Oct 15, 2017
It seems that the error display logic broke in the meantime, which is rather odd, but hints at more serious problems. I am currently considering starting this feature from scratch. |
alexcrichton
commented
Oct 19, 2017
ping @ibabushkin, was curious how the progress is here! |
ibabushkin
commented
Oct 19, 2017
@alexcrichton To be honest, I am currently hung up debugging the test failures - and I suspect more systematic problems with this changeset. I asked in |
carols10cents
commented
Oct 30, 2017
@jseyfried do you have any ideas on how @ibabushkin could debug the test failures here? |
bors
commented
Nov 1, 2017
☔ The latest upstream changes (presumably #45674) made this pull request unmergeable. Please resolve the merge conflicts. |
carols10cents
commented
Nov 13, 2017
Hi @ibabushkin, thank you for this PR! Due to inactivity, merge conflicts, and the possibility you raised that there might be systematic problems with this changeset, I'm going to close this. Please do continue to reach out when you have time to work on this to get help (try pinging @jseyfried specifically on IRC?) and either reopen this PR or start a new PR when you're ready! |
This implements the serialization and deserialization of hygiene info in crate metadata, preserving macro expansion information across crates, addressing some of the points mentioned in #40847. There is some work left to do, however: Apparently there is code that hides macro backtraces from external crates and some of the decoding logic might need to be expanded or moved to accomodate for all current use cases I might have missed.
r? @jseyfried