1429650 - mips64-linux xpconnect xpcom

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[qiaopengcheng]

User Agent: Mozilla/5.0 (X11; Linux mips64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20171220172151

Steps to reproduce:

After compiled firefox on mips64-linux, run the xpcshell-test like :
./mach xpcshell-test js/xpconnect/tests/unit/test_attribute.js
./mach xpcshell-test js/xpconnect/tests/unit/test_params.js


Actual results:

tests failed.


Expected results:

tests should pass.

Comment 1

[qiaopengcheng]

Attachment: mips64-linux PrepareAndDispatch xpcshell-test js connect

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8942550 [details] [diff] [review]
mips64-linux PrepareAndDispatch xpcshell-test  js connect

I am probably not the right reviewer for this. sorry!

Comment 3

[Petr Sumbera]

Comment on attachment 8942550 [details] [diff] [review]
mips64-linux PrepareAndDispatch xpcshell-test  js connect

I think I'm not right person for the review too. Try to select base on:
https://wiki.mozilla.org/Modules/All

The fix probably works for you. But not very nice in general. It's hard to understand what is going on. Some more description in the bug would be nice too.

Comment 4

[qiaopengcheng]

(In reply to Petr Sumbera from comment #3)
> Comment on attachment 8942550 [details] [diff] [review]
> mips64-linux PrepareAndDispatch xpcshell-test  js connect
> 
> I think I'm not right person for the review too. Try to select base on:
> https://wiki.mozilla.org/Modules/All
> 
> The fix probably works for you. But not very nice in general. It's hard to
> understand what is going on. Some more description in the bug would be nice
> too.


Ok, thank a lot.

This module is cross platform, each platform, like x86, x86_64, arm, mips32/64, power, has its own implementation but for same function, saving the corresponding data-format to the stack for transinformation. 
This bug is only fit for mips64-linux.

Like x86_64 platform  implementation  is right,
xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp     ----> Line85,
xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp     ----> Line80,

Especially for single-float-point data, if saving the single-float-point to a 64bit-space (in fact is a union fitting for all data structure), must be saved without conversion.

Here for mips64-linux platform implementation, 
xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp   ----> Line129,   " dp->val.f  = (double)fprData[iCount++]; "

The value of fprData[iCount]  is 64bit space but only low 32bit saving the single-float-point data, 

" dp->val.f  = (double)fprData[iCount++]; "  will load a 64bit double data from the memory into a cpu-register, and then converting the double-date to a single-float and saving single-float to dp->val.f. 
While the value of fprData[iCount]  is 64bit space but only low 32bit saving the single-float-point data, loading fprData[iCount] as a double-float data is error!   And converted result is to single-float is unpredictable.


Mips64-linux should load the corresponding data-format from the memory and saving it to dp->val.f without conversion like x86_64 implementation.

Comment 5

[hev [:hev]]

Why don't use the implementation same as x86_64?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8942550 [details] [diff] [review]
mips64-linux PrepareAndDispatch xpcshell-test  js connect

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

If this works for you, it works for you, but I'd really like to know what endianness you're testing with and whether you're assuming things in this code that are not always true.  The 32-bit MIPS code, at least, has checks for endianness, and it seems pretty likely that this should too...

::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
@@ +132,2 @@
>                else
> +                  dp->val.f  = *((float*)ap++);

This only works for little-endian MIPS, right?  Particularly the fprData branch?

Comment 7

[qiaopengcheng]

(In reply to Heiher [:hev] from comment #5)
> Why don't use the implementation same as x86_64?

Of course，the best is like x86_64 implementation, which implemented by c++ code.
But that depends on the compiler, whether supporting the dynamic stack-alloction.
x86-64 gcc is   "uint64_t *stack = (uint64_t *) __builtin_alloca(nr_stack * 8);"  in Line134 within file "xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp".

On mips64-linux platform, the function "NS_InvokeByIndex" was implemented using assemble language in file:
xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S .

Comment 8

[qiaopengcheng]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8942550 [details] [diff] [review]
> mips64-linux PrepareAndDispatch xpcshell-test  js connect
> 
> Review of attachment 8942550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If this works for you, it works for you, but I'd really like to know what
> endianness you're testing with and whether you're assuming things in this
> code that are not always true.  The 32-bit MIPS code, at least, has checks
> for endianness, and it seems pretty likely that this should too...


This file  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp" is only used for mips64-linux, like the filename.

Our mips64-computer is little-endian.



> ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> @@ +132,2 @@
> >                else
> > +                  dp->val.f  = *((float*)ap++);
> 
> This only works for little-endian MIPS, right?  Particularly the fprData
> branch?

You are right！ for little-endian that is ok.
If I write this patch  like this  "dp->val.d  = *((double*)ap++);".
The float-points are all using double, it will be right ?

Comment 9

[qiaopengcheng]

(In reply to Nathan Froyd [:froydnj] from comment #6)

> ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> @@ +132,2 @@
> >                else
> > +                  dp->val.f  = *((float*)ap++);
> 
> This only works for little-endian MIPS, right?  Particularly the fprData
> branch?

Sorry, I forgot the right answer.
This works for little-endian MIPS64 and also big-endian MIPS64.
Because single-float-point data had been saved in *((float*)ap).
Here the function "PrepareAndDispatch" is shard for different data-types, the ap is just a pointer and copying the data verbatim. It does't matter the spcific format-data saved within.

Comment 10

[hev [:hev]]

(In reply to qiaopengcheng from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> 
> > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > @@ +132,2 @@
> > >                else
> > > +                  dp->val.f  = *((float*)ap++);
> > 
> > This only works for little-endian MIPS, right?  Particularly the fprData
> > branch?
> 
> Sorry, I forgot the right answer.
> This works for little-endian MIPS64 and also big-endian MIPS64.
> Because single-float-point data had been saved in *((float*)ap).
> Here the function "PrepareAndDispatch" is shard for different data-types,
> the ap is just a pointer and copying the data verbatim. It does't matter the
> spcific format-data saved within.

IIRC, The args of PrepareAndDispatch is point to 9th argument address of nsXPTStubs on stack.
If the 9th argument is float type, how much bytes allocated on stack for? 4-byte? or 8-byte?

I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?

void test(double a, double b, double c, double d, double e, double f, double g, double h, float i, double j);

first 8 arguments on fpu registers:
a: f12
b: f13
c: f14
d: f15
e: f16
f: f17
g: f18
h: f19

last 2 arguments on stack:
for little-endian:
sp + 0: i[7:0]
sp + 1: i[15:8]
sp + 2: i[23:16]
sp + 3: i[31:24]
sp + 4: [random]
sp + 5: ..
sp + 6: ..
sp + 7: ..
sp + 8: j[7:0]
sp + 9: j[15:8]
sp + a: j[23:16]
sp + b: j[31:24]
sp + c: j[39:32]
sp + d: j[47:40]
sp + e: j[55:48]
sp + f: j[63:56]

for big-endian:
sp + 0: [random]
sp + 1: ..
sp + 2: ..
sp + 3: ..
sp + 4: i[31:24]
sp + 5: i[23:16]
sp + 6: i[15:8]
sp + 7: i[7:0]
sp + 8: j[63:56]
sp + 9: j[55:48]
sp + a: j[47:40]
sp + b: j[39:32]
sp + c: j[31:24]
sp + d: j[23:16]
sp + e: j[15:8]
sp + f: j[7:0]

Comment 11

[qiaopengcheng]

(In reply to Heiher [:hev] from comment #10)
> (In reply to qiaopengcheng from comment #9)
> > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > 
> > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > @@ +132,2 @@
> > > >                else
> > > > +                  dp->val.f  = *((float*)ap++);
> > > 
> > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > branch?
> > 
> > Sorry, I forgot the right answer.
> > This works for little-endian MIPS64 and also big-endian MIPS64.
> > Because single-float-point data had been saved in *((float*)ap).
> > Here the function "PrepareAndDispatch" is shard for different data-types,
> > the ap is just a pointer and copying the data verbatim. It does't matter the
> > spcific format-data saved within.
> 
> IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> nsXPTStubs on stack.
> If the 9th argument is float type, how much bytes allocated on stack for?
> 4-byte? or 8-byte?
> 
> I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> 
> void test(double a, double b, double c, double d, double e, double f, double
> g, double h, float i, double j);
> 
> first 8 arguments on fpu registers:
> a: f12
> b: f13
> c: f14
> d: f15
> e: f16
> f: f17
> g: f18
> h: f19
> 
> last 2 arguments on stack:
> for little-endian:
> sp + 0: i[7:0]
> sp + 1: i[15:8]
> sp + 2: i[23:16]
> sp + 3: i[31:24]
> sp + 4: [random]
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> sp + 8: j[7:0]
> sp + 9: j[15:8]
> sp + a: j[23:16]
> sp + b: j[31:24]
> sp + c: j[39:32]
> sp + d: j[47:40]
> sp + e: j[55:48]
> sp + f: j[63:56]
> 
> for big-endian:
> sp + 0: [random]
> sp + 1: ..
> sp + 2: ..
> sp + 3: ..
> sp + 4: i[31:24]
> sp + 5: i[23:16]
> sp + 6: i[15:8]
> sp + 7: i[7:0]
> sp + 8: j[63:56]
> sp + 9: j[55:48]
> sp + a: j[47:40]
> sp + b: j[39:32]
> sp + c: j[31:24]
> sp + d: j[23:16]
> sp + e: j[15:8]
> sp + f: j[7:0]



I agree with you for your example.
But your example is not same with this patch's context.

Like the context in  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp",
if I saved a single-float-point-data to double i like this   "*((float*)&i) = float value;"

In fact, the function PrepareAndDispatch's args are pointers, the value saved within the memory like the union-data-type.

Comment 12

[hev [:hev]]

(In reply to qiaopengcheng from comment #11)
> (In reply to Heiher [:hev] from comment #10)
> > (In reply to qiaopengcheng from comment #9)
> > > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > 
> > > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > > @@ +132,2 @@
> > > > >                else
> > > > > +                  dp->val.f  = *((float*)ap++);
> > > > 
> > > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > > branch?
> > > 
> > > Sorry, I forgot the right answer.
> > > This works for little-endian MIPS64 and also big-endian MIPS64.
> > > Because single-float-point data had been saved in *((float*)ap).
> > > Here the function "PrepareAndDispatch" is shard for different data-types,
> > > the ap is just a pointer and copying the data verbatim. It does't matter the
> > > spcific format-data saved within.
> > 
> > IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> > nsXPTStubs on stack.
> > If the 9th argument is float type, how much bytes allocated on stack for?
> > 4-byte? or 8-byte?
> > 
> > I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> > 
> > void test(double a, double b, double c, double d, double e, double f, double
> > g, double h, float i, double j);
> > 
> > first 8 arguments on fpu registers:
> > a: f12
> > b: f13
> > c: f14
> > d: f15
> > e: f16
> > f: f17
> > g: f18
> > h: f19
> > 
> > last 2 arguments on stack:
> > for little-endian:
> > sp + 0: i[7:0]
> > sp + 1: i[15:8]
> > sp + 2: i[23:16]
> > sp + 3: i[31:24]
> > sp + 4: [random]
> > sp + 5: ..
> > sp + 6: ..
> > sp + 7: ..
> > sp + 8: j[7:0]
> > sp + 9: j[15:8]
> > sp + a: j[23:16]
> > sp + b: j[31:24]
> > sp + c: j[39:32]
> > sp + d: j[47:40]
> > sp + e: j[55:48]
> > sp + f: j[63:56]
> > 
> > for big-endian:
> > sp + 0: [random]
> > sp + 1: ..
> > sp + 2: ..
> > sp + 3: ..
> > sp + 4: i[31:24]
> > sp + 5: i[23:16]
> > sp + 6: i[15:8]
> > sp + 7: i[7:0]
> > sp + 8: j[63:56]
> > sp + 9: j[55:48]
> > sp + a: j[47:40]
> > sp + b: j[39:32]
> > sp + c: j[31:24]
> > sp + d: j[23:16]
> > sp + e: j[15:8]
> > sp + f: j[7:0]
> 
> 
> 
> I agree with you for your example.
> But your example is not same with this patch's context.
> 
> Like the context in  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp",
> if I saved a single-float-point-data to double i like this   "*((float*)&i)
> = float value;"
> 
> In fact, the function PrepareAndDispatch's args are pointers, the value
> saved within the memory like the union-data-type.

(In reply to Heiher [:hev] from comment #10)
> (In reply to qiaopengcheng from comment #9)
> > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > 
> > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > @@ +132,2 @@
> > > >                else
> > > > +                  dp->val.f  = *((float*)ap++);
> > > 
> > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > branch?
> > 
> > Sorry, I forgot the right answer.
> > This works for little-endian MIPS64 and also big-endian MIPS64.
> > Because single-float-point data had been saved in *((float*)ap).
> > Here the function "PrepareAndDispatch" is shard for different data-types,
> > the ap is just a pointer and copying the data verbatim. It does't matter the
> > spcific format-data saved within.
> 
> IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> nsXPTStubs on stack.
> If the 9th argument is float type, how much bytes allocated on stack for?
> 4-byte? or 8-byte?
> 
> I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> 
> void test(double a, double b, double c, double d, double e, double f, double
> g, double h, float i, double j);
> 
> first 8 arguments on fpu registers:
> a: f12
> b: f13
> c: f14
> d: f15
> e: f16
> f: f17
> g: f18
> h: f19
> 
> last 2 arguments on stack:
> for little-endian:
> sp + 0: i[7:0]
> sp + 1: i[15:8]
> sp + 2: i[23:16]
> sp + 3: i[31:24]
> sp + 4: [random]
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> sp + 8: j[7:0]
> sp + 9: j[15:8]
> sp + a: j[23:16]
> sp + b: j[31:24]
> sp + c: j[39:32]
> sp + d: j[47:40]
> sp + e: j[55:48]
> sp + f: j[63:56]
> 
> for big-endian:
> sp + 0: [random]
> sp + 1: ..
> sp + 2: ..
> sp + 3: ..
> sp + 4: i[31:24]
> sp + 5: i[23:16]
> sp + 6: i[15:8]
> sp + 7: i[7:0]
> sp + 8: j[63:56]
> sp + 9: j[55:48]
> sp + a: j[47:40]
> sp + b: j[39:32]
> sp + c: j[31:24]
> sp + d: j[23:16]
> sp + e: j[15:8]
> sp + f: j[7:0]

Sorry! in fact, the float typed arguments passed on stack for big-endian is:
sp + 0: i[31:24]
sp + 1: i[23:16]
sp + 2: i[15:8]
sp + 3: i[7:0]
sp + 4: [random]
sp + 5: ..
sp + 6: ..
sp + 7: ..
sp + 8: j[63:56]
sp + 9: j[55:48]
sp + a: j[47:40]
sp + b: j[39:32]
sp + c: j[31:24]
sp + d: j[23:16]
sp + e: j[15:8]
sp + f: j[7:0]

Comment 13

[hev [:hev]]

(In reply to qiaopengcheng from comment #11)
> (In reply to Heiher [:hev] from comment #10)
> > (In reply to qiaopengcheng from comment #9)
> > > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > 
> > > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > > @@ +132,2 @@
> > > > >                else
> > > > > +                  dp->val.f  = *((float*)ap++);
> > > > 
> > > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > > branch?
> > > 
> > > Sorry, I forgot the right answer.
> > > This works for little-endian MIPS64 and also big-endian MIPS64.
> > > Because single-float-point data had been saved in *((float*)ap).
> > > Here the function "PrepareAndDispatch" is shard for different data-types,
> > > the ap is just a pointer and copying the data verbatim. It does't matter the
> > > spcific format-data saved within.
> > 
> > IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> > nsXPTStubs on stack.
> > If the 9th argument is float type, how much bytes allocated on stack for?
> > 4-byte? or 8-byte?
> > 
> > I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> > 
> > void test(double a, double b, double c, double d, double e, double f, double
> > g, double h, float i, double j);
> > 
> > first 8 arguments on fpu registers:
> > a: f12
> > b: f13
> > c: f14
> > d: f15
> > e: f16
> > f: f17
> > g: f18
> > h: f19
> > 
> > last 2 arguments on stack:
> > for little-endian:
> > sp + 0: i[7:0]
> > sp + 1: i[15:8]
> > sp + 2: i[23:16]
> > sp + 3: i[31:24]
> > sp + 4: [random]
> > sp + 5: ..
> > sp + 6: ..
> > sp + 7: ..
> > sp + 8: j[7:0]
> > sp + 9: j[15:8]
> > sp + a: j[23:16]
> > sp + b: j[31:24]
> > sp + c: j[39:32]
> > sp + d: j[47:40]
> > sp + e: j[55:48]
> > sp + f: j[63:56]
> > 
> > for big-endian:
> > sp + 0: [random]
> > sp + 1: ..
> > sp + 2: ..
> > sp + 3: ..
> > sp + 4: i[31:24]
> > sp + 5: i[23:16]
> > sp + 6: i[15:8]
> > sp + 7: i[7:0]
> > sp + 8: j[63:56]
> > sp + 9: j[55:48]
> > sp + a: j[47:40]
> > sp + b: j[39:32]
> > sp + c: j[31:24]
> > sp + d: j[23:16]
> > sp + e: j[15:8]
> > sp + f: j[7:0]
> 
> 
> 
> I agree with you for your example.
> But your example is not same with this patch's context.
> 
> Like the context in  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp",
> if I saved a single-float-point-data to double i like this   "*((float*)&i)
> = float value;"
> 
> In fact, the function PrepareAndDispatch's args are pointers, the value
> saved within the memory like the union-data-type.

Looks you are right. The arguments (including registers and stacks) are saved by invoke, not standard mips n64 abi.

how float (9th) passed to stubs from invoke on stack for mips64?

little-endian:
invoke (*(float*)d++ = s->val.f;):
sp + 0: [7:0] // write to
sp + 1: [15:8]
sp + 2: [23:16]
sp + 3: [31:24]
sp + 4: ..
sp + 5: ..
sp + 6: ..
sp + 7: ..

stub (dp->val.f  = *((float*)ap++);):
sp + 0: [7:0]  // read back
sp + 1: [15:8]
sp + 2: [23:16]
sp + 3: [31:24]
sp + 4: ..
sp + 5: ..
sp + 6: ..
sp + 7: ..

big-endian:
invoke (*(float*)d++ = s->val.f;):
sp + 0: [31:24] // write to
sp + 1: [23:16]
sp + 2: [15:8]
sp + 3: [7:0]
sp + 4: ..
sp + 5: ..
sp + 6: ..
sp + 7: ..

stub (dp->val.f  = *((float*)ap++);):
sp + 0: [31:24] // read back
sp + 1: [23:16]
sp + 2: [15:8]
sp + 3: [7:0]
sp + 4: ..
sp + 5: ..
sp + 6: ..
sp + 7: ..

Seems no problem for both endians?

Comment 14

[qiaopengcheng]

I think so only within this context.

This context is very special. The data format is quite special and different platform shared.
Here using the double or uint64_t  type, like union type defined here "nsXPTCMiniVariant".
which defined in file xpcom/reflect/xptcall/xptcall.h  Line18  struct nsXPTCMiniVariant.

Comment 15

[qiaopengcheng]

(In reply to Heiher [:hev] from comment #13)
> (In reply to qiaopengcheng from comment #11)
> > (In reply to Heiher [:hev] from comment #10)
> > > (In reply to qiaopengcheng from comment #9)
> > > > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > > 
> > > > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > > > @@ +132,2 @@
> > > > > >                else
> > > > > > +                  dp->val.f  = *((float*)ap++);
> > > > > 
> > > > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > > > branch?
> > > > 
> > > > Sorry, I forgot the right answer.
> > > > This works for little-endian MIPS64 and also big-endian MIPS64.
> > > > Because single-float-point data had been saved in *((float*)ap).
> > > > Here the function "PrepareAndDispatch" is shard for different data-types,
> > > > the ap is just a pointer and copying the data verbatim. It does't matter the
> > > > spcific format-data saved within.
> > > 
> > > IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> > > nsXPTStubs on stack.
> > > If the 9th argument is float type, how much bytes allocated on stack for?
> > > 4-byte? or 8-byte?
> > > 
> > > I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> > > 
> > > void test(double a, double b, double c, double d, double e, double f, double
> > > g, double h, float i, double j);
> > > 
> > > first 8 arguments on fpu registers:
> > > a: f12
> > > b: f13
> > > c: f14
> > > d: f15
> > > e: f16
> > > f: f17
> > > g: f18
> > > h: f19
> > > 
> > > last 2 arguments on stack:
> > > for little-endian:
> > > sp + 0: i[7:0]
> > > sp + 1: i[15:8]
> > > sp + 2: i[23:16]
> > > sp + 3: i[31:24]
> > > sp + 4: [random]
> > > sp + 5: ..
> > > sp + 6: ..
> > > sp + 7: ..
> > > sp + 8: j[7:0]
> > > sp + 9: j[15:8]
> > > sp + a: j[23:16]
> > > sp + b: j[31:24]
> > > sp + c: j[39:32]
> > > sp + d: j[47:40]
> > > sp + e: j[55:48]
> > > sp + f: j[63:56]
> > > 
> > > for big-endian:
> > > sp + 0: [random]
> > > sp + 1: ..
> > > sp + 2: ..
> > > sp + 3: ..
> > > sp + 4: i[31:24]
> > > sp + 5: i[23:16]
> > > sp + 6: i[15:8]
> > > sp + 7: i[7:0]
> > > sp + 8: j[63:56]
> > > sp + 9: j[55:48]
> > > sp + a: j[47:40]
> > > sp + b: j[39:32]
> > > sp + c: j[31:24]
> > > sp + d: j[23:16]
> > > sp + e: j[15:8]
> > > sp + f: j[7:0]
> > 
> > 
> > 
> > I agree with you for your example.
> > But your example is not same with this patch's context.
> > 
> > Like the context in  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp",
> > if I saved a single-float-point-data to double i like this   "*((float*)&i)
> > = float value;"
> > 
> > In fact, the function PrepareAndDispatch's args are pointers, the value
> > saved within the memory like the union-data-type.
> 
> Looks you are right. The arguments (including registers and stacks) are
> saved by invoke, not standard mips n64 abi.
> 
> how float (9th) passed to stubs from invoke on stack for mips64?
> 
> little-endian:
> invoke (*(float*)d++ = s->val.f;):
> sp + 0: [7:0] // write to
> sp + 1: [15:8]
> sp + 2: [23:16]
> sp + 3: [31:24]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> stub (dp->val.f  = *((float*)ap++);):
> sp + 0: [7:0]  // read back
> sp + 1: [15:8]
> sp + 2: [23:16]
> sp + 3: [31:24]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> big-endian:
> invoke (*(float*)d++ = s->val.f;):
> sp + 0: [31:24] // write to
> sp + 1: [23:16]
> sp + 2: [15:8]
> sp + 3: [7:0]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> stub (dp->val.f  = *((float*)ap++);):
> sp + 0: [31:24] // read back
> sp + 1: [23:16]
> sp + 2: [15:8]
> sp + 3: [7:0]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> Seems no problem for both endians?




I think so only within this context.

This context is very special. The data format is quite special and different platform shared.
Here using the double or uint64_t  type, like union type defined here "nsXPTCMiniVariant".
which defined in file xpcom/reflect/xptcall/xptcall.h  Line18  struct nsXPTCMiniVariant.

Comment 16

[qiaopengcheng]

Here is just a 64-bits value space for saving data as long as no more than 64-bits, it does't matter the type is double, float or uint64_t.

If the data passed is single-float-point, will saving the single-float-point value using single-float-point pointer to the 64bits memory space.

And when loading back the value previous saved from the memory using the corresponding data-type format.
That works well.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8942550 [details] [diff] [review]
mips64-linux PrepareAndDispatch xpcshell-test  js connect

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

I still think this is completely busted for big-endian, but I don't think this is making things any worse, since there's no endianness checking in this code in the first place.

Comment 18

[qiaopengcheng]

(In reply to Heiher [:hev] from comment #13)
> (In reply to qiaopengcheng from comment #11)
> > (In reply to Heiher [:hev] from comment #10)
> > > (In reply to qiaopengcheng from comment #9)
> > > > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > > 
> > > > > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp
> > > > > @@ +132,2 @@
> > > > > >                else
> > > > > > +                  dp->val.f  = *((float*)ap++);
> > > > > 
> > > > > This only works for little-endian MIPS, right?  Particularly the fprData
> > > > > branch?
> > > > 
> > > > Sorry, I forgot the right answer.
> > > > This works for little-endian MIPS64 and also big-endian MIPS64.
> > > > Because single-float-point data had been saved in *((float*)ap).
> > > > Here the function "PrepareAndDispatch" is shard for different data-types,
> > > > the ap is just a pointer and copying the data verbatim. It does't matter the
> > > > spcific format-data saved within.
> > > 
> > > IIRC, The args of PrepareAndDispatch is point to 9th argument address of
> > > nsXPTStubs on stack.
> > > If the 9th argument is float type, how much bytes allocated on stack for?
> > > 4-byte? or 8-byte?
> > > 
> > > I think is 8-byte and how 9 arguments passed on mips n64 abi, is correctly?
> > > 
> > > void test(double a, double b, double c, double d, double e, double f, double
> > > g, double h, float i, double j);
> > > 
> > > first 8 arguments on fpu registers:
> > > a: f12
> > > b: f13
> > > c: f14
> > > d: f15
> > > e: f16
> > > f: f17
> > > g: f18
> > > h: f19
> > > 
> > > last 2 arguments on stack:
> > > for little-endian:
> > > sp + 0: i[7:0]
> > > sp + 1: i[15:8]
> > > sp + 2: i[23:16]
> > > sp + 3: i[31:24]
> > > sp + 4: [random]
> > > sp + 5: ..
> > > sp + 6: ..
> > > sp + 7: ..
> > > sp + 8: j[7:0]
> > > sp + 9: j[15:8]
> > > sp + a: j[23:16]
> > > sp + b: j[31:24]
> > > sp + c: j[39:32]
> > > sp + d: j[47:40]
> > > sp + e: j[55:48]
> > > sp + f: j[63:56]
> > > 
> > > for big-endian:
> > > sp + 0: [random]
> > > sp + 1: ..
> > > sp + 2: ..
> > > sp + 3: ..
> > > sp + 4: i[31:24]
> > > sp + 5: i[23:16]
> > > sp + 6: i[15:8]
> > > sp + 7: i[7:0]
> > > sp + 8: j[63:56]
> > > sp + 9: j[55:48]
> > > sp + a: j[47:40]
> > > sp + b: j[39:32]
> > > sp + c: j[31:24]
> > > sp + d: j[23:16]
> > > sp + e: j[15:8]
> > > sp + f: j[7:0]
> > 
> > 
> > 
> > I agree with you for your example.
> > But your example is not same with this patch's context.
> > 
> > Like the context in  "xpcom/reflect/xptcall/md/unix/xptcstubs_mips64.cpp",
> > if I saved a single-float-point-data to double i like this   "*((float*)&i)
> > = float value;"
> > 
> > In fact, the function PrepareAndDispatch's args are pointers, the value
> > saved within the memory like the union-data-type.
> 
> Looks you are right. The arguments (including registers and stacks) are
> saved by invoke, not standard mips n64 abi.
> 
> how float (9th) passed to stubs from invoke on stack for mips64?
> 
> little-endian:
> invoke (*(float*)d++ = s->val.f;):
> sp + 0: [7:0] // write to
> sp + 1: [15:8]
> sp + 2: [23:16]
> sp + 3: [31:24]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> stub (dp->val.f  = *((float*)ap++);):
> sp + 0: [7:0]  // read back
> sp + 1: [15:8]
> sp + 2: [23:16]
> sp + 3: [31:24]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> big-endian:
> invoke (*(float*)d++ = s->val.f;):
> sp + 0: [31:24] // write to
> sp + 1: [23:16]
> sp + 2: [15:8]
> sp + 3: [7:0]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> stub (dp->val.f  = *((float*)ap++);):
> sp + 0: [31:24] // read back
> sp + 1: [23:16]
> sp + 2: [15:8]
> sp + 3: [7:0]
> sp + 4: ..
> sp + 5: ..
> sp + 6: ..
> sp + 7: ..
> 
> Seems no problem for both endians?



Maybe we can look at the issus from another angle.
Big or little endian  is different if the data-widths is not same, .
For example, there is a 64bits memory space, assuming double pointer "double * d_pointer;".
As long as we saved a float-32bits value to the d_pointer and load value with same type float-32bits,
at this moment， big and little endian are the same.

Now the patch's context is this situation.

Comment 19

[qiaopengcheng]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> Comment on attachment 8942550 [details] [diff] [review]
> mips64-linux PrepareAndDispatch xpcshell-test  js connect
> 
> Review of attachment 8942550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still think this is completely busted for big-endian, but I don't think
> this is making things any worse, since there's no endianness checking in
> this code in the first place.



Thank you！

Maybe we can look at the issus from another angle.
Big or little endian  is different if the data-widths is not same, .
For example, there is a 64bits memory space, assuming double pointer "double * d_pointer;".
As long as we saved a float-32bits value to the d_pointer and load value with same type float-32bits,
at this moment， big and little endian are the same.

Now the patch's context is this situation.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9da3d717ee0
mips64-linux PrepareAndDispatch float-formate data error when running xpcshell-test js/xpconnect/tests/unit/test_attribute.js and test_params.js r=froydnj

Comment 21

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/a9da3d717ee0