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

Add expansion info to crate metadata #43847

Closed
wants to merge 13 commits into from

Conversation

ibabushkin
Copy link
Contributor

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

@@ -129,13 +138,98 @@ impl HygieneData {
}
HYGIENE_DATA.with(|data| f(&mut *data.borrow_mut()))
}

pub fn safe_with<T, F: FnOnce(&HygieneData) -> T>(f: F) -> T {
// FIXME(twk): not sure how this would behave...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@ibabushkin ibabushkin closed this Aug 13, 2017
@ibabushkin ibabushkin reopened this Aug 13, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2017
@ibabushkin
Copy link
Contributor Author

ibabushkin commented Aug 14, 2017

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).

@bors
Copy link
Contributor

bors commented Aug 16, 2017

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

@ibabushkin
Copy link
Contributor Author

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.rs

I'm starting my search from there.

@ibabushkin ibabushkin force-pushed the cross-crate-expand branch 3 times, most recently from d6e9dee to 3a5c9f1 Compare August 19, 2017 17:10
@ibabushkin
Copy link
Contributor Author

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
Copy link
Contributor

Started reviewing, looks good so far. I think we should be able to test using nested macro_rules! (at least to observe that this PR behaves differently from master) and/or nested macros (with #![feature(decl_macro)]).

@ibabushkin
Copy link
Contributor Author

Alright, I'll fiddle something like that together.

@alexcrichton alexcrichton 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 Aug 24, 2017
@ibabushkin
Copy link
Contributor Author

Quick update: in the meantime, some recent changes are breaking the functionality again - I'll try to find out what happens.

@bors
Copy link
Contributor

bors commented Aug 30, 2017

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

@arielb1
Copy link
Contributor

arielb1 commented Sep 5, 2017

What's the status of this PR? Are you still interested in it @ibabushkin?

@ibabushkin
Copy link
Contributor Author

ibabushkin commented Sep 5, 2017 via email

@aidanhs
Copy link
Member

aidanhs commented Sep 13, 2017

Hi @ibabushkin, just a friendly ping to make sure this PR doesn't get lost! :)

@arielb1
Copy link
Contributor

arielb1 commented Sep 19, 2017

What's the status of this PR? Are you still interested in it @ibabushkin?

@ibabushkin
Copy link
Contributor Author

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
Copy link
Member

Would you prefer me to close the PR in the meantime and reopen it when it is ready?

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
Copy link
Member

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
Copy link
Contributor Author

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.
#[macro_use]
extern crate macro_spans_lib;

abc!(Name); //~ ERROR recursive type `Name::Name` has infinite size
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

ibabushkin commented Oct 14, 2017 via email

@ibabushkin
Copy link
Contributor Author

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
Copy link
Member

ping @ibabushkin, was curious how the progress is here!

@ibabushkin
Copy link
Contributor Author

@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 #rust-internals about approaches to a more comfortable debugging/logging setup, but so far I wasn't really successful.

@carols10cents
Copy link
Member

@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 #rust-internals about approaches to a more comfortable debugging/logging setup, but so far I wasn't really successful.

@jseyfried do you have any ideas on how @ibabushkin could debug the test failures here?

@bors
Copy link
Contributor

bors commented Nov 1, 2017

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

@carols10cents
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants