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

-Zdump-mir does not dump all the MIR: drop glue missing #53532

Closed
RalfJung opened this issue Aug 20, 2018 · 13 comments · Fixed by #58103
Closed

-Zdump-mir does not dump all the MIR: drop glue missing #53532

RalfJung opened this issue Aug 20, 2018 · 13 comments · Fixed by #58103
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 20, 2018

There seems to be no way currently to dump at least some of the MIR that rustc generates itself, e.g. for drop.

In the following code

struct Bar(u16); // ZSTs are tested separately

static mut DROP_COUNT: usize = 0;

impl Drop for Bar {
    fn drop(&mut self) {
        assert_eq!(self.0 as usize, unsafe { DROP_COUNT }); // tests whether we are called at a valid address
        unsafe { DROP_COUNT += 1; }
    }
}

fn main() {
    let b = [Bar(0), Bar(1), Bar(2), Bar(3)];
    assert_eq!(unsafe { DROP_COUNT }, 0);
    drop(b);
    assert_eq!(unsafe { DROP_COUNT }, 4);

    // check empty case
    let b : [Bar; 0] = [];
    drop(b);
    assert_eq!(unsafe { DROP_COUNT }, 4);
}

I can clearly see miri execute an Offset binop (a binop which is only ever emitted by the auto-generated drop glue), but grepping the entire dump directory for Offset shows no hit.

@RalfJung
Copy link
Member Author

From Zulip:

I'd guess we're not running any passes on shims and thus they don't get their mir dumped, either

So... instance_mir calls make_shim (transitively), which does run a few passes, but it runs them manually instead of via the regular run_passes scheme

@RalfJung
Copy link
Member Author

RalfJung commented Nov 22, 2018

I got started on this, wrote https://github.com/RalfJung/rust/commits/mir-shim-dump/. Now it tries to dump MIR for shims. The problem is, in

let item_name = tcx.hir
.def_path(source.def_id)
.to_filename_friendly_no_crate();

we call def_path, which asserts that the DefId comes from the local crate, but for shims that is not always true (it could be core::ptr::drop_in_place).

@oli-obk @eddyb any suggestions?

@eddyb
Copy link
Member

eddyb commented Nov 22, 2018

@RalfJung Use the method on tcx that works cross-crate.

@RalfJung
Copy link
Member Author

@eddyb Thanks! Now it gets a bit further and panics at

fn write_mir_sig(tcx: TyCtxt, src: MirSource, mir: &Mir, w: &mut dyn Write) -> io::Result<()> {
let id = tcx.hir.as_local_node_id(src.def_id).unwrap();
let body_owner_kind = tcx.hir.body_owner_kind(id);

because, well, this is not a local node.

I wonder if I should approach this differently -- special case the drop shim for printing?

@eddyb
Copy link
Member

eddyb commented Nov 22, 2018

I'd say it's worth fixing these. Instead of checking the body owner kind, could it check tcx.describe_def? Only Def::Fn and Def::Method should have method arguments and Def::Static would make it print static.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 23, 2018

Dumping MIR of another crate works now. However, MIR dumping expects to dump polymorphic MIR, meaning (a) all drop shims get the same filename, overwriting one another, and (b) in the dumped MIR, you cannot even see which type this is dropping.

I guess I have to adjust to_filename_friendly_no_crate at the least?

@eddyb
Copy link
Member

eddyb commented Nov 23, 2018

@RalfJung What about appending instead of overwriting if the file is one that was created by the current MIR dumping? (i.e. by keeping a set of used file names)

@RalfJung
Copy link
Member Author

That sounds like a real bad hack. :/ Not to mention that it'd require some global state.

@eddyb
Copy link
Member

eddyb commented Nov 23, 2018

Oh the printing is a side-effect of building MIR? Oops, I forgot.
Well then, put a (type) hash in the file name (cc @michaelwoerister)

@RalfJung
Copy link
Member Author

RalfJung commented Nov 24, 2018

I hear @matthewjasper has a branch where dumping shims at least somewhat works. Could you point me to that branch?

(EDIT: It's https://github.com/matthewjasper/rust/tree/dump-mir-shims)

@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Jan 27, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Feb 2, 2019

I rebased the branch to see if I can make some more progress, but what used to work doesn't work any more. I am now getting a panic in this line when dumping the MIR for DefId(0/1:11 ~ test[317d]::main[0]::{{constant}}[0]) (using the code from the OP).

Any idea why that DefId cannot be described?

@RalfJung
Copy link
Member Author

RalfJung commented Feb 2, 2019

Ah, turns out this DefId is an AnonConst, and describe does not like those. But why?

Cc @oli-obk

@RalfJung
Copy link
Member Author

RalfJung commented Feb 3, 2019

Ah I got something, it's all in https://github.com/RalfJung/rust/commits/mir-shim-dump/. I also extended MirSource to take an InstanceDef so that the filename can be computed appropriately.

bors added a commit that referenced this issue Feb 10, 2019
Make -Zdump-mir dump shims

Fixes #53532 by (a) making the MIR shim generation use the MIR pass infrastructure, and (b) fixing said infrastructure to handle the fallout.

Cc @eddyb @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants