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

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.8a1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 obsolete attachments)

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
Dbradley : Could you review this patches or Do you have no time for that ?
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

16 years ago
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)

Comment 9

15 years ago
(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?
Attachment #142092 - Attachment description: add file @ 2004/2/24 → xptcstubs_asm_x86_64.asm @ 2004/2/24
Attachment #142092 - Attachment is patch: false
Posted file xptcinvoke_asm_x86_64.asm (obsolete) —
Attachment #135723 - Attachment is obsolete: true

Comment 14

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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.

Updated

15 years ago
Blocks: 237202

Updated

15 years ago
Flags: blocking1.8a?

Updated

15 years ago
Flags: blocking1.8a? → blocking1.8a-

Comment 19

15 years ago
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

15 years ago
(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???

Comment 22

15 years ago
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?
Posted 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
Posted patch patch at 1/8/2005 (obsolete) — Splinter Review
Attachment #168422 - Attachment is obsolete: true

Updated

15 years ago
Attachment #135722 - Flags: review?(dbradley)

Updated

15 years ago
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
Last Resolved: 14 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 28

14 years ago
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+

Comment 29

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

Comment 30

13 years ago
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 32

13 years ago
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

Updated

13 years ago
Assignee: nobody → m_kato

Comment 33

13 years ago
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
Last Resolved: 14 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.