Last Comment Bug 326838 - [BeOS] Build broken due to fix for 313398
: [BeOS] Build broken due to fix for 313398
Status: RESOLVED FIXED
[nvn-dl]
: fixed1.8.0.2, fixed1.8.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 BeOS
: -- critical (vote)
: ---
Assigned To: Mark Mentovai
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 313398
  Show dependency treegraph
 
Reported: 2006-02-11 13:39 PST by Doug Shelton
Modified: 2006-03-02 10:55 PST (History)
9 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
terminal error output from crash (2.34 KB, text/plain)
2006-02-11 13:43 PST, Doug Shelton
no flags Details
Possible patch (4.71 KB, patch)
2006-02-11 14:28 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
Fix bogus clobber spec (5.04 KB, patch)
2006-02-11 15:32 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
Better approach (3.86 KB, patch)
2006-02-11 15:58 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
fails with patch applied - console output (2.72 KB, text/plain)
2006-02-11 23:20 PST, Doug Shelton
no flags Details
Eliminate an operand (4.73 KB, patch)
2006-02-12 10:05 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
Eliminate an operand (7.70 KB, patch)
2006-02-12 14:00 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
Eliminate an operand (4.89 KB, patch)
2006-02-12 14:03 PST, Mark Mentovai
jaas: review+
shaver: superreview+
shaver: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
xpcom fails during static linking (2.15 KB, text/plain)
2006-02-12 22:51 PST, Doug Shelton
no flags Details
xpcom fails during debug build, too (1.82 KB, text/plain)
2006-02-12 22:58 PST, Doug Shelton
no flags Details

Description Doug Shelton 2006-02-11 13:39:57 PST
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
Comment 1 Doug Shelton 2006-02-11 13:43:23 PST
Created attachment 211513 [details]
terminal error output from crash
Comment 2 Mark Mentovai 2006-02-11 13:55:14 PST
What compiler are you using?
Comment 3 Doug Shelton 2006-02-11 14:18:04 PST
(In reply to comment #2)
> What compiler are you using?
>
2.95.3 - it's the latest supported under BeOS at this time.
Comment 4 Mark Mentovai 2006-02-11 14:19:14 PST
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?
Comment 5 Doug Shelton 2006-02-11 14:28:01 PST
(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.


Comment 6 Mark Mentovai 2006-02-11 14:28:53 PST
Created attachment 211517 [details] [diff] [review]
Possible patch

This is a patch that does what I suggested in comment 4.  Doug, give it a try and let me know if it works.
Comment 7 Mark Mentovai 2006-02-11 14:30:55 PST
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.
Comment 8 Doug Shelton 2006-02-11 14:51:25 PST
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
$                           
Comment 9 Mark Mentovai 2006-02-11 15:32:03 PST
Created attachment 211526 [details] [diff] [review]
Fix bogus clobber spec

Oh, yeah.  Try this.
Comment 10 Mark Mentovai 2006-02-11 15:58:46 PST
Created attachment 211532 [details] [diff] [review]
Better approach

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.
Comment 11 Doug Shelton 2006-02-11 16:55:01 PST
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
Comment 12 Sergei Dolgov 2006-02-11 18:14:26 PST
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
Comment 13 Mark Mentovai 2006-02-11 20:10:10 PST
(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!
Comment 14 Doug Shelton 2006-02-11 23:20:42 PST
Created attachment 211566 [details]
fails with patch applied - console output

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.
Comment 15 Doug Shelton 2006-02-11 23:29:47 PST
(In reply to comment #12)
Found and made the suggested change, but it didn't take care of this issue, fyysik.  

Comment 16 Sergei Dolgov 2006-02-12 04:40:26 PST
If it counts 

# as -v
GNU assembler version 2.15 (i586-pc-beos) using BFD version 2.15-beos-041202 
Comment 17 Sergei Dolgov 2006-02-12 05:32:05 PST
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 Sergei Dolgov 2006-02-12 05:41:36 PST
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 Sergei Dolgov 2006-02-12 06:25:22 PST
adding author of BeOS gcc 2.953 port.
Also sent mail to him.
Comment 20 Mark Mentovai 2006-02-12 07:55:37 PST
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 Sergei Dolgov 2006-02-12 08:08:05 PST
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
Comment 22 Mark Mentovai 2006-02-12 10:05:41 PST
Created attachment 211604 [details] [diff] [review]
Eliminate an operand

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 Oliver Tappe 2006-02-12 10:14:18 PST
(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 Sergei Dolgov 2006-02-12 10:27:21 PST
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 25 Mark Mentovai 2006-02-12 10:29:34 PST
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 26 Mark Mentovai 2006-02-12 10:33:12 PST
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 Sergei Dolgov 2006-02-12 13:55:30 PST
with patch
https://bugzilla.mozilla.org/attachment.cgi?id=211604
xpcom builds and works.

Anyone brave enough to checkin?
Comment 28 Mark Mentovai 2006-02-12 14:00:40 PST
Created attachment 211640 [details] [diff] [review]
Eliminate an operand

Not without review!

This version is the same as the previous, but with the change I mentioned in comment 25.
Comment 29 Mark Mentovai 2006-02-12 14:03:38 PST
Created attachment 211641 [details] [diff] [review]
Eliminate an operand

Wrong file
Comment 30 Sergei Dolgov 2006-02-12 14:17:03 PST
https://bugzilla.mozilla.org/attachment.cgi?id=211641

Compiles and works here.
Comment 31 Doug Shelton 2006-02-12 22:44:15 PST
clean compile here, also.  thank you, Mark, for the creative fix to accomodate our compiler's idiosyncracies.
Comment 32 Doug Shelton 2006-02-12 22:51:41 PST
Created attachment 211690 [details]
xpcom fails during static linking

This may be completely unrelated but once the initial problem is fixed and compile is clean, linking now fails.
Comment 33 Doug Shelton 2006-02-12 22:58:32 PST
Created attachment 211693 [details]
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.
Comment 34 Mark Mentovai 2006-02-13 05:57:17 PST
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.
Comment 35 Doug Shelton 2006-02-13 09:45:13 PST
(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 Josh Aas 2006-02-13 15:18:32 PST
Comment on attachment 211641 [details] [diff] [review]
Eliminate an operand

Looks good, works just fine on Mac OS X.
Comment 37 Doug Shelton 2006-02-15 14:40:39 PST
Still waiting on SR.  Also, opening separate bug to address static build bustage (c#32)
Comment 38 Sergei Dolgov 2006-02-19 05:15:10 PST
Comment on attachment 211690 [details]
xpcom fails during static linking

unrelated
Comment 39 Sergei Dolgov 2006-02-19 05:16:42 PST
Comment on attachment 211693 [details]
xpcom fails during debug build, too

Bug 318918
Comment 40 Mark Mentovai 2006-02-19 07:33:15 PST
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 Sergei Dolgov 2006-02-19 18:11:34 PST
I think that bugs which break building should be marked as critical.
Comment 42 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-02-21 13:53:49 PST
Comment on attachment 211641 [details] [diff] [review]
Eliminate an operand

sr=shaver; thanks to josh for his clarifications.
Comment 43 Daniel Veditz [:dveditz] 2006-02-21 15:37:12 PST
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
Comment 44 Mark Mentovai 2006-02-21 20:06:06 PST
Checked in on trunk.

I'll land this on the branches along with bug 313398 once this has a181.
Comment 45 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-02-23 11:22:38 PST
Comment on attachment 211641 [details] [diff] [review]
Eliminate an operand

b181=shaver
Comment 46 Mark Mentovai 2006-02-23 11:37:45 PST
Checked in on 1_8_0 and 1_8 branches.
Comment 47 Dave Liebreich [:davel] 2006-03-02 10:55:00 PST
marking [nvn-dl], which removes this bug from the "to be verified by QA" list
for Firefox 1.5.0.2.

Note You need to log in before you can comment on or make changes to this bug.