615181 - Extend SlotStorageType enum to allow more types

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Virgil Palanciuc]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.44 Safari/534.7
Build Identifier: 

SlotStorageType now has 8 members and is stored on 3 bits. In order to be able to add new types (like float, float4 etc) we need to extend this enum with new members

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: Patch to enlarge SlotStorageType

Comment 2

[Steven Johnson]

Comment on attachment 493714 [details] [diff] [review]
Patch to enlarge SlotStorageType

I'm guessing you want someone else to review the patch?

Comment 3

[Steven Johnson]

Comment on attachment 493714 [details] [diff] [review]
Patch to enlarge SlotStorageType

One nit: the comment

// offset is always a multiple of 4 so skip those, gives us a max of 1<<31-1

Is no longer correct, since MAX_SLOT_OFFSET is different now.

Comment 4

[Virgil Palanciuc]

Attachment: updated patch to enlarge SlotStorageType

I fixed the comment, removed changes related to bibop + project change.
This should be theoretically "good to go" if you want to take it in mainline

Comment 5

[Lars T Hansen]

Comment on attachment 493924 [details] [diff] [review]
updated patch to enlarge SlotStorageType

Redirecting review to Edwin for feedback, this is not my area of expertise and there's no really good reason to push for this to land right away.

(The comments about what the situation "was" vs what it "is" and who did what to it do not normally belong in the code; if we're curious about a file's history we have the mercurial log.  Documentation should pertain to what the code currently does.)

Comment 6

[Virgil Palanciuc]

Attachment: patch to enlarge SlotStorageType

fixed comment as per Lars' remarks

Comment 7

[Edwin Smith]

Comment on attachment 493932 [details] [diff] [review]
patch to enlarge SlotStorageType

Looks okay.  A few new comments should be capitalized and end with .

The most important part of this patch is the argumentation that the reduced
upper limit on slot offset is still fine.  I think it is, but did not review
that part very carefully because I couldn't find the existing argumentation
for why 29 bits is okay.  (asking Steven for feedback).

The function tables in jit-calls.h could grow when MAX_SST changes, and it would be a bug if they didn't get new entries.  if we are super paranoid, a static
assertion would ensure we fail fast.  but we'd probably fail fast anyway and
I'm not *that* paranoid.

Comment 8

[Steven Johnson]

29 bits was OK because we stored the offset in 4-byte-words, not bytes, so we could really express 31 bits of range in terms of byte-offset. Since MMgc never allowed allocations larger than 2GB anyway, this was safe. This reduces our effective range to 1GB, which is almost certainly fine as a practical matter, but probably is worth adding a resolve-time range check to ensure we don't overflow.

Comment 9

[Lars T Hansen]

FWIW the object size limit in MMgc is now approx 4GB (actually a hair below that).

Comment 10

[Steven Johnson]

Comment on attachment 493932 [details] [diff] [review]
patch to enlarge SlotStorageType

F+ conditional on adding a resolve-time range check to ensure we don't
overflow.

Comment 11

[Virgil Palanciuc]

Attachment: enlarge SlotStorageType

Really only a rebase + a small comment change.
@Steven - by "adding a resolve-time range check" do you mean something different than the assert that already exists in Traits-inlines.h / setSlotInfo() ?

Comment 12

[Virgil Palanciuc]

Attachment: enlarge SlotStorageType

(resubmitted)

Really only a rebase + a small comment change.
@Steven - by "adding a resolve-time range check" do you mean something different than the assert that already exists in Traits-inlines.h / setSlotInfo() ?

Comment 13

[Virgil Palanciuc]

Attachment: enlarge SlotStorageType

Comment 14

[Steven Johnson]

(In reply to comment #12)
> @Steven - by "adding a resolve-time range check" do you mean something
> different than the assert that already exists in Traits-inlines.h /
> setSlotInfo() ?

Hm, nope that will probably do it.

Comment 15

[Tamarin Bot]

changeset: 6396:457203bf8d0d
user:      Lars T Hansen <[e-mail]>
summary:   Fix 615181 - Extend SlotStorageType enum to allow more types (patch=virgilp, r=edwsmith, f=stejohns, push=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/457203bf8d0d

Comment 16

[Brent Baker]

Lars/Virgil: Is there anything that needs to be done before I can mark this as "Verified"?