[BeOS] Build broken due to fix for 313398

RESOLVED FIXED

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Doug Shelton, Assigned: Mark Mentovai)

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
x86
BeOS
fixed1.8.0.2, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

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

12 years ago
Created attachment 211513 [details]
terminal error output from crash
(Assignee)

Comment 2

12 years ago
What compiler are you using?
(Reporter)

Comment 3

12 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

12 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

12 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

12 years ago
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.
(Assignee)

Updated

12 years ago
Component: General → XPConnect
Product: Firefox → Core
Version: unspecified → Trunk
(Assignee)

Comment 7

12 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

12 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

12 years ago
Created attachment 211526 [details] [diff] [review]
Fix bogus clobber spec

Oh, yeah.  Try this.
Attachment #211517 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
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.
(Reporter)

Comment 11

12 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

12 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

12 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

12 years ago
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.
(Reporter)

Comment 15

12 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

12 years ago
If it counts 

# as -v
GNU assembler version 2.15 (i586-pc-beos) using BFD version 2.15-beos-041202 

Comment 17

12 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

12 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

12 years ago
adding author of BeOS gcc 2.953 port.
Also sent mail to him.
(Assignee)

Comment 20

12 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

12 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

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

12 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

12 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

12 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

12 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

12 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

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

12 years ago
Created attachment 211641 [details] [diff] [review]
Eliminate an operand

Wrong file
Attachment #211640 - Attachment is obsolete: true
Attachment #211641 - Flags: review?(joshmoz)
Attachment #211640 - Flags: review?(joshmoz)

Comment 30

12 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=211641

Compiles and works here.
(Reporter)

Comment 31

12 years ago
clean compile here, also.  thank you, Mark, for the creative fix to accomodate our compiler's idiosyncracies.
(Reporter)

Comment 32

12 years ago
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.
(Reporter)

Comment 33

12 years ago
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.
(Assignee)

Comment 34

12 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

12 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

12 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

12 years ago
Attachment #211641 - Flags: superreview?(shaver)
(Reporter)

Comment 37

12 years ago
Still waiting on SR.  Also, opening separate bug to address static build bustage (c#32)

Comment 38

12 years ago
Comment on attachment 211690 [details]
xpcom fails during static linking

unrelated
Attachment #211690 - Attachment is obsolete: true

Comment 39

12 years ago
Comment on attachment 211693 [details]
xpcom fails during debug build, too

Bug 318918
Attachment #211693 - Attachment is obsolete: true
(Assignee)

Comment 40

12 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

12 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

12 years ago
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+
(Assignee)

Comment 44

12 years ago
Checked in on trunk.

I'll land this on the branches along with bug 313398 once this has a181.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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

12 years ago
Checked in on 1_8_0 and 1_8 branches.
Keywords: fixed1.8.0.2, fixed1.8.1
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.