Skip to content

Fix clashing_extern_declarations stack overflow for recursive types. #75554

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

Merged
merged 10 commits into from
Aug 19, 2020

Conversation

jumbatm
Copy link
Contributor

@jumbatm jumbatm commented Aug 15, 2020

Fixes #75512.

Adds a seen set to structurally_same_type to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.

@rust-highfive
Copy link
Contributor

r? @petrochenkov

(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 Aug 15, 2020
@petrochenkov
Copy link
Contributor

Could someone else review this? I don't have the capacity.
r? @lcnr for example

@rust-highfive rust-highfive assigned lcnr and unassigned petrochenkov Aug 15, 2020
structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind)
}
(RawPtr(a_tymut), RawPtr(b_tymut)) => {
a_tymut.mutbl == b_tymut.mutbl
Copy link
Contributor

Choose a reason for hiding this comment

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

while not part of this PR I would expect *const T and *mut T to be structurally equal.

To my knowledge they only differ in variance

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 see. My reasoning for having them not equal is because I want this case to clash:

mod a {
    extern "C" {
        fn take(_: *const S);
    }
}
mod b {
    extern "C" {
        fn take(_: *mut S);
    }
}

Specifically, I want to guard against the case where take actually mutates the data underneath, but a::take is called with data that was supposed to be immutable -- a conflicting declaration of the mutability of some data should ring some alarm bells.

@lcnr lcnr 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 15, 2020
@jumbatm
Copy link
Contributor Author

jumbatm commented Aug 16, 2020

@rustbot modify labels to -S-waiting-on-author, +S-waiting-on-review

@rustbot rustbot 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 Aug 16, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

small nits, otherwise this is looking good 👍

@lcnr lcnr 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 16, 2020
@jumbatm jumbatm force-pushed the fix-clashing-extern-decl-overflow branch from 0e28346 to af0fb0e Compare August 16, 2020 08:38
@jumbatm
Copy link
Contributor Author

jumbatm commented Aug 16, 2020

Cool, this is good to go.

@rustbot modify labels to -S-waiting-on-author, +S-waiting-on-review

@rustbot rustbot 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 Aug 16, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

Looks good to me, r=me after the comment by @nagisa is resolved

Adds a seen set to structurally_same_type to avoid recursing
indefinitely when a reference or pointer member introduces a cycle in
the visited types.
That cache is unlikely to be particularly useful within a single
invocation of structurally_same_type, especially compared to memoizing
results across _all_ invocations of that function.
@jumbatm jumbatm force-pushed the fix-clashing-extern-decl-overflow branch from af0fb0e to bc15dd6 Compare August 17, 2020 16:26
@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

⌛ Trying commit bc15dd6 with merge 6f54fb4d9fd2b0e3b6dec01b4b67bd2692cbb454...

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6f54fb4d9fd2b0e3b6dec01b4b67bd2692cbb454 (6f54fb4d9fd2b0e3b6dec01b4b67bd2692cbb454)

@rust-timer
Copy link
Collaborator

Queued 6f54fb4d9fd2b0e3b6dec01b4b67bd2692cbb454 with parent b97e9b5, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6f54fb4d9fd2b0e3b6dec01b4b67bd2692cbb454): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2020

perf looks neutral to me

@bors r+ rollup=always

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

📌 Commit bc15dd6 has been approved by lcnr

@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 18, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…erflow, r=lcnr

Fix clashing_extern_declarations stack overflow for recursive types.

Fixes rust-lang#75512.

Adds a seen set to `structurally_same_type` to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…erflow, r=lcnr

Fix clashing_extern_declarations stack overflow for recursive types.

Fixes rust-lang#75512.

Adds a seen set to `structurally_same_type` to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…erflow, r=lcnr

Fix clashing_extern_declarations stack overflow for recursive types.

Fixes rust-lang#75512.

Adds a seen set to `structurally_same_type` to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…erflow, r=lcnr

Fix clashing_extern_declarations stack overflow for recursive types.

Fixes rust-lang#75512.

Adds a seen set to `structurally_same_type` to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…erflow, r=lcnr

Fix clashing_extern_declarations stack overflow for recursive types.

Fixes rust-lang#75512.

Adds a seen set to `structurally_same_type` to avoid recursing indefinitely for types which contain values of the same type through a pointer or reference.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#75038 (See also X-Link mem::{swap, take, replace})
 - rust-lang#75049 (docs(marker/copy): provide example for `&T` being `Copy`)
 - rust-lang#75499 (Fix documentation error)
 - rust-lang#75554 (Fix clashing_extern_declarations stack overflow for recursive types.)
 - rust-lang#75646 (Move to intra doc links for keyword documentation)
 - rust-lang#75652 (Resolve true and false as booleans)
 - rust-lang#75658 (Don't emit "is not a logical operator" error outside of associative expressions)
 - rust-lang#75665 (Add doc examples coverage)
 - rust-lang#75685 (Switch to intra-doc links in /src/sys/unix/ext/*.rs)

Failed merges:

r? @ghost
@bors bors merged commit e8d30bf into rust-lang:master Aug 19, 2020
@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.

Stack overflow in rustc frontend on Windows
9 participants