Closed Bug 461270 Opened 11 years ago Closed 11 years ago

linking NSPR shared libs should use LDFLAGS

Categories

(NSPR :: NSPR, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: murali, Assigned: wtc)

Details

Attachments

(3 files, 5 obsolete files)

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
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).
Assignee: nobody → wtc
Component: Build Config → NSPR
Product: Firefox → NSPR
QA Contact: build.config → nspr
Version: unspecified → other
Status: UNCONFIRMED → NEW
Ever confirmed: true
Then  $(LD_FLAGS) should be added to the line # 352. That should fix the problem.
I meant $(LDFLAGS)
Modified the file rule.mk for libnpr to make the instrumented build compile.
Attachment #354227 - Flags: review+
Read it as nsprpub instead of 'libnpr'
Attachment #354227 - Flags: review+ → review?(wtc)
Comment on attachment 354227 [details] [diff] [review]
Fixed the rules.mk file for building instrumented build.

You need to request review from wtc.
Summary: rules.mk in nsprpub/config needs to be modified. → linking NSPR shared libs should use LDFLAGS
WTC, please review my change and approve. This fix helps my instrumentation automation scripts work well.
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.
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.
This patch is required in order to clean up the system due to the changes made to rules.mk to support instrumented build.
Ted,WTC can either of you please review and approve the patches so that I can check them in.
Just set review? wtc@google.com like I did on the previous patch.
Attachment #361316 - Flags: review?(wtc)
Attachment #361317 - Flags: review?(wtc)
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.
Roger that .. wilco
Attached patch configure.in CVS patch (obsolete) — Splinter Review
CVS patches are added
Attachment #354227 - Attachment is obsolete: true
Attachment #361316 - Attachment is obsolete: true
Attachment #361317 - Attachment is obsolete: true
Attachment #354227 - Flags: review?(wtc)
Attachment #361316 - Flags: review?(wtc)
Attachment #361317 - Flags: review?(wtc)
Attached patch rules.mk cvs patch (obsolete) — Splinter Review
Attachment #361387 - Flags: review?(nelson)
Comment on attachment 361387 [details] [diff] [review]
configure.in CVS patch


Please review and approve and check-in
Comment on attachment 361388 [details] [diff] [review]
rules.mk cvs patch

Please review,approve and check-in
Attachment #361388 - Flags: review?(nelson)
Attachment #361388 - Attachment is patch: true
Attachment #361388 - Attachment mime type: application/octet-stream → text/plain
Murali,  we need "unified" diffs, e.g. the output of "cvs diff -u"
Comment on attachment 361387 [details] [diff] [review]
configure.in CVS patch

need unified diffs.
Attachment #361387 - Flags: review?(nelson) → review-
Attachment #361388 - Flags: review?(nelson) → review-
unified diff for configure.in
Attachment #361387 - Attachment is obsolete: true
Attachment #361391 - Flags: review?(nelson)
Please review
Attachment #361388 - Attachment is obsolete: true
Attachment #361392 - Flags: review?(nelson)
Comment on attachment 361317 [details] [diff] [review]
Same patch as the original rule.mk patch but more in tune with the conventions.

r=wtc.
Attachment #361317 - Flags: review+
Comment on attachment 361391 [details] [diff] [review]
cvs unified diff for configure.in

r=wtc.
Attachment #361391 - Flags: review?(nelson) → review+
Attachment #361392 - Flags: review?(nelson) → review+
Comment on attachment 361392 [details] [diff] [review]
cvs unified diff foe rules.mk

r=wtc.
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
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
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8
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
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.
You need to log in before you can comment on or make changes to this bug.