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

[WIP] middle/check_const: CheckItemRecursion should only recurse into NodeItems #14262

Closed
wants to merge 5 commits into from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented May 17, 2014

Note: This used to be #13950. Unfortunately due to poor branch management practices on my part, I had to close it in favor of this issue.

This fixes a bug in libsyntax which would crash on ASTs containing references to foreign externs. Previously the included test would fail with,

rustc' failed at 'expected item, found foreign item test_extern::test_extern (id=13)', /opt/exp/rust/src/libsyntax/ast_map.rs:264

@bgamari
Copy link
Contributor Author

bgamari commented May 17, 2014

@alexcrichton, would you happen to have the text from my comment on #13950 describing the state of my debugging sitting in your email inbox? If so, you could paste it here. Somehow Github dropped it and unfortunately this was my only copy.

@huonw
Copy link
Member

huonw commented May 17, 2014

Does this do anything to address a case like the following?

extern { static X: uint; }

static Y: uint = X;

(i.e. trying to refer to the actual value of the extern constant, not the address.)

@huonw
Copy link
Member

huonw commented May 17, 2014

Also, it seems like it may close #13325 (depending on the answer to my comment at the end)?

@alexcrichton
Copy link
Member

This is the last message I have in my inbox about the old PR, the one you were looking for?


@alexcrichton, thanks!

I've been working on this but I've been on the road so there have been few opportunities to post updates. It seems this commit is only a partial fix. While the code that lead me to find this issue now compiles, the test case included in this set still crashes, as the CI run demonstrates.

The problem appears to be a lack of handling of mutable statics in middle::trans::const_expr_unadjusted. In the case of the included test case the following happens,

  1. get_item_val is called on static test::test which calls
  2. const_expr on &test_extern which calls const_expr_unadjusted which calls
  3. const_expr on test_extern which calls const_expr_unadjusted which calls
  4. get_const_value which then fails as apparently this should never happen.

Going to be on and off of flights for the next 12 hours or so but I'll be online intermittently.

@alexcrichton
Copy link
Member

@bgamari
Copy link
Contributor Author

bgamari commented May 18, 2014

@alexcrichton, indeed that's the one. Thanks!

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@bgamari
Copy link
Contributor Author

bgamari commented Jun 4, 2014

I just pushed a rebase and will have a stab at finishing this up now.

@huonw huonw reopened this Jun 4, 2014
@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@bgamari
Copy link
Contributor Author

bgamari commented Oct 19, 2014

I seem to be running into this again. I'll try dusting off the patch again.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 19, 2014

Hmm, never mind; I'm actually see #18164 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants