461270 - linking NSPR shared libs should use LDFLAGS

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Murali Nandigama [:murali]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_4; en-us) AppleWebKit/525.18 (KHTML, like Gecko) Version/3.1.2 Safari/525.20.1
Build Identifier: 3.1b2Pre

There is a bug in http://mxr.mozilla.org/mozilla-central/source/nsprpub/config/rules.mk at line #352.

The rules.mk does not pass to the $(MKSHLIB) command,   the DSO_LDOPTS values properly. 

Also  DSO_LDOPTS  that are set in the .mozconfig are not being passed to the line #352. It's probably worth  filing a bug on that, since otherwise there's no way to get libnspr to  link against optional additional libraries.
 

Reproducible: Always

Steps to Reproduce:
1.set LDFLAGS='-lgcov -static-libgcc' ;  DSO_LDOPTS='-lgcov -static-libgcc' in the .mozconfig file
2.run the make command 'make -f client.mk build
3.Linker fails due to unresolved gcov issues since the DSO_LDOPTS in the .mozconfig are not passed to the line #352 in the rules.mk
Actual Results:  
unsatisfied DSO reference errors

Expected Results:  
It should link and generate the libnspr4.so

Comment 1

[Benjamin Smedberg]

I believe NSPR should honor LDFLAGS from the environment even for shared library linking. I don't think it should honor DSO_LDOPTS (that's an internal implementation detail).

Comment 2

[Murali Nandigama [:murali]]

Then  $(LD_FLAGS) should be added to the line # 352. That should fix the problem.

Comment 3

[Murali Nandigama [:murali]]

I meant $(LDFLAGS)

Comment 4

[Murali Nandigama [:murali]]

Attachment: Fixed the rules.mk file for building instrumented build.

Modified the file rule.mk for libnpr to make the instrumented build compile.

Comment 5

[Murali Nandigama [:murali]]

Read it as nsprpub instead of 'libnpr'

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 354227 [details] [diff] [review]
Fixed the rules.mk file for building instrumented build.

You need to request review from wtc.

Comment 7

[Murali Nandigama [:murali]]

WTC, please review my change and approve. This fix helps my instrumentation automation scripts work well.

Comment 8

[Wan-Teh Chang]

Comment on attachment 354227 [details] [diff] [review]
Fixed the rules.mk file for building instrumented build.

Thanks for the patch.  I no longer have time to stay on
top of build issues, so my strategy is to make NSPR's
build system similar to Mozilla's build system.  So,
please put $(LDFLAGS) between $(RES) and $(EXTRA_LIBS)
in the change below:

>-	$(MKSHLIB) $(OBJS) $(RES) $(EXTRA_LIBS)
>+	$(MKSHLIB) $(OBJS) $(RES) $(EXTRA_LIBS) $(LDFLAGS)

to match the ordering in mozilla/config/rules.mk:
(http://mxr.mozilla.org/mozilla-central/source/config/rules.mk)

  $(MKSHLIB) $(SHLIB_LDSTARTFILE) $(OBJS) $(LOBJS) $(SUB_SHLOBJS) $(DTRACE_PROBE_OBJ) $(PROBE_LOBJS) $(RESFILE) $(LDFLAGS) $(EXTRA_DSO_LDOPTS) $(OS_LIBS) $(EXTRA_LIBS) $(DEF_FILE) $(SHLIB_LDENDFILE)

Also, this patch changes the NSPR build system's long-time
assumption that $(LDFLAGS) is not used in building shared
libraries, so we need to update our configure script and
makefiles with the new assumption.  Here are some specific
examples of code in mozilla/nsprpub/configure.in that need
to be updated:

http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#1104

1104                 dnl LDFLAGS is for the utilities built in config (now and
1105                 dnl nsinstall).  DSO_LDOPTS is used when linking shared
1106                 dnl libraries.
1107                 LDFLAGS="${MACOS_SDK_LIBS} $LDFLAGS"
1108                 DSO_LDOPTS="${MACOS_SDK_LIBS} $DSO_LDOPTS"

http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#1131

1131                 dnl Both LDFLAGS and DSO_LDOPTS are set here, see the
1132                 dnl gcc < 4.0 case for the explanation.
1133                 if test "$GCC_VERSION_FULL" != "4.0.0" ; then
1134                     dnl gcc > 4.0.0 will pass -syslibroot to ld automatically
1135                     dnl based on the -isysroot it receives.
1136                     LDFLAGS="$LDFLAGS -isysroot ${MACOS_SDK_DIR}"
1137                     DSO_LDOPTS="$DSO_LDOPTS -isysroot ${MACOS_SDK_DIR}"
1138                 else
1139                     dnl gcc 4.0.0 doesn't pass -syslibroot to ld, it needs
1140                     dnl to be explicit.
1141                     LDFLAGS="$LDFLAGS -Wl,-syslibroot,${MACOS_SDK_DIR}"
1142                     DSO_LDOPTS="$DSO_LDOPTS -Wl,-syslibroot,${MACOS_SDK_DIR}"
1143                 fi

http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#2205

2205       DSO_LDOPTS="$DSO_LDOPTS -Zhigh-mem"
2206       LDFLAGS="$LDFLAGS -Zhigh-mem"

These examples show that if we don't update the NSPR build
system with the new assumption, we will be passing duplicate
linker flags when we build shared libraries on some platforms.
It should be a harmless problem, but we should take the
time to fix it so that our build system won't degrade with
the patch.

Comment 9

[Wan-Teh Chang]

I forgot to mention that for the examples I cited, the fix
is to simply remove any comments that explain why both
LDFLAGS and DSO_LDOPTS need to be set, and remove the
DSO_LDOPTS assignments, leaving only the LDFLAGS assignments.

Comment 10

[Murali Nandigama [:murali]]

Attachment: Modifications done to configure.in to remove clutter.

This patch is required in order to clean up the system due to the changes made to rules.mk to support instrumented build.

Comment 11

[Murali Nandigama [:murali]]

Attachment: Same patch as the original rule.mk patch but more in tune with the conventions.

Comment 12

[Murali Nandigama [:murali]]

Ted,WTC can either of you please review and approve the patches so that I can check them in.

Comment 13

[(not currently active) Ted Mielczarek]

Just set review? [e-mail] like I did on the previous patch.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Murali, your patch is to the Hg repository.
I hope you understand that the master repository for NSPR is the CVS 
repository, and that any approved changes must be checked in there first.
The Hg repository should always match a tagged version of NSPR in the 
CVS repository.  That means that changes get committed in CVS, then a 
CVS tag is created, and finally the "snapshot" from that tag is committed
(or "pushed" or whatever is the right Hg term) to Hg.

Comment 15

[Murali Nandigama [:murali]]

Roger that .. wilco

Comment 16

[Murali Nandigama [:murali]]

Attachment: configure.in CVS patch

CVS patches are added

Comment 17

[Murali Nandigama [:murali]]

Attachment: rules.mk cvs patch

Comment 18

[Murali Nandigama [:murali]]

Comment on attachment 361387 [details] [diff] [review]
configure.in CVS patch


Please review and approve and check-in

Comment 19

[Murali Nandigama [:murali]]

Comment on attachment 361388 [details] [diff] [review]
rules.mk cvs patch

Please review,approve and check-in

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Murali,  we need "unified" diffs, e.g. the output of "cvs diff -u"

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 361387 [details] [diff] [review]
configure.in CVS patch

need unified diffs.

Comment 22

[Murali Nandigama [:murali]]

Attachment: cvs unified diff for configure.in

unified diff for configure.in

Comment 23

[Murali Nandigama [:murali]]

Attachment: cvs unified diff foe rules.mk

Please review

Comment 24

[Wan-Teh Chang]

Comment on attachment 361317 [details] [diff] [review]
Same patch as the original rule.mk patch but more in tune with the conventions.

r=wtc.

Comment 25

[Wan-Teh Chang]

Comment on attachment 361391 [details] [diff] [review]
cvs unified diff for configure.in

r=wtc.

Comment 26

[Wan-Teh Chang]

Comment on attachment 361392 [details] [diff] [review]
cvs unified diff foe rules.mk

r=wtc.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

I committed Murali's patch to CVS trunk
Checking in configure.in;    new revision: 1.250; previous revision: 1.249
Checking in config/rules.mk; new revision: 3.69;  previous revision: 3.68

Comment 28

[Wan-Teh Chang]

Attachment: Supplemental patch for configure.in

Attachment 361391 [details] [diff] missed some redundancies between
DSO_LDOPTS and LDFLAGS.  This supplemental patch
removes them.

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.246; previous revision: 1.245
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.251; previous revision: 1.250
done

Comment 29

[Wan-Teh Chang]

I pushed NSPR_HEAD_20090210 (which contains the patches in
this bug) to mozilla-central in changeset 171d7e245883:
http://hg.mozilla.org/mozilla-central/rev/171d7e245883

Comment 30

[Ivan Pechorin]

It seems like the change in nsprpub/config/rules.mk broke non-GCC (HP aCC) build on HP-UX. I wonder if anyone except me cares of this platform anymore?

My environment:

export CC=aCC
export CXX=aCC
export LD=aCC
export CFLAGS="-g +Z -Ae -mt"
export CXXFLAGS="-g +Z -AA -mt"
export LDFLAGS="-AA -mt"

Despite LD being defined as "aCC", NSPR was traditionally linked using /usr/bin/ld, and it worked fine until aCC's LDFLAGS were added.

Linking error after this change:

/usr/bin/ld -b +h libnspr4.so +b '$ORIGIN' -o libnspr4.so ./prvrsion.o io/./prfdcach.o io/./prmwait.o io/./prmapopt.o io/./priometh.o io/./pripv6.o io/./prlayer.o io/./prlog.o io/./prmmap.o io/./prpolevt.o io/./prprf.o io/./prscanf.o io/./prstdio.o threads/./prcmon.o threads/./prrwlock.o threads/./prtpd.o linking/./prlink.o malloc/./prmalloc.o malloc/./prmem.o md/./prosdep.o memory/./prshm.o memory/./prshma.o memory/./prseg.o misc/./pralarm.o misc/./pratom.o misc/./prcountr.o misc/./prdtoa.o misc/./prenv.o misc/./prerr.o misc/./prerror.o misc/./prerrortable.o misc/./prinit.o misc/./prinrval.o misc/./pripc.o misc/./prlog2.o misc/./prlong.o misc/./prnetdb.o misc/./praton.o misc/./prolock.o misc/./prrng.o misc/./prsystem.o misc/./prthinfo.o misc/./prtpool.o misc/./prtrace.o misc/./prtime.o pthreads/./ptsynch.o pthreads/./ptio.o pthreads/./ptthread.o pthreads/./ptmisc.o md/unix/./unix.o md/unix/./unix_errors.o md/unix/./uxproces.o md/unix/./uxrng.o md/unix/./uxshm.o md/unix/./uxwrap.o md/unix/./hpux.o md/unix/./os_HPUX_ia64.o  -AA -mt -lpthread -lrt -ldld -lm -lc
ld: Unrecognized argument: -AA
Fatal error.
gmake[5]: *** [libnspr4.so] Error 1

Comment 31

[Wan-Teh Chang]

It seems that NSPR should use aCC (your LD) to link
shared libraries.  I don't know why that isn't happening.

I see this statement in NSPR's configure.in:
    AC_PATH_PROGS(LD, ld link, echo not_ld)

Does that not honor the value of the LD environment
variable?  (I don't know autoconf very well.)

I suspect the other people build NSPR on HP-UX with
the default compilers, linker, and flags.