323114 - xptcstubs_linux_m68k.cpp needs fixing for gcc-4.0

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Zack Weinberg (:zwol)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051224 Debian/1.5.dfsg-3 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux m68k; en-US; rv:1.8) Gecko/20051224 Debian/1.5.dfsg-3 Firefox/1.5

gcc-4.0 will not compile Firefox for m68k - the file xptcstubs_linux_m68k.cpp spews errors:

../../../../../../dist/include/xpcom/xptcstubsdef.inc: In member function 'virtual nsresult nsXPTCStubBase::Stub3()':
../../../../../../dist/include/xpcom/xptcstubsdef.inc:5: error: asm-specifier for variable 'result' conflicts with asm clobber list

(repeat several hundred times with different member functions).

The problem is with the definition of STUB_ENTRY() in xptcstubs_linux_m68k.cpp.  I will quote the full text of that macro here:

#define STUB_ENTRY(n) \
nsresult nsXPTCStubBase::Stub##n() \
{ \
  register nsresult result asm("d0"); \
  void *frame = __builtin_frame_address(0); \
  __asm__ __volatile__( \
    "pea   %2@(12)\n\t"             /* args */ \
    "pea  "#n"\n\t"                 /* method index */ \
    "movl  %1, %/sp@-\n\t"          /* this */ \
    "jbsr  PrepareAndDispatch\n\t" \
    "addw  #12, %/sp" \
    : "=d" (result)     /* %0 */ \
    : "a" (this), "a" (frame) \
    : "a0", "a1", "d0", "d1", "memory" ); \
    return result; \
}

Note how the 'result' variable is forced to be in register d0.  The asm statement has 'result' as an output, and the clobber list contains "d0".  Thus, register d0 is in both the output list and the clobber list.  This has never worked in 100% of cases, and GCC 4.0 will issue errors rather than (potentially) generate incorrect code.

I suspect whoever wrote this code was simply listing all call-clobbered registers in the asm().  But GCC doesn't need to be told that here.  The asm statement is the only thing in the function, and GCC will treat a0/a1/d0/d1 as clobbered when compiling the caller.  Therefore, the only thing that *needs* to be in the clobber list is "memory".  You in fact get substantially better code by taking all the registers out, because then GCC knows it can use a0 and a1 for the 'frame' and 'this' variables.

I'll attach a patch to the bug.

Incidentally, I think you don't need an asm() at all in this function - it should suffice to write (since a declaration of PrepareAndDispatch is in scope):

#define STUB_ENTRY(n) \
nsresult nsXPTCStubBase::Stub##n() \
{ \
  uint32 *offset_frame = (uint32 *) __builtin_frame_address(0) + 3; \
  return PrepareAndDispatch(this, n, offset_frame); \
}

My patch doesn't do this, though, because I don't actually have an m68k box so I can't test it thoroughly enough.

Reproducible: Always

Comment 1

[Zack Weinberg (:zwol)]

Attachment: patch for bug

See also Debian bug #343687, by the way.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's the extra & for?

Also, do you happen to know with what gcc version __builtin_frame_address started existing?

Comment 3

[Calum Mackay]

It was in gcc-2.95, certainly.

Comment 4

[Zack Weinberg (:zwol)]

The extra & tells GCC not to put 'result' in the same register as any of the
input operands.  It's probably not necessary in this case, since the input
operands are constrained to go in address registers whereas 'result' has to be in a data register, but I was being extra cautious since (as I mentioned) I don't have access to an m68k.  (The patch was tested with a cross compiler and hand inspection of the assembly output.)

I believe __builtin_frame_address was available all the way back to gcc 2.0, if not earlier.  But why do you ask?  It is used in the original version of the code.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 208244 [details] [diff] [review]
patch for bug

ok, sounds good to me, although it would be nice if somebody with an m68k Linux machine could try this.  (If there aren't any users, why bother having code for it?)

Comment 6

[Zack Weinberg (:zwol)]

I presume that the Debian/m68k folks have a use for it, or they wouldn't have been trying to build it.  I'm involved because I know GCC dark corners pretty well and the Debian packager of Firefox asked for help.

Comment 7

[Zack Weinberg (:zwol)]

Um.  What does assigning the bug to me mean?  I'm not a Mozilla developer.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Um.  What does assigning the bug to me mean? 

It means you provided the patch, essentially.

Comment 10

[Zack Weinberg (:zwol)]

Oh, okay.  I thought you might have gotten the misapprehension I had CVS write access or something.

It would be nice if the patch could go on all active release branches, by the way.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 208244 [details] [diff] [review]
patch for bug

I'm a little hesitant to put it on branches until somebody confirms that it works.  To request approval for branches, edit the attachment and set the approval1.8.1 and approval1.8.0.2 flags to "?".

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 208244 [details] [diff] [review]
patch for bug

so flagged.  I'll poke the Debian folks to report whether it works.  I take it the 1.7.x branch is closed?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm holding off on giving this branch approval until somebody actually says it works.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 208244 [details] [diff] [review]
patch for bug

No response to dbaron, looks like this one misses 1.8.0.2

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No news on whether anybody's actually tested the patch?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 208244 [details] [diff] [review]
patch for bug

Denying approval since nobody's been able to say they tested it and it works.  (I even somewhat question whether we should have it on the trunk in that case.)

Comment 17

[Mike Hommey [:glandium]]

FWIW it's been applied for a while on debian and nobody ever complained it doesn't work.