Bug 1700690 Comment 5 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

The crash location is the third instruction in the stub for the second division:
```
   0x2418c930:	push   %ebp
   0x2418c931:	mov    %esp,%ebp
=> 0x2418c933:	mov    0x10(%esi),%ecx
   0x2418c936:	mov    0xdc(%ecx),%ecx
   0x2418c93c:	movl   $0x41,0x40(%ecx)
   0x2418c943:	or     $0x1,%ebp
   ...
```
$esi is the TLS register.  Looking back up in the calling function, we find that it is read from the top of the frame, right above the area that is used for the multi-value return.  Every indication is that the multi-value return scribbles over the saved TLS in the frame.  In the frame (when no space has been allocated for outgoing args and no words have been pushed temporarily) the TLS is at sp+24.  The return area address is computed as sp+20, leaving four bytes.  The values stored in this area should be all but the last [sic] return value, ie, we should store a single f32 value.  So we have enough space, but the question is, what does the code actually do?

Drilling down, we get to UnpackResults in WasmInstance.cpp, which calls ToWebAssemblyValue with a type of F32 and mustWrite64=true.  Looking at the location, as a uint32_t array, the second element is definitely the tls pointer, and it will be clobbered.

So why is mustWrite64==true?  This is because, up in UnpackResults, result_size==8.  The reason a float is 8 bytes is because on x86, a float is pushed as 8 bytes (see comments in ABIResult in WasmStubs.h).  Ergo the problem here is that the result pointer is incorrectly computed, it should make more space.  But is the problem that the area set aside for results is too small, or is the pointer computation off?

It looks like the area computation is off.  It gets its slot sizes from ToMIRType which will return 4 for float on x86.  At the end of the computation in passStackResultAreaCallArg in WasmIonCompile.cpp, stackResultArea->byteSize() == 4, which is too small.  It looks like this is specific to Ion; Baseline uses a different mechanism based on the ABIResultIter and its stackBytesConsumedSoFar() mechanism.
The crash location is the third instruction in the stub for the second division:
```
   0x2418c930:	push   %ebp
   0x2418c931:	mov    %esp,%ebp
=> 0x2418c933:	mov    0x10(%esi),%ecx
   0x2418c936:	mov    0xdc(%ecx),%ecx
   0x2418c93c:	movl   $0x41,0x40(%ecx)
   0x2418c943:	or     $0x1,%ebp
   ...
```
$esi is the TLS register.  Looking back up in the calling function, we find that it is read from the top of the frame, right above the area that is used for the multi-value return.  Every indication is that the multi-value return scribbles over the saved TLS in the frame.  In the frame (when no space has been allocated for outgoing args and no words have been pushed temporarily) the TLS is at sp+24.  The return area address is computed as sp+20, leaving four bytes.  The values stored in this area should be all but the last [sic] return value, ie, we should store a single f32 value.  So we have enough space, but the question is, what does the code actually do?

Drilling down, we get to UnpackResults in WasmInstance.cpp, which calls ToWebAssemblyValue with a type of F32 and mustWrite64=true.  Looking at the location, as a uint32_t array, the second element is definitely the tls pointer, and it will be clobbered.

So why is mustWrite64==true?  This is because, up in UnpackResults, result_size==8.  The reason a float is 8 bytes is because on x86, a float is pushed as 8 bytes (see comments in ABIResult in WasmStubs.h).  Ergo the problem here is that the result pointer is incorrectly computed, it should make more space.  But is the problem that the area set aside for results is too small, or is the pointer computation off?

It looks like the area size computation is off.  It gets its slot sizes from ToMIRType which will return 4 for float on x86.  At the end of the computation in passStackResultAreaCallArg in WasmIonCompile.cpp, stackResultArea->byteSize() == 4, which is too small.  It looks like this is specific to Ion; Baseline uses a different mechanism based on the ABIResultIter and its stackBytesConsumedSoFar() mechanism.
The crash location is the third instruction in the stub for the second division:
```
   0x2418c930:	push   %ebp
   0x2418c931:	mov    %esp,%ebp
=> 0x2418c933:	mov    0x10(%esi),%ecx
   0x2418c936:	mov    0xdc(%ecx),%ecx
   0x2418c93c:	movl   $0x41,0x40(%ecx)
   0x2418c943:	or     $0x1,%ebp
   ...
```
$esi is the TLS register.  Looking back up in the calling function, we find that it is read from the top of the frame, right above the area that is used for the multi-value return.  Every indication is that the multi-value return scribbles over the saved TLS in the frame.  In the frame (when no space has been allocated for outgoing args and no words have been pushed temporarily) the TLS is at sp+24.  The return area address is computed as sp+20, leaving four bytes.  The values stored in this area should be all but the last [sic] return value, ie, we should store a single f32 value.  So we have enough space, but the question is, what does the code actually do?

Drilling down, we get to UnpackResults in WasmInstance.cpp, which calls ToWebAssemblyValue with a type of F32 and mustWrite64=true.  Looking at the location, as a uint32_t array, the second element is definitely the tls pointer, and it will be clobbered.

So why is mustWrite64==true?  This is because, up in UnpackResults, result_size==8.  The reason a float is 8 bytes is because on x86, a float is pushed as 8 bytes (see comments in ABIResult in WasmStubs.h).  Ergo the problem here is that the result pointer is incorrectly computed, it should make more space.  But is the problem that the area set aside for results is too small, or is the pointer computation off?

It looks like the area size computation is off.  It gets its slot sizes from MIRTypeToSize which will return 4 for float on x86.  At the end of the computation in passStackResultAreaCallArg in WasmIonCompile.cpp, stackResultArea->byteSize() == 4, which is too small.  It looks like this is specific to Ion; Baseline uses a different mechanism based on the ABIResultIter and its stackBytesConsumedSoFar() mechanism.

Back to Bug 1700690 Comment 5