225978 - Support XPTCALL for Windows XP 64 bits for AMD64 (x86-64)

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Makoto Kato [:m_kato]]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.2; Win64; AMD64)
Build Identifier: 

Currently code tree cannot build for Windows XP 64bits for AMD64.

Reproducible: Always

Steps to Reproduce:
Not need.  Because this is build issue.
Actual Results:  
Cannot build on AMD64

Expected Results:  
Can build on Windows XP for AMD64

Comment 1

[Makoto Kato [:m_kato]]

Attachment: xptcstubs_asm_x86_64.asm

Comment 2

[Makoto Kato [:m_kato]]

Attachment: xptcinvoke_asm_x86_64.asm

Comment 3

[Makoto Kato [:m_kato]]

Attachment: xptcinvoke_x86_64.cpp

Comment 4

[Makoto Kato [:m_kato]]

Attachment: xptcstubs_x86_64.cpp

Comment 5

[Makoto Kato [:m_kato]]

Attachment: makefile diff

Comment 6

[Matthias Versen [:Matti]]

Dbradley : Could you review this patches or Do you have no time for that ?

Comment 7

[David Bradley]

I can review, but it may take a few days. If you're in a hurry might want to
find someone else familiar with assembler.

Comment 8

[Matthias Versen [:Matti]]

Comment on attachment 135722 [details] [diff] [review]
xptcstubs_asm_x86_64.asm

I don't know someone who could review this: 
You got the job :-)

Comment 9

[Alessandro Minelli]

(In reply to comment #7)
> I can review, but it may take a few days. If you're in a hurry might want to
> find someone else familiar with assembler.

Hi, did you review the patch? More than 3 months I think are enough to review :-)

Comment 10

[Doug Turner (:dougt)]

asm reviewers that have time?

Comment 11

[Makoto Kato [:m_kato]]

Attachment: xptcstubs_asm_x86_64.asm @ 2004/2/24

Comment 12

[Makoto Kato [:m_kato]]

Attachment: xptcstubs_x86_64.cpp @ 2004/2/24

Comment 13

[Makoto Kato [:m_kato]]

Attachment: xptcinvoke_asm_x86_64.asm

Comment 14

[timeless]

Comment on attachment 142092 [details]
xptcstubs_asm_x86_64.asm @ 2004/2/24

>;  Version: NPL 1.1/GPL 2.0/LGPL 2.1
what's the history of this file? is it really derived from an NPL file?

>;  The Initial Developer of the Original Code is 
>;  Netscape Communications Corporation.
what's the history of this file? is it really derived from a Netscape file?

>;  Portions created by the Initial Developer are Copyright (C) 1999
>;  the Initial Developer. All Rights Reserved.
what's the history of this file? is it really derived from a file written in
1999?

>   ; store 4 paramters to stack
parameters

>   ; fist 3 parameter is passed by register.
first
which register? what do you mean by 'by', do you mean 'in'?
>   ; for other value
>    ;
is there any reason for the change in indentation?

>    ; 1st paramter (this) (not need re-assign)
parameter

>    ; Uninitialize this function
what does this mean?

>?Stub147@nsXPTCStubBase@@UEAAIXZ PROC EXPORT
>    push    rbx
>    mov     rbx, 147
>    jmp     SharedStub
>?Stub147@nsXPTCStubBase@@UEAAIXZ ENDP
there's no way to macroize this ^^? :(

Comment 15

[timeless]

Comment on attachment 142093 [details]
xptcstubs_x86_64.cpp @ 2004/2/24

>        case nsXPTType::T_U16:
>            if (iCount < PARAM_GPR_COUNT)
>               dp->val.u16  = (PRUint16)gprData[iCount++];
indentation magically changed ^v (the preceding is wrong)
>            else
>                dp->val.u16  = *((PRUint16*)ap++);
>            break;
...
indentation magically changed (again):
>        case nsXPTType::T_DOUBLE:
>              if (iCount < PARAM_FPR_COUNT)
>                 dp->val.d  = (double)fprData[iCount++];
>              else
>                 dp->val.d  = *((double*)ap++);
>              break;

Comment 16

[timeless]

Comment on attachment 142094 [details]
xptcinvoke_asm_x86_64.asm

>   ; store register paramter
parameter

Blah, i'm not going to complain about it each time, but you use "paramter" many
times in this file.

>    ; calcurate call address
calculate

>    ; So, make return address pool!
what does this mean?

>    ; back registers
restore?

Comment 17

[David Bradley]

Can you summarize what's going on here? My Intel assembler is a little out off
date. 1st param of what? I assume the vtable call. I'm more interested in xmm#
register usage.

    mov     rdx, [rsp]           ; 1st param
    movsd   xmm1, qword ptr [rsp] ; for double

    mov     r8, [rsp+8]          ; 2nd param
    movsd   xmm2, qword ptr [rsp+8] ; for double

    mov     r9, [rsp+16]         ; 3rd param
    movsd   xmm3, qword ptr [rsp+16] ; for double

Comment 18

[Makoto Kato [:m_kato]]

David, floating parameters on AMD64 uses SSE2 register.  You can find it on 
AMD64 spec.

Comment 19

[Michael Moy]

xmm registers are 128-bit registers added by Intel with their SSE instructions circa Pentium 3. Intel has extended them with SSE2 instructions (some FP and lots of integer instructions).

There are vector instructions (usually referred to as Packed) and scalar instructions which operate on the datatype of the instruction using the low-order bits of the xmm register.

I didn't know that they were being used as the FP registers and I'll need to check this. AMD added an additional 8 128-bit registers in A64/Opteron and I
was hoping that they would be available for general use.

It may also be that the compiler assumes that these SIMD registers are available
in the architecture and makes use of them. These can be routinely seen if you
compile in 32-bit windows with msvc++ and arch:SSE or arch:SSE2.

(In reply to comment #17)
> Can you summarize what's going on here? My Intel assembler is a little out off
> date. 1st param of what? I assume the vtable call. I'm more interested in xmm#
> register usage.
> 
>     mov     rdx, [rsp]           ; 1st param
>     movsd   xmm1, qword ptr [rsp] ; for double
> 
>     mov     r8, [rsp+8]          ; 2nd param
>     movsd   xmm2, qword ptr [rsp+8] ; for double
> 
>     mov     r9, [rsp+16]         ; 3rd param
>     movsd   xmm3, qword ptr [rsp+16] ; for double
> 
> 

Comment 20

[Michael Moy]

(In reply to comment #18)
> David, floating parameters on AMD64 uses SSE2 register.  You can find it on 
> AMD64 spec.

The diagram below shows that the MMX area is used for floating point operations
as they are in the legacy x86 chips. But the 128-bit media registers can also
be used for scalar and vector floating point operations.

I would have expected that GPRs could hold FP data as well now that they are
64 bit but this doesn't appear to be the case.

http://www.devx.com/amd/Article/17962/2046?supportItem=3

Comment 21

[Makoto Kato [:m_kato]]

(In reply to comment #14)
> (From update of attachment 142092 [details])
> >;  Version: NPL 1.1/GPL 2.0/LGPL 2.1
> what's the history of this file? is it really derived from an NPL file?

Timeless, This is from xptcinvoke.cpp.  Please tell me where valid licence block???

Comment 22

[timeless]

normally new files would get something like
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

i'm guessing that you just copied the license block and that the code is all
yours, am i correct?

Comment 23

[Makoto Kato [:m_kato]]

Attachment: Patch at 2004/12/11

Comment 24

[Makoto Kato [:m_kato]]

Attachment: patch at 1/8/2005

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 170568 [details] [diff] [review]
patch at 1/8/2005

rs=shaver for well-isolated changes to a port that doesn't really work right
now, but dbradley's love is required still.

Comment 26

[Makoto Kato [:m_kato]]

checked in

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Why was this checked in without dbradley's review?  Please back it out ASAP,
unless you can get very fast post-facto review from him.

Comment 28

[David Bradley]

Comment on attachment 170568 [details] [diff] [review]
patch at 1/8/2005

r=dbradley

I haven't had a chance to thoroughly review it yet, but given it's isolated I'm
ok with leaving it in. I'll review in within the next week in more detail and
you can fix anything then.

Comment 29

[Edward Lam]

Any chance of these changes committed into the 1.8 branch soon?

Comment 30

[melissa]

Everyone should be aware that assembly language is *extremely strict* in Win64.  You must declare *every* function into a runtime-visible table, and must declare a function's prolog and epilog correctly using your assembler's features.  You *cannot* have a function with a nonstandard stack layout.  Failure to abide by these rules will result in incorrect exception handling, both SEH and C++.

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Probably not much chance of it being committed to the 1.8 branch if nobody requests such approval.  If you want to do so, please indicate what tests have been performed (on a 1.8-branch tree, natch) to validate the code that you want landed, when you're making your approval request.

Comment 32

[timeless]

Comment on attachment 170568 [details] [diff] [review]
patch at 1/8/2005

mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke_asm_x86_64.asm 	1.1
mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke_asm_x86_64.asm 	1.2
mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke_x86_64.cpp 	1.1
mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs_asm_x86_64.asm 	1.1
mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs_asm_x86_64.asm 	1.2
mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs_x86_64.cpp 	1.1
mozilla/xpcom/reflect/xptcall/src/md/win32/Makefile.in 	1.8
mozilla/xpcom/reflect/xptcall/src/md/win32/Makefile.in 	1.9

As module owner, I'd like to take this opportunity to thank the patch author for *IGNORING* my comments. Which were present in this bug for essentially 11 months. This is not the way to do business.

It's not my job to clean up after your patches, and you have no right to ignore comments. It's not like you didn't have notice or time.

That said, it seems like I ended up wasting my time cleaning up after your mess. I do not appreciate having to do this.

Comment 33

[timeless]

we've been getting bogus bug reports from confused reporters who think they have official mozilla.org 64 bit builds. So I'm going to assume this bug is fixed to their satisfaction.

In the future, I expect people to actually follow procedure which involves *listening* to review comments. Including anyone's drive bys.

I can be a nice module owner and will establish flexible commit rules, but if there are drive by comments, I expect them to be addressed.