Closed Bug 326838 Opened 18 years ago Closed 18 years ago

[BeOS] Build broken due to fix for 313398

Categories

(Core :: XPConnect, defect)

x86
BeOS
defect
Not set
critical

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)

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
What compiler are you using?
(In reply to comment #2)
> What compiler are you using?
>
2.95.3 - it's the latest supported under BeOS at this time.
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?
(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.


Attached patch Possible patch (obsolete) — Splinter Review
This is a patch that does what I suggested in comment 4.  Doug, give it a try and let me know if it works.
Component: General → XPConnect
Product: Firefox → Core
Version: unspecified → Trunk
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.
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
$                           
Attached patch Fix bogus clobber spec (obsolete) — Splinter Review
Oh, yeah.  Try this.
Attachment #211517 - Attachment is obsolete: true
Attached patch Better approach (obsolete) — Splinter Review
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.
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
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
(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!
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.
(In reply to comment #12)
Found and made the suggested change, but it didn't take care of this issue, fyysik.  

If it counts 

# as -v
GNU assembler version 2.15 (i586-pc-beos) using BFD version 2.15-beos-041202 
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.
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
adding author of BeOS gcc 2.953 port.
Also sent mail to him.
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 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
Attached patch Eliminate an operand (obsolete) — Splinter Review
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.
(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.
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 ?
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 */
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.
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
Attached patch Eliminate an operand (obsolete) — Splinter Review
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)
Wrong file
Attachment #211640 - Attachment is obsolete: true
Attachment #211641 - Flags: review?(joshmoz)
Attachment #211640 - Flags: review?(joshmoz)
clean compile here, also.  thank you, Mark, for the creative fix to accomodate our compiler's idiosyncracies.
Attached file xpcom fails during static linking (obsolete) —
This may be completely unrelated but once the initial problem is fixed and compile is clean, linking now fails.
Attached file xpcom fails during debug build, too (obsolete) —
again, a different place than before, but still breaking in xpcom.   If unrelated I'll open a different bug.
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.
(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 on attachment 211641 [details] [diff] [review]
Eliminate an operand

Looks good, works just fine on Mac OS X.
Attachment #211641 - Flags: review?(joshmoz) → review+
Attachment #211641 - Flags: superreview?(shaver)
Still waiting on SR.  Also, opening separate bug to address static build bustage (c#32)
Comment on attachment 211690 [details]
xpcom fails during static linking

unrelated
Attachment #211690 - Attachment is obsolete: true
Comment on attachment 211693 [details]
xpcom fails during debug build, too

Bug 318918
Attachment #211693 - Attachment is obsolete: true
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.
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+
Attachment #211641 - Flags: approval1.8.0.2?
Attachment #211641 - Flags: approval-branch-1.8.1?(shaver)
Flags: blocking1.8.0.2+
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+
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+
Checked in on 1_8_0 and 1_8 branches.
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.

Attachment

General

Creator:
Created:
Updated:
Size: