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

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

Merged
merged 6 commits into from Nov 19, 2018

Conversation

Projects
None yet
7 participants
@RalfJung
Member

RalfJung commented Nov 4, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 4, 2018

r? @varkor

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

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 4, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 4, 2018

Like this?

@RalfJung RalfJung force-pushed the RalfJung:miri-extern-types branch from aa34f15 to aca76d4 Nov 4, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 4, 2018

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

@@ -354,7 +354,8 @@ where
let (meta, offset) = if field_layout.is_unsized() {
// re-use parent metadata to determine dynamic field layout
let (_, align) = self.size_and_align_of(base.meta, field_layout)?
.expect("Fields cannot be extern types");
// If this is an extern type, we fall back to its static size and alignment.
.unwrap_or_else(|| base.layout.size_and_align());

This comment has been minimized.

@eddyb

eddyb Nov 4, 2018

Member

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!

This comment has been minimized.

@eddyb

eddyb Nov 4, 2018

Member

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

This comment has been minimized.

@RalfJung

RalfJung Nov 4, 2018

Member

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.

This comment has been minimized.

@eddyb

eddyb Nov 4, 2018

Member

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.

This comment has been minimized.

@RalfJung

RalfJung Nov 5, 2018

Member

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.

This comment has been minimized.

@RalfJung

RalfJung Nov 5, 2018

Member

I implemented what you suggested.

@RalfJung RalfJung referenced this pull request Nov 5, 2018

Open

Tracking issue for RFC 1861: Extern types #43467

1 of 3 tasks complete
@eddyb

eddyb approved these changes Nov 5, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 5, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

📌 Commit a6622c2 has been approved by eddyb

// FIXME: Once we have made decisions for how to handle size and alignment
// of `extern type`, this should be adapted. It is just a temporary hack
// to get some code to work that probably ought to work.
.unwrap_or_else(|| base.layout.align);

This comment has been minimized.

@RalfJung

RalfJung Nov 5, 2018

Member

Should this really be base.layout, not field_layout?

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 5, 2018

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

This comment has been minimized.

Member

eddyb commented Nov 5, 2018

@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

This comment has been minimized.

Member

RalfJung commented Nov 6, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 6, 2018

I fixed that, I think.

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 6, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

📌 Commit ba0bab3 has been approved by eddyb

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 16 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55871 (codegen_llvm_back: improve allocations)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 16, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 16 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55871 (codegen_llvm_back: improve allocations)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 16, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 16, 2018

Auto merge of #55672 - RalfJung:miri-extern-types, r=eddyb
miri: accept extern types in structs if they are the only field

Fixes #55541

Cc @oli-obk @eddyb #43467
@bors

This comment has been minimized.

Contributor

bors commented Nov 16, 2018

⌛️ Testing commit ba0bab3 with merge b2282c6...

@bors

This comment has been minimized.

Contributor

bors commented Nov 16, 2018

💔 Test failed - status-appveyor

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 16, 2018

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?

kennytm added a commit to kennytm/rust that referenced this pull request Nov 17, 2018

Rollup merge of rust-lang#55672 - RalfJung:miri-extern-types, r=eddyb
miri: accept extern types in structs if they are the only field

Fixes rust-lang#55541

Cc @oli-obk @eddyb rust-lang#43467

bors added a commit that referenced this pull request Nov 17, 2018

Auto merge of #56026 - kennytm:rollup, r=kennytm
Rollup of 17 pull requests

Successful merges:

 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55767 (Disable some pretty-printers when gdb is rust-enabled)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55867 (do not panic just because cargo failed)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55999 (Update Cargo submodule)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56017 (std: Add debugging for a failing test on appveyor)

Failed merges:

 - #56010 (fix intra-link resolution spans in block comments)

r? @ghost
@bors

This comment has been minimized.

Contributor

bors commented Nov 17, 2018

⌛️ Testing commit ba0bab3 with merge 54966b1...

bors added a commit that referenced this pull request Nov 17, 2018

Auto merge of #55672 - RalfJung:miri-extern-types, r=eddyb
miri: accept extern types in structs if they are the only field

Fixes #55541

Cc @oli-obk @eddyb #43467
@bors

This comment has been minimized.

Contributor

bors commented Nov 17, 2018

💔 Test failed - status-appveyor

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 18, 2018

@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

This comment has been minimized.

Contributor

bors commented Nov 18, 2018

⌛️ Testing commit ba0bab3 with merge a9b791b...

bors added a commit that referenced this pull request Nov 18, 2018

Auto merge of #55672 - RalfJung:miri-extern-types, r=eddyb
miri: accept extern types in structs if they are the only field

Fixes #55541

Cc @oli-obk @eddyb #43467
@bors

This comment has been minimized.

Contributor

bors commented Nov 19, 2018

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

@bors bors merged commit ba0bab3 into rust-lang:master Nov 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@RalfJung RalfJung deleted the RalfJung:miri-extern-types branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment