Closed
Bug 1455028
Opened 6 years ago
Closed 6 years ago
[MIPS64] Fix jit -> c++ int32 argument passing for simulator builds
Categories
(Core :: JavaScript Engine: JIT, enhancement, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)
References
Details
Attachments
(1 file, 1 obsolete file)
9.71 KB,
patch
|
dragan.mladjenovic
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
While testing Bug 1455019 I discovered that changes I introduced in Bug 1453628 would produce incorrect result on simulator / c++ boundary for functions that have for example void* (*)(double) signature whilst their abi signature value being Args_Int_Double because Int in this context actually labels general purpose register. To make things simple this patch reverts changes done by previous one and does explicit result value coercion for known external function that requires it.
Assignee: nobody → dragan.mladjenovic
Attachment #8968983 -
Flags: review?(bbouvier)
Updated•6 years ago
|
Priority: -- → P5
Comment 3•6 years ago
|
||
Comment on attachment 8968983 [details] [diff] [review] bug1455028.patch Review of attachment 8968983 [details] [diff] [review]: ----------------------------------------------------------------- It's weird. I think there should be a clear cutoff between General and Int in the Prototype_ typedefs family. In the example you mention, could it alternatively be solved by using the more precise return type, viz. General, when a function returns a pointer type, instead of Int? Or are the explicit coercions needed because the C++ compiler doesn't clear the high 32 bits when returning int32 values? (waiting on answers before r+) ::: js/src/jit/mips64/Simulator-mips64.cpp @@ +2242,5 @@ > case Args_Int_Double: { > double dval0 = getFpuRegisterDouble(12); > Prototype_Int_Double target = reinterpret_cast<Prototype_Int_Double>(external); > + int64_t result = target(dval0); > + if(external == intptr_t((int32_t(*)(double))JS::ToInt32)) { nit: here and below: space before opening parenthesis, no need for {} for single line if-bodies Can the explicit function type coercion be removed? Seems that a compiler might have all the information to convert directly to intptr_t, but, oh well..
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #3) > Comment on attachment 8968983 [details] [diff] [review] > bug1455028.patch > > Review of attachment 8968983 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's weird. I think there should be a clear cutoff between General and Int > in the Prototype_ typedefs family. In the example you mention, could it > alternatively be solved by using the more precise return type, viz. General, > when a function returns a pointer type, instead of Int? > > Or are the explicit coercions needed because the C++ compiler doesn't clear > the high 32 bits when returning int32 values? > Yes that is correct. The issue would be solved by using more precise return type. In fact compiler does clear high 32 bits (not sure if you can rely on it to do so), but we are expecting them to be sign extension of lower 32 bits. By looking into FunctionInfo source (https://searchfox.org/mozilla-central/search?q=symbol:T_js%3A%3Ajit%3A%3ATypeToDataType&redirect=false) it seems that that path of calling into C++ does not expect int32 as return value, so the only offenders would be wasm builtin calls(for now). The whole thing with AbiArgs could be cleaned up. At lest "Int" could be replaced with "General" to avoid further confusion. I'm not sure what is the scope of changes needed to disambiguate between int32 return value and the pointer/boolean/.. ones, so I took the path of least resistance with this manual coercion.
Comment 5•6 years ago
|
||
Comment on attachment 8968983 [details] [diff] [review] bug1455028.patch Review of attachment 8968983 [details] [diff] [review]: ----------------------------------------------------------------- Alright, thanks for your answer. r=me with previous comments addressed. Thanks!
Attachment #8968983 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8968983 -
Attachment is obsolete: true
Attachment #8970829 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/64b83e067db2 [MIPS64] Fix jit -> c++ int32 argument passing for simulator builds; r=bbouvier
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/53422b422071 [MIPS64] Fix jit -> c++ int32 argument passing for simulator builds. r=bbouvier
Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6e69352a940e Backed out 3 changesets (bug 1455028, bug 1455019, bug 1455016) because they were already pushed to inbound. DONTBUILD
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/64b83e067db2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•