miri: accept extern types in structs if they are the only field

[RalfJung]

Fixes #55541

Cc @oli-obk @eddyb #43467

[rust-highfive]

r? @varkor

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

[eddyb]

I would prefer it if this was based on offset being 0, not field count.

[RalfJung]

Like this?

[eddyb]

I'm not sure the alignment condition is needed, since 0 is aligned to infinity, but I don't think it hurts.

[eddyb]

Interestingly, we can make size_and_align_of error, but have a separate align_of that works even for extern type. Only the alignment is used here, anyway!

[eddyb]

Oh, you can also just move the Option to be around the size, but not the alignment.

[RalfJung]

I could, but I'd rather wait for the decision to treat the size_of_val and align_of_val intrinsics asymmetrically before doing that.

extern type will likely get alignment 1 currently, which is likely not actually correct for the type they are opaquely representing, so I am a bit worried about unilaterally implementing such behavior.

[eddyb]

You can't access that alignment without first having a reference, so having it too high would be a problem (the assumption might not be guaranteed by what you're FFI-ing to), but having it too low wouldn't break anything (other than field offsets).

So the way I see it, the alignment is only a lower bound, and 1 is fine, unless you want to use it as an aligned field in a struct.

Anyway, my concern is also kind of a confusing code thing.
You could also add a .map(|(_, align)| align) before unwrap_or_else, which would mean the code and the comment wouldn't mention the "size" of the extern type.

[RalfJung]

You can't access that alignment without first having a reference, so having it too high would be a problem (the assumption might not be guaranteed by what you're FFI-ing to), but having it too low wouldn't break anything (other than field offsets).

Oh that's a good point. We cannot create such references ourselves anyway as we cannot have data of that type.

Still I think miri shouldn't move ahead of the rest of the decision process here.

I will implement your map suggestion.

[RalfJung]

I implemented what you suggested.

[eddyb]

@bors r+

[bors]

📌 Commit a6622c2 has been approved by eddyb

[RalfJung]

Should this really be base.layout, not field_layout?

[RalfJung]

Hm actually I am not entirely sure I got this right, will this error out appropriately when there are other fields? It will error out in size_and_align_of, but what about mplace_field? It does not do any such check.

[eddyb]

@RalfJung Ah, yeah, you probably want it in both. This is why decoupling alignment from size should help - size and offset computation are similar/equivalent, whereas alignment is simpler.

[RalfJung]

Offset computation of a field only uses the field's alignment, though (and the offsets data in the layout).

[RalfJung]

I fixed that, I think.

[eddyb]

@bors r+

[bors]

📌 Commit ba0bab3 has been approved by eddyb

[bors]

⌛ Testing commit ba0bab3 with merge b2282c6f23a0d1c0e850f6cf871e5cbe8b1985f9...

[bors]

💔 Test failed - status-appveyor

[RalfJung]

It's that timing test again:

failures:
    time::tests::instant_math

@bors retry

@rust-lang/infra maybe the permitted time difference should be increased to 500ms or so?

[bors]

⌛ Testing commit ba0bab3 with merge 54966b13b4f4b6d5d168c952194b6f9e2e73551b...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry

error: failed to remove file `C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\cit\t989\foo\target\release\libfoo.rmeta`
Caused by:
  Access is denied. (os error 5)

[bors]

⌛ Testing commit ba0bab3 with merge a9b791b...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing a9b791b to master...