Skip to content

Adapt AddRetag for shallow retagging #63306

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Aug 15, 2019
Merged

Adapt AddRetag for shallow retagging #63306

merged 3 commits into from
Aug 15, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 5, 2019

With rust-lang/miri#872, Miri only retags "bare" references, not those nested in compound types. This adjust Retag statement generation to don't emit retags if they are definitely not a bare reference.

I also expanded the mir-opt test to cover the Retag in the drop shim, which had previously not been tested.

@rust-highfive
Copy link
Contributor

r? @varkor

(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 5, 2019
@varkor
Copy link
Member

varkor commented Aug 11, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 11, 2019

📌 Commit 3df672f has been approved by varkor

@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 11, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 12, 2019
Adapt AddRetag for shallow retagging

With rust-lang/miri#872, Miri only retags "bare" references, not those nested in compound types. This adjust `Retag` statement generation to don't emit retags if they are definitely not a bare reference.

I also expanded the mir-opt test to cover the `Retag` in the drop shim, which had previously not been tested.
bors added a commit that referenced this pull request Aug 12, 2019
Rollup of 10 pull requests

Successful merges:

 - #62108 (Use sharded maps for queries)
 - #63297 (Improve pointer offset method docs)
 - #63306 (Adapt AddRetag for shallow retagging)
 - #63406 (Suggest using a qualified path in patterns with inconsistent bindings)
 - #63431 (Revert "Simplify MIR generation for logical ops")
 - #63449 (resolve: Remove remaining special cases from built-in macros)
 - #63461 (docs: add stdlib env::var(_os) panic)
 - #63473 (Regression test for #56870)
 - #63474 (Add tests for issue #53598 and #57700)
 - #63480 (Fixes #63477)

Failed merges:

r? @ghost
@Centril
Copy link
Contributor

Centril commented Aug 12, 2019

Failed in #63482 (comment), @bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2019
@RalfJung
Copy link
Member Author

That is odd, I ran these locally before pushing... probably a merge conflict.

@Centril
Copy link
Contributor

Centril commented Aug 12, 2019

Seems likely yeah; might interact with the change in MIR building?

@RalfJung
Copy link
Member Author

I rebased and it still passed locally, so my guess is the conflict is with another PR that got rolled up together.

@bors rollup-

I guess I'll just wait until the MIR building change lands? This one here has very low priority.

@RalfJung RalfJung force-pushed the retag branch 2 times, most recently from 49fb2a7 to b122be4 Compare August 12, 2019 14:25
@RalfJung
Copy link
Member Author

I cannot reproduce a failure locally even after rebasing. Let's see what CI on this branch says. If it is green as well my gut feeling is to let bors try again...?

@Centril
Copy link
Contributor

Centril commented Aug 12, 2019

SGTM.

@RalfJung
Copy link
Member Author

Branch CI is green. I also tested stage 2 locally, green as well.

@bors r=varkor

@bors
Copy link
Collaborator

bors commented Aug 12, 2019

📌 Commit b122be4 has been approved by varkor

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Aug 12, 2019

@bors p=-1 rollup=never

but let's wait till after beta cutoff

@RalfJung
Copy link
Member Author

Beta has branched, let's raise priority to normal again.

@bors p=0

Centril added a commit to Centril/rust that referenced this pull request Aug 15, 2019
Adapt AddRetag for shallow retagging

With rust-lang/miri#872, Miri only retags "bare" references, not those nested in compound types. This adjust `Retag` statement generation to don't emit retags if they are definitely not a bare reference.

I also expanded the mir-opt test to cover the `Retag` in the drop shim, which had previously not been tested.
@Centril
Copy link
Contributor

Centril commented Aug 15, 2019

Failed in #63583 (comment), @bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 15, 2019
@RalfJung
Copy link
Member Author

Hm, that "actual" MIR contains no unwinding. Might this be a platform that doesn't support unwinding, which would change MIR generation?

@Centril
Copy link
Contributor

Centril commented Aug 15, 2019

cc @matthewjasper ^---

@RalfJung
Copy link
Member Author

Ah, I found another test with a comment like

// ignore-wasm32-bare compiled with panic=abort by default

I added that here, too. I hope that does it.

@bors r=varkor

@bors
Copy link
Collaborator

bors commented Aug 15, 2019

📌 Commit 2122fe4 has been approved by varkor

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 15, 2019
Adapt AddRetag for shallow retagging

With rust-lang/miri#872, Miri only retags "bare" references, not those nested in compound types. This adjust `Retag` statement generation to don't emit retags if they are definitely not a bare reference.

I also expanded the mir-opt test to cover the `Retag` in the drop shim, which had previously not been tested.
bors added a commit that referenced this pull request Aug 15, 2019
Rollup of 9 pull requests

Successful merges:

 - #63155 (Add UWP MSVC targets)
 - #63165 (Add builtin targets for mips64(el)-unknown-linux-muslabi64)
 - #63306 (Adapt AddRetag for shallow retagging)
 - #63467 (Add Catalyst (iOS apps running on macOS) target)
 - #63546 (Remove uses of `mem::uninitialized()` from cloudabi)
 - #63572 (remove unused Level::PhaseFatal)
 - #63577 (Test HRTB issue accepted by compiler)
 - #63582 (Fix ICE #63226)
 - #63586 (cleanup: Remove `Spanned` where possible)

Failed merges:

r? @ghost
@bors bors merged commit 2122fe4 into rust-lang:master Aug 15, 2019
@RalfJung RalfJung deleted the retag branch August 15, 2019 20:25
# for free to join this conversation on GitHub. Already have an account? # 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.

5 participants