Closed Bug 225978 Opened 19 years ago Closed 16 years ago
Support XPTCALL for Windows XP 64 bits for AMD64 (x86-64)
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
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?
Attachment #142093 - Attachment is patch: false
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.
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?
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+
Status: NEW → RESOLVED
Closed: 18 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
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: 18 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.