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

Don't visit foreign function bodies when lowering ast to hir #74204

Merged
merged 5 commits into from
Aug 16, 2020

Conversation

ayazhafiz
Copy link
Contributor

Previously the existence of bodies inside a foreign function block would
cause a panic in the hir NodeCollector during its collection of crate
bodies to compute a crate hash:

// Insert bodies into the map
for (id, body) in self.krate.bodies.iter() {
let bodies = &mut self.map[id.hir_id.owner].with_bodies.as_mut().unwrap().bodies;
assert!(bodies.insert(id.hir_id.local_id, body).is_none());
}

The collector walks the hir tree and creates a map of hir nodes, then
attaching bodies in the crate to their owner in the map. For a code like

extern "C" {
    fn f() {
        fn g() {}
    }
}

The crate bodies include the body of the function g. But foreign
functions cannot have bodies, and while the parser AST permits a foreign
function to have a body, the hir doesn't. This means that the body of
f is not present in the hir, and so neither is g. So when the
NodeCollector finishes the walking the hir, it has no record of g,
cannot find an owner for the body of g it sees in the crate bodies,
and blows up.

Why do the crate bodies include the body of g? The AST walker has a
need a for walking function bodies, and FFIs share the same AST node as
functions in other contexts.

There are at least two options to fix this:

  • Don't unwrap the map entry for an hir node in the NodeCollector
  • Modifier the ast->hir lowering visitor to ignore foreign function
    blocks

I don't think the first is preferrable, since we want to know when we
can't find a body for an hir node that we thought had one (dropping this
information may lead to an invalid hash). So this commit implements the
second option.

Closes #74120

Previously the existence of bodies inside a foreign function block would
cause a panic in the hir `NodeCollector` during its collection of crate
bodies to compute a crate hash:

https://github.com/rust-lang/rust/blob/e59b08e62ea691916d2f063cac5aab4634128022/src/librustc_middle/hir/map/collector.rs#L154-L158

The collector walks the hir tree and creates a map of hir nodes, then
attaching bodies in the crate to their owner in the map. For a code like

```rust
extern "C" {
    fn f() {
        fn g() {}
    }
}
```

The crate bodies include the body of the function `g`. But foreign
functions cannot have bodies, and while the parser AST permits a foreign
function to have a body, the hir doesn't. This means that the body of
`f` is not present in the hir, and so neither is `g`. So when the
`NodeCollector` finishes the walking the hir, it has no record of `g`,
cannot find an owner for the body of `g` it sees in the crate bodies,
and blows up.

Why do the crate bodies include the body of `g`? The AST walker has a
need a for walking function bodies, and FFIs share the same AST node as
functions in other contexts.

There are at least two options to fix this:

- Don't unwrap the map entry for an hir node in the `NodeCollector`
- Modifier the ast->hir lowering visitor to ignore foreign function
  blocks

I don't think the first is preferrable, since we want to know when we
can't find a body for an hir node that we thought had one (dropping this
information may lead to an invalid hash). So this commit implements the
second option.

Closes rust-lang#74120
@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2020
@eddyb eddyb 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 Jul 15, 2020
@ayazhafiz ayazhafiz requested a review from eddyb July 15, 2020 01:14
@ayazhafiz ayazhafiz requested a review from eddyb July 16, 2020 00:22
@JohnTitor JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2020
@ayazhafiz
Copy link
Contributor Author

@eddyb friendly ping for this :)

@eddyb
Copy link
Member

eddyb commented Aug 16, 2020

Ugh I should've mentioned that you should PM me on Zulip once you've addressed the comments.

@eddyb
Copy link
Member

eddyb commented Aug 16, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2020

📌 Commit d442bf7 has been approved by eddyb

@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 Aug 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 16, 2020
Don't visit foreign function bodies when lowering ast to hir

Previously the existence of bodies inside a foreign function block would
cause a panic in the hir `NodeCollector` during its collection of crate
bodies to compute a crate hash:

https://github.com/rust-lang/rust/blob/e59b08e62ea691916d2f063cac5aab4634128022/src/librustc_middle/hir/map/collector.rs#L154-L158

The collector walks the hir tree and creates a map of hir nodes, then
attaching bodies in the crate to their owner in the map. For a code like

```rust
extern "C" {
    fn f() {
        fn g() {}
    }
}
```

The crate bodies include the body of the function `g`. But foreign
functions cannot have bodies, and while the parser AST permits a foreign
function to have a body, the hir doesn't. This means that the body of
`f` is not present in the hir, and so neither is `g`. So when the
`NodeCollector` finishes the walking the hir, it has no record of `g`,
cannot find an owner for the body of `g` it sees in the crate bodies,
and blows up.

Why do the crate bodies include the body of `g`? The AST walker has a
need a for walking function bodies, and FFIs share the same AST node as
functions in other contexts.

There are at least two options to fix this:

- Don't unwrap the map entry for an hir node in the `NodeCollector`
- Modifier the ast->hir lowering visitor to ignore foreign function
  blocks

I don't think the first is preferrable, since we want to know when we
can't find a body for an hir node that we thought had one (dropping this
information may lead to an invalid hash). So this commit implements the
second option.

Closes rust-lang#74120
@bors
Copy link
Contributor

bors commented Aug 16, 2020

⌛ Testing commit d442bf7 with merge 57c460f246822f269a5c8c3df7f9787c37e3dd13...

@tmandry
Copy link
Member

tmandry commented Aug 16, 2020

Rolled up,
@bors retry

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#74204 (Don't visit foreign function bodies when lowering ast to hir)
 - rust-lang#74314 (rustc_typeck: construct {Closure,Generator}Substs more directly.)
 - rust-lang#74346 (Use LocalDefId instead of HirId for reachable_set elements.)
 - rust-lang#74399 (Move DelaySpanBugEmitted to ty::context)
 - rust-lang#75177 (Add regression test for issue-66768)
 - rust-lang#75223 (Add #[track_caller] to `Session::delay_span_bug`)
 - rust-lang#75423 (Move to intra-doc links for /library/core/src/hint.rs)
 - rust-lang#75485 (pin docs: add some forward references)
 - rust-lang#75569 (Bump minor version of emsdk to 1.38.47)
 - rust-lang#75596 (Switch to intra-doc links in /sys/windows/ext/{ffi,fs,process}.rs)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Aug 16, 2020

⌛ Testing commit d442bf7 with merge 7e6d6e5...

@bors bors merged commit 8eba138 into rust-lang:master Aug 16, 2020
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 6, 2022
…=cjgillot

Don't resolve blocks in foreign functions

Although it is an error for a foreign function to have a block, it is still possible at the level of the AST. rust-lang#74204 made AST lowering skip over blocks belonging to foreign functions, since they're invalid. However, resolve still treated these blocks normally, resulting in a mismatch between the HIR and resolve, which could cause an ICE under certain circumstances. This PR changes resolve to skip over blocks belonging to foreign functions, as AST lowering does.

Fixes rust-lang#91370.

r? `@cjgillot`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 9, 2022
…=cjgillot

Don't resolve blocks in foreign functions

Although it is an error for a foreign function to have a block, it is still possible at the level of the AST. rust-lang#74204 made AST lowering skip over blocks belonging to foreign functions, since they're invalid. However, resolve still treated these blocks normally, resulting in a mismatch between the HIR and resolve, which could cause an ICE under certain circumstances. This PR changes resolve to skip over blocks belonging to foreign functions, as AST lowering does.

Fixes rust-lang#91370.

r? ``@cjgillot``
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.

ICE on invalid input: fn f(){;
7 participants