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.

Sign up for GitHub

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

Jump to bottom

Allow Size to be any valid u64 #50916

Merged
merged 1 commit into from
May 23, 2018
Merged

Allow Size to be any valid u64 #50916

merged 1 commit into from
May 23, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 20, 2018 •
edited
Loading

cc rust-lang/miri#378 (comment)

The alternative is to make mir::interpret's pointer offsets not be Size

fixes #50917

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2018
@RalfJung
Copy link
Member

LLVM doesn't support allocations or types larger than half the address space (and I am not sure about exactly half the address space). Is there any risk of those coming up after this patch?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 20, 2018

We do have tests for these kinds of types. Not sure how to do tests for allocations of such sizes xD

eddyb
eddyb reviewed May 20, 2018
View reviewed changes
src/librustc/mir/interpret/mod.rs Outdated
@@ -117,6 +117,10 @@ impl<'tcx> MemoryPointer {
MemoryPointer { alloc_id, offset }
}

pub fn zero(alloc_id: AllocId) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a From impl?

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 considered that. But i think being explicit is better. I don't feel too strongly about that though, so if you want I'll change it

Copy link
Member

Choose a reason for hiding this comment

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

I think zero is misleading, I initially assumed it'd be a null pointer.

Copy link
Member

@eddyb eddyb May 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, random thought: maybe MemoryPointer should be AllocPointer or AbstractPointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just name it Pointer and rename the current Pointer struct to PrimvalPointer

Copy link
Member

@eddyb eddyb May 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That seems worse. Maybe this?

enum Scalar {
    Undef,
    Bits(u128),
    Ptr(Pointer),
}
enum Value {
    Scalar(Scalar),
    ScalarPair(Scalar, Scalar),
    ByRef(Pointer, Align),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the current struct Pointer(Primval) type? It's just a wrapper around a PrimVal, but helps us not confuse normal PrimVals with those whose type is a pointer type.

Copy link
Member

Choose a reason for hiding this comment

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

The type doesn't really matter, the value does (unless there are pointer-specific methods on the wrapper?). And I prefer Scalar over Primval, but I also forgot the latter is really a thing.

@eddyb
Copy link
Member

eddyb commented May 20, 2018

cc @alexcrichton @nagisa @arielb1 @rkruppe
Does any of you remember what failure mode LLVM had when we started adding type size checks?
This PR does not affect any of the checks done when computing sizes of ADTs, arrays, etc. - but Size can now contain byte counts that could show up as negative if placed in signed integers.
(or overflow if multiplied by 8 other than through the .bits() method)

@alexcrichton
Copy link
Member

Hm I don't recall myself, but others might!

@eddyb
Copy link
Member

eddyb commented May 21, 2018

@bors r+

@bors
Copy link
Contributor

bors commented May 21, 2018

📌 Commit 9f79a19 has been approved by eddyb

@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 May 21, 2018
@oli-obk oli-obk mentioned this pull request May 22, 2018
Merged
@bors
Copy link
Contributor

bors commented May 23, 2018

⌛ Testing commit 9f79a19 with merge 29ffe51...

bors added a commit that referenced this pull request May 23, 2018
@bors
Auto merge of #50916 - oli-obk:miri, r=eddyb
29ffe51 
Allow `Size` to be any valid `u64`

cc rust-lang/miri#378 (comment)

The alternative is to make mir::interpret's pointer offsets not be `Size`

fixes #50917

r? @eddyb
@bors
Copy link
Contributor

bors commented May 23, 2018

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

@bors bors merged commit 9f79a19 into rust-lang:master May 23, 2018
@oli-obk oli-obk deleted the miri branch May 25, 2018 13:15
bors added a commit that referenced this pull request May 25, 2018
@bors
Auto merge of #50967 - oli-obk:miri_api_refactor, r=eddyb
990d8aa 
Miri api refactor

r? @eddyb

cc @Zoxc

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

@eddyb eddyb eddyb left review comments

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

thread 'main' panicked at 'Size::from_bytes: 18446744073709551614 bytes in bits doesn't fit in u64
6 participants
@oli-obk @RalfJung @eddyb @alexcrichton @bors @rust-highfive