Closed
Bug 1429650
Opened 8 years ago
Closed 7 years ago
mips64-linux xpconnect xpcom
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: qiaopengcheng-hf, Assigned: qiaopengcheng-hf)
Details
Attachments
(1 file)
1.73 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8942550 -
Flags: review?(petr.sumbera)
Assignee | ||
Updated•7 years ago
|
Attachment #8942550 -
Flags: review?(sledru)
Comment 2•7 years ago
|
||
Comment on attachment 8942550 [details] [diff] [review]
mips64-linux PrepareAndDispatch xpcshell-test js connect
I am probably not the right reviewer for this. sorry!
Attachment #8942550 -
Flags: review?(sledru)
Comment 3•7 years ago
|
||
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.
Attachment #8942550 -
Flags: review?(petr.sumbera)
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8942550 -
Flags: review?(nfroyd)
Attachment #8942550 -
Flags: review?(erahm)
Comment 5•7 years ago
|
||
Why don't use the implementation same as x86_64?
Flags: needinfo?(qiaopengcheng-hf)
![]() |
||
Comment 6•7 years ago
|
||
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?
Attachment #8942550 -
Flags: review?(nfroyd)
Attachment #8942550 -
Flags: review?(erahm)
Updated•7 years ago
|
Assignee: nobody → qiaopengcheng-hf
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → XPCOM
Ever confirmed: true
Product: Firefox → Core
Assignee | ||
Comment 7•7 years ago
|
||
(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 .
Flags: needinfo?(qiaopengcheng-hf)
Assignee | ||
Comment 8•7 years ago
|
||
(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 ?
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
(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]
Assignee | ||
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
(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?
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
(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.
Assignee | ||
Comment 16•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8942550 -
Flags: review?(nfroyd)
![]() |
||
Comment 17•7 years ago
|
||
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.
Attachment #8942550 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 18•7 years ago
|
||
(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.
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 20•7 years ago
|
||
Pushed by r@hev.cc:
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
Keywords: checkin-needed
Comment 21•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•