Closed Bug 225978 Opened 17 years ago Closed 14 years ago

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

Categories

(Core :: XPCOM, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(10 obsolete files)

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
Attached patch xptcstubs_asm_x86_64.asm (obsolete) — Splinter Review
Attached patch xptcinvoke_asm_x86_64.asm (obsolete) — Splinter Review
Attached patch xptcinvoke_x86_64.cpp (obsolete) — Splinter Review
Attached patch xptcstubs_x86_64.cpp (obsolete) — Splinter Review
Attached patch makefile diff (obsolete) — Splinter Review
Dbradley : Could you review this patches or Do you have no time for that ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 on attachment 135722 [details] [diff] [review]
xptcstubs_asm_x86_64.asm

I don't know someone who could review this: 
You got the job :-)
Attachment #135722 - Flags: review?(BradleyJunk)
(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 :-)
asm reviewers that have time?
Attached file xptcstubs_asm_x86_64.asm @ 2004/2/24 (obsolete) —
Attachment #135722 - Attachment is obsolete: true
Attached file xptcstubs_x86_64.cpp @ 2004/2/24 (obsolete) —
Attachment #135725 - Attachment is obsolete: true
Attachment #142092 - Attachment description: add file @ 2004/2/24 → xptcstubs_asm_x86_64.asm @ 2004/2/24
Attachment #142092 - Attachment is patch: false
Attachment #142093 - Attachment is patch: false
Attached file xptcinvoke_asm_x86_64.asm (obsolete) —
Attachment #135723 - Attachment is obsolete: true
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 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 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?
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

David, floating parameters on AMD64 uses SSE2 register.  You can find it on 
AMD64 spec.
Blocks: 237202
Flags: blocking1.8a?
Flags: blocking1.8a? → blocking1.8a-
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
> 
> 

(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
(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???
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?
Attached patch Patch at 2004/12/11 (obsolete) — Splinter Review
Attachment #135724 - Attachment is obsolete: true
Attachment #135726 - Attachment is obsolete: true
Attachment #142092 - Attachment is obsolete: true
Attachment #142093 - Attachment is obsolete: true
Attachment #142094 - Attachment is obsolete: true
Attached patch patch at 1/8/2005 (obsolete) — Splinter Review
Attachment #168422 - Attachment is obsolete: true
Attachment #170568 - Flags: review?(timeless)
Attachment #135722 - Flags: review?(dbradley)
Attachment #170568 - Flags: superreview?(shaver)
Attachment #170568 - Flags: review?(timeless)
Attachment #170568 - Flags: review?(dbradley)
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.
Attachment #170568 - Flags: superreview?(shaver) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #170568 - Flags: review?(dbradley) → review+
Any chance of these changes committed into the 1.8 branch soon?
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++.
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.
Assignee: dougt → nobody
Status: REOPENED → NEW
QA Contact: xpcom
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.
Attachment #170568 - Attachment is obsolete: true
Assignee: nobody → m_kato
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.
Status: NEW → RESOLVED
Closed: 16 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.