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

ICH: Hash expression spans if their source location is captured for panics. #37513

Merged
merged 2 commits into from
Nov 3, 2016

Conversation

michaelwoerister
Copy link
Member

Since the location of some expressions is captured in error message constants, it has an influence on machine code and consequently we need to take them into account by the incr. comp. hash. This PR makes this happen for +, -, *, /, % and for array indexing -- let me know if I forgot anything.

In the future we might want to change the codegen strategy for those error messages, so that they are stored in a separate object file with a stable symbol name, so that only this object file has to be regenerated when source locations change. This strategy would also eliminate unnecessary duplications due to monomorphization, as @arielb1 has pointed out on IRC. I opened #37512, so we don't forget about this.

r? @nikomatsakis

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me with new test

@@ -842,4 +904,10 @@ impl<'a, 'hash, 'tcx> StrictVersionHashVisitor<'a, 'hash, 'tcx> {
indices.sort_by_key(|index| get_key(&items[*index]));
indices
}

fn maybe_enable_overflow_checks(&mut self, item_attrs: &[ast::Attribute]) {
if attr::contains_name(item_attrs, "rustc_inherit_overflow_checks") {
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, i'd forgotten about this, can we add this to the test?

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

Test case added. Thanks for the review!

@bors
Copy link
Contributor

bors commented Nov 2, 2016

📌 Commit 0e391bf has been approved by nikomatsakis

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Nov 2, 2016
…r=nikomatsakis

ICH: Hash expression spans if their source location is captured for panics.

Since the location of some expressions is captured in error message constants, it has an influence on machine code and consequently we need to take them into account by the incr. comp. hash. This PR makes this happen for `+, -, *, /, %` and for array indexing -- let me know if I forgot anything.

In the future we might want to change the codegen strategy for those error messages, so that they are stored in a separate object file with a stable symbol name, so that only this object file has to be regenerated when source locations change. This strategy would also eliminate unnecessary duplications due  to monomorphization, as @arielb1 has pointed out on IRC. I opened rust-lang#37512, so we don't forget about this.

r? @nikomatsakis
bors added a commit that referenced this pull request Nov 2, 2016
Rollup of 10 pull requests

- Successful merges: #37351, #37405, #37473, #37482, #37488, #37498, #37502, #37513, #37517, #37523
- Failed merges: #37521
@bors bors merged commit 0e391bf into rust-lang:master Nov 3, 2016
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