1478902 - Hardern types in JIT nursery strings allocation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Paul Bone [:pbone]]

In Bug 1476500 I forgot to make the similar change for nursery strings.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1478902 - Improve types in nursery allocation

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8995402 [details] [diff] [review]
Bug 1478902 - Improve types in nursery allocation

Review of attachment 8995402 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler.cpp
@@ +990,4 @@
>  
>      movePtr(ImmPtr(zone->addressOfNurseryPosition()), temp);
>      loadPtr(Address(temp, 0), result);
>      addPtr(Imm32(totalSize), result);

existing nit: The comment above state that this is true, but this deserves a proper assertions:

    MOZ_ASSERT(int32_t(totalSize) == totalSize, "Encode nursery displacement instead of 64 bits immediates");
    MOZ_ASSERT(
       reinterpret_cast<uint8_t*>(zone->addressOfNurseryPosition()) + totalSize == zone->addressOfStringNurseryPosition(),
       "String nursery is at some small offset within the nursery");

@@ +991,5 @@
>      movePtr(ImmPtr(zone->addressOfNurseryPosition()), temp);
>      loadPtr(Address(temp, 0), result);
>      addPtr(Imm32(totalSize), result);
> +    branchPtr(Assembler::Below, Address(temp, 
> +        uintptr_t(nurseryEndAddr) - uintptr_t(nurseryPosAddr)), result, fail);

nit: Move this outside this expression and give it a name:
    ptrdiff_t stringNurserySize = … - …;

Comment 3

[Paul Bone [:pbone]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #2)
> Comment on attachment 8995402 [details] [diff] [review]
> Bug 1478902 - Improve types in nursery allocation
> 
> Review of attachment 8995402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MacroAssembler.cpp
> @@ +990,4 @@
> >  
> >      movePtr(ImmPtr(zone->addressOfNurseryPosition()), temp);
> >      loadPtr(Address(temp, 0), result);
> >      addPtr(Imm32(totalSize), result);
> 
> existing nit: The comment above state that this is true, but this deserves a
> proper assertions:
> 
>     MOZ_ASSERT(int32_t(totalSize) == totalSize, "Encode nursery displacement
> instead of 64 bits immediates");
>     MOZ_ASSERT(
>        reinterpret_cast<uint8_t*>(zone->addressOfNurseryPosition()) +
> totalSize == zone->addressOfStringNurseryPosition(),
>        "String nursery is at some small offset within the nursery");

I can see similar problems elsewhere, therefore I'll fix this in a seperate bug.

> @@ +991,5 @@
> >      movePtr(ImmPtr(zone->addressOfNurseryPosition()), temp);
> >      loadPtr(Address(temp, 0), result);
> >      addPtr(Imm32(totalSize), result);
> > +    branchPtr(Assembler::Below, Address(temp, 
> > +        uintptr_t(nurseryEndAddr) - uintptr_t(nurseryPosAddr)), result, fail);
> 
> nit: Move this outside this expression and give it a name:
>     ptrdiff_t stringNurserySize = … - …;

Okay.

Comment 4

[Paul Bone [:pbone]]

(In reply to Paul Bone [:pbone] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #2)
> > ::: js/src/jit/MacroAssembler.cpp
> > @@ +990,4 @@
> > >  
> > >      movePtr(ImmPtr(zone->addressOfNurseryPosition()), temp);
> > >      loadPtr(Address(temp, 0), result);
> > >      addPtr(Imm32(totalSize), result);
> > 
> > existing nit: The comment above state that this is true, but this deserves a
> > proper assertions:
> > 
> >     MOZ_ASSERT(int32_t(totalSize) == totalSize, "Encode nursery displacement
> > instead of 64 bits immediates");
> >     MOZ_ASSERT(
> >        reinterpret_cast<uint8_t*>(zone->addressOfNurseryPosition()) +
> > totalSize == zone->addressOfStringNurseryPosition(),
> >        "String nursery is at some small offset within the nursery");
> 
> I can see similar problems elsewhere, therefore I'll fix this in a seperate
> bug.

I think these assertions arn't correct, and I will add them on this bug, but as seperate patches.

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1478902 Part 2 - Nursery strings allocation code uses the wrong pointer

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1478902 Part 3 - Add assertions to JIT nursery strings allocation

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1478902 Part 3 - Add assertions to JIT nursery strings allocation

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8995878 [details] [diff] [review]
Bug 1478902 Part 3 - Add assertions to JIT nursery strings allocation

Review of attachment 8995878 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler.cpp
@@ +980,4 @@
>      addPtr(Imm32(totalSize), result);
>      const ptrdiff_t endOffset =
>          uintptr_t(nurseryEndAddr) - uintptr_t(nurseryPosAddr);
> +    MOZ_ASSERT(endOffset < ptrdiff_t(1)<<31,

nit: ptrdiff_t is a signed type. This might not do what you expect on 32 bits architectures.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8995872 [details] [diff] [review]
Bug 1478902 Part 2 - Nursery strings allocation code uses the wrong pointer

Review of attachment 8995872 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch! Nice catch, thanks.

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1478902 (squash into part 3) - Use CheckedInt for assertions

I'll roll this into the previous change that adds the assertions.  But
sfink suggested this and I wanted to get his r+ first.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b92b3fbb206
Part 1 - Improve types in nursery allocation r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1f02d10be7
Part 2 - Nursery strings allocation code uses the wrong pointer r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/07a0de606788
Part 3 - Add assertions to JIT nursery strings allocation r=nbp,sfink

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6b92b3fbb206
https://hg.mozilla.org/mozilla-central/rev/ce1f02d10be7
https://hg.mozilla.org/mozilla-central/rev/07a0de606788