Closed Bug 86446 Opened 23 years ago Closed 20 years ago

linux-alpha xptcstubs needs to be fixed for gcc3

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Platform:
DEC
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: shaver)

References

Details

Attachments

(3 files)

patch
14.06 KB, patch
shaver
: superreview+
Details | Diff | Splinter Review
patch fixes build when compiling w/ gcc-3.1
5.18 KB, patch
dbaron
: review+
shaver
: superreview+
Details | Diff | Splinter Review
patch to run under IRIX (used in conjunction with BZ:71627)
11.62 KB, patch
Details | Diff | Splinter Review 
We currently don't link correctly with gcc3 on linux alpha since the xptcstubs
code hard-codes the mangling of function names for the stubs (and perhaps other
ports as well -- I haven't looked yet).  This was reported in
news://news.mozilla.org/87itht24nw.fsf@student.uni-tuebingen.de

If it's not possible to use inline assembly within a function to do this as it's
done on x86, then the preprocessor magic will probably be messy since the new
mangled symbols include the length of the name of the function, e.g. (at least
for the x86 symbols):

_ZN14nsXPTCStubBase5Stub4Ev     instead of     Stub4__14nsXPTCStubBase
_ZN14nsXPTCStubBase6Stub29Ev    instead of     Stub29__14nsXPTCStubBase

See also my comments on bug 71627 about how gcc3 stuff should probably be #ifdef-ed.
We have this problem in:
xptcstubs_arm_netbsd.cpp
xptcstubs_linux_alpha.cpp
xptcstubs_netbsd_m68k.cpp
xptcstubs_ppc_linux.cpp
xptcstubs_ppc_netbsd.cpp
xptcstubs_ppc_rhapsody.cpp
xptcstubs_asm_irix.s
xptcstubs_asm_openvms_alpha.s
xptcstubs_asm_osf1_alpha.s
I'd really like to get this bug fixed, but I don't know how. Do I
understand it right that we have to either know the format of the
virtual table, or know the mangling? Both is extremely ugly, so if
there was a third way... If not, knowing the mangling seems slightly
less ugly to me, since bugs will already be detected at link time. As
mentioned, g++ 3.0 mangles the length of the method name;
unfortunately, I can't think of any magic that would make the C
preprocessor generate an appropritate mangled name. The best idea I
had is:

Make genstubs.pl add to the top of xptcstubsdef.inc:

#ifndef STUB_ENTRY1
# define STUB_ENTRY1(n) STUB_ENTRY(n)
# define STUB_ENTRY2(n) STUB_ENTRY(n)
# define STUB_ENTRY3(n) STUB_ENTRY(n)
#endif

and generate calls according to the number of digits of the stub
number. Not really all that nice, but would work with minimal
impact. Should I try that, or does anybody have a better idea?

The other possibility is to just try to use inline assembler within functions
like the x86 code does.  I'm not sure whether that will work, though.

Your STUB_ENTRY1 plan sounds like a good idea to me if that won't work.

We have to know the vtable format (see bug 71627).  We only need to know the
name mangling if inline assembler within functions doesn't work.
I can attempt a patch for OSF alpha (which has the same problem) - to go ontop 
of that patch at the bottom.

Any interest?

Note my own personal solution was 

#ifndef STUB_ENTRYWS
#define STUB_ENTRYWS(n,k) STUB_ENTRY(n)
#endif

and changed xptcstubsdef to 
STUB_ENTRYWS(1,5)
...
STUB_ENTRYWS(10,6)
...
STUB_ENTRYWS(100,7)
...
STUB_ENTRYWS(249,7)
then only need one #define in the assembly rather then 3.
Add myself as CC - incase anyone replies.
Sure, every bit helps.

(Want to take the bug?)
I will give this ago sometime, but unfortunately the computer I was using had a
licence expiration, and the licence update didnt apply cleanly.  Thus we now
have a non functional computer, hopefully not for too long.  The shared hard
drive is also running out of space so I had to delete my work so far, but it
should be fairly fast to pull back togeather.

Annoyingly, even when I finally got everything to compile and seemingly run on
the 4.0d box... mozilla died with stack overflow.  (if only the nightly 4.0f
snapshot actually ran on our 4.0f box - but it seems theres a few c++ library
symbols that are problematic.)
Hmmm.  I thought I checked this in long ago, but apparently I didn't.
Attachment #53024 - Flags: needs-work+
Comment on attachment 53024 [details] [diff] [review]
patch

dbaron: You may have asked the wrong reviewer :)

I'm *almost* OK with this. I have a few concerns...

1) Please add some code to generate a short C-style comment that
gives a clue that STUB_ENTRYx has some relationship to the log10 
of the index and that this is used for generating appropriate
symbol names on some platforms and that implementors can use it
or fallback on STUB_ENTRY as they choose. This just is not obvious
from the code. I know *many* things are not obvious, but this 
pushes things to a new level. Do this and convince me on the below
and I'm happy.

2) I didn't see any indication in the bug that anyone (you?) had
ever actually tested this and seen it run on at least one platform.
There also didn't seem to be a concensus that this is the best
solution. It just appears to be the only solution that anyone coded
and attached. Please, let's have some indication from interested
parties that this solution works and is the best we can come up
with.

3) I remain a bit concerned about the possible effects on other 
platforms. That generated header is included not only into .cpp
files were the macro processor can be assumed to have standard
support, it is also included into asm files on various platforms.
I recall that at least one platform had issues with some C macro
patterns. The #ifndefs and redefines *might* overpower the
cababilities on some platforms. I suppose that checking this in 
might be the only reasonable way to find out for sure. If there
are problems it might take a while to find out since we don't 
have universal tinderbox coverage.

Anyway, I'd say add some comments and get interested parties to 
claim some support - preferably saying they tested it - and I'll
OK this checkin.
Bug 142594 has a better way to do this.
I'm the original contributor of the alpha linux xptcall port.  Sorry I didn't
respond earlier...  I wasn't aware of this bug report until recently when I
upgraded to gcc-3.1 from egcs-1.1.2, and I haven't been building mozilla
regularly on alpha linux.  Anyway, I successfully built mozilla off the 1.0
branch using gcc-3.1 and it passes TestXPTCInvoke but it isn't pretty.  Anyone
know how to write a pure assembly function in gcc without prologue/epilogue
sequences?  Some ports have __attribute__((naked)) but that changes the function
prototype a bit and I don't think it's available in the alpha port.  I've also
looked at __builtin_frame_address() but I'm not sure how reliable that is. 
Maybe I'll just follow the PPC port to handle the name mangling issue?  Any
comments?
Okay here's the patch...

It passes TestXPTCInvoke when compiling w/ egcs-1.1.2 and gcc-3.1.  I don't
have gcc-2.95.x nor gcc-3.0.x installed so other people will have to check
those compilers if they're interested.
Comment on attachment 84336 [details] [diff] [review]
patch fixes build when compiling w/ gcc-3.1

r=dbaron
Attachment #84336 - Flags: review+
(And, FWIW, I stole a bit from that patch when working on bug 153525.)
If the IRIX patch in 71627 is applied, then this patch (or something like it)
must 
be applied to use gcc-3.

AFAIK we cannot (easily) implement an ASM solution as in 142594 on IRIX.
Mozilla and TestXPTCInvoke work with the patch and gcc-2.96-108
Attachment #84336 - Flags: superreview?(shaver)
Attachment #84336 - Flags: superreview?(shaver) → superreview+
dbaron, shaver: can attachment 84366 [details] [diff] [review] get checked in?
Any chance of this making it into the main tree? I've successfully built both Firefox and Mozilla on the 
Alpha under an in-testing port of Fedora Core 2 with the patch supplied. Perfectly stable. It's the only 
thing stopping the build on this platform with a recent toolchain installed.

Can supply the recent re-generated diffs, if necessary.
attachment 84336 [details] [diff] [review] checked in to trunk, 2004-09-06 09:14 -0700.
Blocks: 71627
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: