232763 - build breaks when static libiberty is used in a dynamic library

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Henrik Lynggaard Hansen]

When building for amd64 (using the pacth in bug 232742) I cannot build in
non-static versions

The firebird I built in the mentioned bug had static on thats why it works, but
the thunderbird i just tried to build failed since i "forgot" eneable static

I get the following in my log:

nsComponentManager.cpp:2813: warning: unused variable `const char*className'
/usr/lib/gcc-lib/x86_64-pc-linux-gnu/3.3.2/../../../../x86_64-pc-linux-gnu/bin/ld:
/usr/lib/gcc-lib/x86_64-pc-linux-gnu/3.3.2/libiberty.a(cplus-dem.o): relocation
R_X86_64_32S can not be used when making a shared object; recompile with -fPIC

I'm makeing this a blocker as I don't know if compiling with static is an option
for all..

Comment 1

[Henrik Lynggaard Hansen]

requesting this block 1.7a

Comment 2

[cls]

This is that old problem where libiberty.a wasn't compiled with -fPIC so it
cannot be linked into any shared libraries (see bug 35419). We need to find an
alternative to using libiberty, since it's never built with -fPIC and gcc is now
complaining about the type mismatches.

The workaround is to do a --disable-debug build or to unset MOZ_DEMANGLE_SYMBOLS
in both config/autoconf.mk & mozilla-config.h after running configure.

Comment 3

[cls]

Can someone (Mitchell?) take a look at the additional permissions listed in
cp-demange.{c,h} and determine if those additional perms allow us to incorporate
those files into the tree w/o tainting the tree?

http://sources.redhat.com/cgi-bin/cvsweb.cgi/gcc/libiberty/cp-demangle.c?rev=1.67&content-type=text/x-cvsweb-markup&cvsroot=gcc

http://sources.redhat.com/cgi-bin/cvsweb.cgi/gcc/libiberty/cp-demangle.h?rev=1.1&content-type=text/x-cvsweb-markup&cvsroot=gcc

Comment 4

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

We could probably add a cascade of autoconf tests for __cxa_demangle and then
what we're currently using, and just use neither if both tests fail.

Comment 5

[cls]

Ok, __cxa_demangle it is.  What we currently have really isn't an option since
we detect & attempt to use libiberty by default with no way to override that.

Comment 6

[Mitchell Baker]

Re cls' Comment #3 , the permission applies to  linking "the
   compiled version" of this file.  When you say include in the tree, do you
mean include the compiled version in our tree, or include the source version
which then gets included in the binary we build?  

I'm not sure if the two settings are different, maybe someone knows?

mitchell

Comment 7

[Henrik Lynggaard Hansen]

Maybe it is a silly qestion, but where does llibiberty.a come from ?

-Is it something that is just picked up from my system, because then it should
have compiled as 64bit given this is a gentoo system.

- If it comes precompiled in the mozillatree, would it then not be possible to
tequest a new version from whomever has built it ?

I will try the workaroun of using disable debug later today....

Comment 8

[cls]

I meant include the source files in the tree since that routine is the only
reason we link against libiberty.  That may be a non-issue now if we can just
switch to using __cxa_demangle as dbaron suggested. 

libiberty is built with binutils & gcc.  Being built as 64bit is not the issue.
 Since libiberty was built as a static library, it wasn't compiled using -fPIC
and therefore cannot be linked into a shared library.  Apparently, that
restriction is platform dependent as the builds obviously worked at some point.

Comment 9

[cls]

Attachment: v1.0

Here's an untested patch to use __cxa_demangle instead of cplus_demangle.  My
trace-malloc enabled builds are breaking even without the patch so I can't
verify that it works like it's supposed to in that case.  --enable-debug builds
work fine though.

Comment 10

[Andrew Schultz]

Comment on attachment 140639 [details] [diff] [review]
v1.0

> #if defined(MOZ_DEMANGLE_SYMBOLS)
>+    char oufBuffer[4096];

outBuffer?

Comment 11

[cls]

Attachment: v1.1

Whoops.  Turns out that I botched that last change to the configure test so it
wasn't being built anyway.  I've switched to another box which is actually able
to run the pre-patch trace-malloc builds so this patch was actually tested. :)

Comment 12

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

Comment on attachment 140941 [details] [diff] [review]
v1.1

>+    case "$target" in
>+    *-linux*|*-solaris*)

No need to limit to linux and solaris -- it should work anywhere we have gcc3.

If you've tested that demangling still works in nsTraceRefcnt or trace-malloc
output, r=dbaron.

Comment 13

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

Considering that we've shipped with this many times, it's not a blocker, but it
would be nice to fix.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 140941 [details] [diff] [review]
v1.1

sr=me.

/be

Comment 15

[cls]

Attachment: tracemalloc C->C++ changes

Well, so much for that.  I missed the cplus_demangle call in libtracemalloc.so
so we have the same issue there.  Making the cplus_demangle->__cxa_demangle
switch in nsTraceMalloc.c means compiling the file as a C++ file.

Comment 16

[cls]

Attachment: v1.2

This patch contains the full diff including the moving of nsTraceMalloc.c to
nsTraceMalloc.cpp .

Comment 17

[Brendan Eich [:brendan]]

cls: can't you just feed the .c file to g++ with some option to enable linking
with the cxa stuff?  I don't want that source "becoming" C++ if we can help it.

/be

Comment 18

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

That wouldn't work, but a wrapper function, defined within an extern "C" block,
in another file that just calls abi::__cxa_demangle would.

Comment 19

[Brendan Eich [:brendan]]

Alright, I give up, but I want cvs history preserved.  I'll do the copy if need
be and if dbaron approves the patch -- cls, say the word.

/be

Comment 20

[Brendan Eich [:brendan]]

I'm not just a C luddite, I've also hacked nsTraceMalloc.h-exported calls into C
code, and probably will again.  So I really do want C linkage for those exports.
 That means extern "C" stuff
(http://lxr.mozilla.org/mozilla/source/tools/trace-malloc/lib/nsTraceMalloc.h#41)
should remain.

/be

Comment 21

[cls]

Attachment: v1.3

I ran into a problem with the previous patch anyway and it was causing mozilla
to hang.  I split nsDemangle() into a separate file like we do for
nsGetTypeName().

Comment 22

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

Comment on attachment 141087 [details] [diff] [review]
v1.3

There's no need to |#ifdef NS_TRACE_MALLOC| inside tools/trace-malloc/lib.

nsDemangle doesn't need to be extern if it's only used in nsTraceMalloc.  It
should also probably only be declared within an extern "C" block, not defined. 
The nested ifs could just be if (status && demangled).	Finally, __cxa_demangle
only allocates if you pass it null for the incoming buffer, and the incoming
buffer may not be a stack buffer, since it's allocated via malloc.  In full (to
quote the libstdc++ source code):

// __cxa_demangle
//
// Demangle a C++ symbol or type name.
//
// `mangled-name' is a pointer to a null-terminated array of characters.
// It may be either an external name, i.e. with a "_Z" prefix, or an
// internal NTBS mangling, e.g. of a type for type_info.
//
// `buf' may be null.  If it is non-null, then n must also be non-null,
// and buf is a pointer to an array, of at least *n characters, that
// was allocated using malloc.
//
// `status' points to an int that is used as an error indicator. It is
// permitted to be null, in which case the user just doesn't get any
// detailed error information.
//
// Returns: a pointer to a null-terminated array of characters, the
//	    demangled name.  Or NULL in case of failure.
//
// If there is an error in demangling, the return value is a null pointer.
// The user can examine *status to find out what kind of error occurred.
// Meaning of error indications:
//
//     *  0: success
//     * -1: memory allocation failure
//     * -2: invalid mangled name
//     * -3: invalid arguments (e.g. buf nonnull and n null)

Comment 23

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

(In other words, I think the last three arguments to __cxa_demangle should all
be null.)

Comment 24

[cls]

Attachment: v1.4

It would have been nice if they had included that info in the header but oh,
well.

Comment 25

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

Comment on attachment 141090 [details] [diff] [review]
v1.4

>+extern "C" char *nsDemangle(const char *symbol);

I think there's a difference between extern "C" in a function declaration and
being in an |extern "C"| block, and I think you want the latter.  My memory
could be wrong, though.

>+
>+char *nsDemangle(const char *symbol) {
>+    return  abi::__cxa_demangle(symbol, 0, 0, 0);
>+}
>+
>+#endif
>Index: tools/trace-malloc/lib/nsTraceMalloc.c
>+extern char *nsDemangle(const char *);

You shouldn't need |extern| here.

Comment 26

[cls]

> (From update of attachment 141090 [details] [diff] [review])
> >+extern "C" char *nsDemangle(const char *symbol);
> 
> I think there's a difference between extern "C" in a function declaration and
> being in an |extern "C"| block, and I think you want the latter.  My memory
> could be wrong, though.

I thought the |extern "C"| block was just the multi-statement version of |extern
"C" <func-decl>|.  In any case, I'm just following the example set by
nsGetTypeName() in tools/trace-malloc/lib/nsTypeInfo.cpp.  It uses |extern "C"|
for the function declaration but not for the rest of the file.  Which also has
the extra extern, btw.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 141090 [details] [diff] [review]
v1.4

Merger before you check in -- I sync'ed up an old patch to add
NS_TrackAllocation, a way to dump stacks to a trace file for every realloc and
for the final free, given a live allocation pointer.

/be

Comment 28

[cls]

Patch has been checked in.

Comment 29

[cls]

*** Bug 225420 has been marked as a duplicate of this bug. ***

Comment 30

[cls]

*** Bug 222723 has been marked as a duplicate of this bug. ***