Closed
Bug 326838
Opened 18 years ago
Closed 18 years ago
[BeOS] Build broken due to fix for 313398
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: doug, Assigned: mark)
References
Details
(Keywords: fixed1.8.0.2, fixed1.8.1, Whiteboard: [nvn-dl])
Attachments
(3 files, 7 obsolete files)
2.34 KB,
text/plain
|
Details | |
2.72 KB,
text/plain
|
Details | |
4.89 KB,
patch
|
jaas
:
review+
shaver
:
superreview+
shaver
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060206 Firefox/1.6a1 Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060206 Firefox/1.6a1 Seems 313398 fix causes BeOS builds to fail. Reproducible: Always Steps to Reproduce: 1.build Firefox after 2006-02-09 Actual Results: build breaks at xptcinvoke_unixish_x86.cpp Expected Results: good build
Reporter | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
What compiler are you using?
Reporter | ||
Comment 3•18 years ago
|
||
(In reply to comment #2) > What compiler are you using? > 2.95.3 - it's the latest supported under BeOS at this time.
Assignee | ||
Comment 4•18 years ago
|
||
I can reduce the argument count by getting rid of the false temp1 and temp2 arguments, but I fear that they were introduced intentionally in bug 16612 to work around a compiler bug. To be clear, what I'm suggesting is changing the argument spec to: : "=a" (result), /* %0 */ "=g" (saved_esp), /* %1 */ #ifdef KEEP_STACK_16_BYTE_ALIGNED "=D" (stack_params) /* %2 */ #else /* Don't waste a register, this isn't used if alignment is unimportant */ "=m" (stack_params) /* %2 */ #endif : "g" (that), /* %3 */ "g" (methodIndex), /* %4 */ "c" (paramCount), /* %5 */ "d" (params), /* %6 */ "g" (n), /* %7 */ "0" (fn_copy) /* %8 */ : "memory", "%ecx", "%edx" and adjusting the indices in the asm appropriately. Ulrich, if you're still out there, could you explain what the temps were about? Or maybe Mike might remember?
Reporter | ||
Comment 5•18 years ago
|
||
(In reply to comment #4) > I can reduce the argument count by getting rid of the false temp1 and temp2 > arguments This might be worth a try. I'm afraid I'm still getting used to handling code, so if this approach can be tested, I'd appreciate some hand-holding (or a patch or revised source) so I can change and test here.
Assignee | ||
Comment 6•18 years ago
|
||
This is a patch that does what I suggested in comment 4. Doug, give it a try and let me know if it works.
Assignee | ||
Updated•18 years ago
|
Component: General → XPConnect
Product: Firefox → Core
Version: unspecified → Trunk
Assignee | ||
Comment 7•18 years ago
|
||
I didn't actually remove temp1 and temp2 from the C++ source, I'd do that if this were to be promoted from possible patch to real patch.
Reporter | ||
Comment 8•18 years ago
|
||
With patch, we get closer, but not all the way there: c++ -o xptcinvoke_unixish_x86.o -c -DMOZILLA_INTERNAL_API -DOSTYPE=\"BeOS6.0.1\" -DOSARCH=\"BeOS\" -DBUILD_ID=2006021113 -DEXPORT_XPTC_API -I../../../../../../dist/include -I../../../../../../dist/include/xpcom -I../../../../../../dist/include/nspr -I/boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/../.. -fPIC -frtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-multichar -Wno-long-long -pedantic -pipe -DNDEBUG -DTRIMMED -ffunction-sections -O3 -march=i686 -mcpu=i686 -frerun-cse-after-loop -frerun-loop-opt -DMOZILLA_CLIENT -include ../../../../../../mozilla-config.h -Wp,-MD,.deps/xptcinvoke_unixish_x86.pp /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp: In function `nsresult XPTC_InvokeByIndex(class nsISupports *, unsigned int, unsigned int, struct nsXPTCVariant *)': /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:87: warning: unused variable `int temp2' /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:87: warning: unused variable `int temp1' /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:173: error: Invalid `asm' statement: /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:173: error: fixed or forbidden register 1 (dx) was spilled for class DREG. make[1]: *** [xptcinvoke_unixish_x86.o] Error 1 make[1]: Leaving directory `/boot/home/develop/mozilla/opt-Zeta-ZOOM/xpcom/reflect/xptcall/src/md/unix' make: *** [all] Error 2 $
Assignee | ||
Comment 9•18 years ago
|
||
Oh, yeah. Try this.
Attachment #211517 -
Attachment is obsolete: true
Assignee | ||
Comment 10•18 years ago
|
||
That last one probably won't work. This should, as long as gcc 2.95 supports the "+" constraint on outputs to mark them as inputs too (not sure if it does). I'd really like to avoid having two whole separate asm blocks just for this issue.
Reporter | ||
Comment 11•18 years ago
|
||
3rd time is not at charm :( Here's the output: c++ -o xptcinvoke_unixish_x86.o -c -DMOZILLA_INTERNAL_API -DOSTYPE=\"BeOS6.0.1\" -DOSARCH=\"BeOS\" -DBUILD_ID=2006021116 -DEXPORT_XPTC_API -I../../../../../../dist/include -I../../../../../../dist/include/xpcom -I../../../../../../dist/include/nspr -I/boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/../.. -fPIC -frtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-multichar -Wno-long-long -pedantic -pipe -DNDEBUG -DTRIMMED -ffunction-sections -O3 -march=i686 -mcpu=i686 -frerun-cse-after-loop -frerun-loop-opt -DMOZILLA_CLIENT -include ../../../../../../mozilla-config.h -Wp,-MD,.deps/xptcinvoke_unixish_x86.pp /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp: In function `nsresult XPTC_InvokeByIndex(class nsISupports *, unsigned int, unsigned int, struct nsXPTCVariant *)': /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:172: error: more than 10 operands in `asm' /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:81: warning: `PRUint32 result' might be used uninitialized in this function make[9]: *** [xptcinvoke_unixish_x86.o] Error 1 make[9]: Leaving directory `/boot/home/develop/mozilla/opt-Zeta-ZOOM/xpcom/reflect/xptcall/src/md/unix' make[8]: *** [libs] Error 2
Blocks: 313398
Comment 12•18 years ago
|
||
Doug, if nothing helps, search *.h files in mozilla/source/xpcom/reflect/xptcall/src/md/unix/ for BEOS defines and replace there #define CFRONT_STYLE_THIS_ADJUST for #define THUNK_BASED_THIS_ADJUST as we don't use anything lower than gcc 2.95 anymore
Assignee | ||
Comment 13•18 years ago
|
||
(In reply to comment #11) > 3rd time is not at charm :( Here's the output: > /boot/home/develop/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:172: > error: more than 10 operands in `asm' You haven't applied the patch!
Reporter | ||
Comment 14•18 years ago
|
||
It looked like I hadn't applied the patch, so I went carefully through the steps of deleting the source, getting clean from CVS, applying the patch, cleaning the object dir and making. Same error. I even tried applying patch .1, then .3 thinking they might be cumulative, but .3 fails to apply in that case so I assume not.
Reporter | ||
Comment 15•18 years ago
|
||
(In reply to comment #12) Found and made the suggested change, but it didn't take care of this issue, fyysik.
Comment 16•18 years ago
|
||
If it counts # as -v GNU assembler version 2.15 (i586-pc-beos) using BFD version 2.15-beos-041202
Comment 17•18 years ago
|
||
it seems i found real bug reason. #ifdef KEEP_STACK_16_BYTE_ALIGNED http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp#162 is misplaced. Probably to must be before "=g" (saved_esp), /* %3 */ http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp#161 With this fix all compiles. Though, i wish to know if my proposed change for *.h adds some benefit too.
Comment 18•18 years ago
|
||
actually i don't understand asm for 86 enough, but it is strange, according to comment http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp#89 that saved_esp, stack_params; are "used" outside KEEP_STACK_16_BYTE_ALIGNED case in several places
Comment 19•18 years ago
|
||
adding author of BeOS gcc 2.953 port. Also sent mail to him.
Assignee | ||
Comment 20•18 years ago
|
||
Comment 12: I am sure BeOS uses cfront-style "this" adjust for a reason. This is a choice dictated by the compiler, not a switch that will affect whether or not the code compiles. Comment 17: the #if is where it is for a reason. The asm opernad indices need to be the same in all compilation cases, because they're referred to by number in the asm. The #if is only there at all to free up a register when alignment is unimportant. Comment 14: OK, it sure looks like the output from the unpatched source, but apparently, gcc 2.95.3 is quietly treating single in-out asm operands as two, which is exactly the same behavior that we get with the unpatched source. I'll see if I can come up with a different way to eliminate an operand.
Comment 21•18 years ago
|
||
Comment 20: "I am sure BeOS uses cfront-style "this" adjust for a reason." When BeOS port was created, and this code, i believe, only working gcc for BeOS was 2.9. Now we have several problems with Mozilla + gcc 2.9, so are using 2.953 exclusively
Assignee | ||
Comment 22•18 years ago
|
||
This reuses the |n| operand for what |stack_params| did in the 16-bit alignment case, since |n| is no longer useful after initially moving the stack pointer if ESP is saved. The comments in the asm indicate that this is what I intended to do, although I apparently never actually made it happen.
Comment 23•18 years ago
|
||
(In reply to comment 21) fyysik: I think Mark is correct in saying that it is no good to use thunks on BeOS, as the _G_config.h used by the system doesn't make use of them. This is not a compiler issue, but one of the system. BeOS doesn't use thunks (aka: the C++-libraries provided by BeOS have not been compiled with thunks), so I suppose it simply won't work if you activate them in Mozilla.
Comment 24•18 years ago
|
||
Per Comment 23: Hello, Oliver. I'm glad you found time to look at our problem and explained situation with thunks, but what do you think about bug itself? I counted as i could, operands in old code, and for me nuber looked to be ever 10, but may be i'm wrong. Is it some our asm issue or gcc issue in parsing inline asm ?
Assignee | ||
Comment 25•18 years ago
|
||
Comment on attachment 211604 [details] [diff] [review] Eliminate an operand This only needs to be "r" when KEEP_STACK_16_BYTE_ALIGNED is defined because of the LEA instruction, it can remain "g" elsewhere. + "r" (n), /* %8 */
Assignee | ||
Comment 26•18 years ago
|
||
Comment 24: this bug appears to only be about exceeding gcc 2.95.3's limitation in the operand count. It's a build problem, not a runtime problem. If you were able to run Mozilla on BeOS before, then after patch v4 (or whatever patch eventually is produced here), you'll be able to run it again.
Comment 27•18 years ago
|
||
with patch https://bugzilla.mozilla.org/attachment.cgi?id=211604 xpcom builds and works. Anyone brave enough to checkin?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 28•18 years ago
|
||
Not without review! This version is the same as the previous, but with the change I mentioned in comment 25.
Assignee: nobody → mark
Attachment #211526 -
Attachment is obsolete: true
Attachment #211532 -
Attachment is obsolete: true
Attachment #211604 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #211640 -
Flags: review?(joshmoz)
Assignee | ||
Comment 29•18 years ago
|
||
Wrong file
Attachment #211640 -
Attachment is obsolete: true
Attachment #211641 -
Flags: review?(joshmoz)
Attachment #211640 -
Flags: review?(joshmoz)
Comment 30•18 years ago
|
||
https://bugzilla.mozilla.org/attachment.cgi?id=211641 Compiles and works here.
Reporter | ||
Comment 31•18 years ago
|
||
clean compile here, also. thank you, Mark, for the creative fix to accomodate our compiler's idiosyncracies.
Reporter | ||
Comment 32•18 years ago
|
||
This may be completely unrelated but once the initial problem is fixed and compile is clean, linking now fails.
Reporter | ||
Comment 33•18 years ago
|
||
again, a different place than before, but still breaking in xpcom. If unrelated I'll open a different bug.
Assignee | ||
Comment 34•18 years ago
|
||
Those both look like things that were affected by bug 325229, and may have since been fixed. Try updating if you haven't done so lately. In any event, they've got nothing to do with xptcall.
Reporter | ||
Comment 35•18 years ago
|
||
(In reply to comment #33) > Created an attachment (id=211693) [edit] > xpcom fails during debug build, too > > again, a different place than before, but still breaking in xpcom. If > unrelated I'll open a different bug. > bug was unrelated: new bug 326981 opened, fixed and closed. We still have the static linking problem, though. Any ideas?
Comment 36•18 years ago
|
||
Comment on attachment 211641 [details] [diff] [review] Eliminate an operand Looks good, works just fine on Mac OS X.
Attachment #211641 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #211641 -
Flags: superreview?(shaver)
Reporter | ||
Comment 37•18 years ago
|
||
Still waiting on SR. Also, opening separate bug to address static build bustage (c#32)
Comment 38•18 years ago
|
||
Comment on attachment 211690 [details]
xpcom fails during static linking
unrelated
Attachment #211690 -
Attachment is obsolete: true
Comment 39•18 years ago
|
||
Comment on attachment 211693 [details] xpcom fails during debug build, too Bug 318918
Attachment #211693 -
Attachment is obsolete: true
Assignee | ||
Comment 40•18 years ago
|
||
Comment on attachment 211641 [details] [diff] [review] Eliminate an operand Can we get some sr action here? This is a BeOS bustage fix, and the bustage was caused by a patch that has branch approval (bug 313398), and I'm waiting on this to land that patch on the branches.
Comment 41•18 years ago
|
||
I think that bugs which break building should be marked as critical.
Severity: major → critical
Comment on attachment 211641 [details] [diff] [review] Eliminate an operand sr=shaver; thanks to josh for his clarifications.
Attachment #211641 -
Flags: superreview?(shaver) → superreview+
Assignee | ||
Updated•18 years ago
|
Attachment #211641 -
Flags: approval1.8.0.2?
Attachment #211641 -
Flags: approval-branch-1.8.1?(shaver)
Updated•18 years ago
|
Flags: blocking1.8.0.2+
Comment 43•18 years ago
|
||
Comment on attachment 211641 [details] [diff] [review] Eliminate an operand approved for 180 branch, a=dveditz for drivers dependent on shaver's a= for 1.8.1
Attachment #211641 -
Flags: approval1.8.0.2? → approval1.8.0.2+
Assignee | ||
Comment 44•18 years ago
|
||
Checked in on trunk. I'll land this on the branches along with bug 313398 once this has a181.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 211641 [details] [diff] [review] Eliminate an operand b181=shaver
Attachment #211641 -
Flags: approval-branch-1.8.1?(shaver) → approval-branch-1.8.1+
Assignee | ||
Comment 46•18 years ago
|
||
Checked in on 1_8_0 and 1_8 branches.
Keywords: fixed1.8.0.2,
fixed1.8.1
Comment 47•18 years ago
|
||
marking [nvn-dl], which removes this bug from the "to be verified by QA" list for Firefox 1.5.0.2.
Whiteboard: [nvn-dl]
You need to log in
before you can comment on or make changes to this bug.
Description
•