983186 - js/src/configure.in should append -Wl,--build-id to NSPR_LDFLAGS

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Remove the unused NSPR_LDFLAGS variable from js/src/configure.in

The first issue is easy to fix.

In http://hg.mozilla.org/mozilla-central/rev/178f3283557b,
the use of NSPR_LDFLAGS was inadvertently copied to
js/src/configure.in.

Comment 1

[Wan-Teh Chang]

Attachment: Don't add -Wl,-z,text -Wl,--build-id to both LDFLAGS and NSPR_LDFLAGS

Steve: I cc'ed you on this patch because you are moving this
code to a new file in bug 975011.

The second issue is that the linker flags -Wl,-z,text -Wl,--build-id
passed to NSPR are duplicated, because these flags are added to both
LDFLAGS and NSPR_LDFLAGS, and we pass "$LDFLAGS $NSPR_LDFLAGS" to NSPR
like this:

    export LDFLAGS="$LDFLAGS $NSPR_LDFLAGS"
    export CFLAGS="$CFLAGS $MOZ_FRAMEPTR_FLAGS"

    AC_OUTPUT_SUBDIRS(nsprpub)

There are two ways to fix this. In this patch, I don't add a linker
flag to NSPR_LDFLAGS if it's already added to LDFLAGS. This fix is
simpler, but has the downside that some linker flags in LDFLAGS
could be inappropriate or irrelevant to NSPR, for example,
-static-libstdc++.

The other way to fix this is to pass only $NSPR_LDFLAGS to NSPR.
This requires inspecting how configure.in builds LDFLAGS and
adding any applicable linker flags to NSPR_LDFLAGS.

Comment 2

[Wan-Teh Chang]

Comment on attachment 8390582 [details] [diff] [review]
Don't add -Wl,-z,text -Wl,--build-id to both LDFLAGS and NSPR_LDFLAGS

Review of attachment 8390582 [details] [diff] [review]:
-----------------------------------------------------------------

I only see the second issue in the B2G {ICS,JB,KK} Emulator builds.
So I guess this issue only occurs when $direct_nspr_config is 1,
and so this patch seems incorrect in general.

Comment 3

[Wan-Teh Chang]

Comment on attachment 8390582 [details] [diff] [review]
Don't add -Wl,-z,text -Wl,--build-id to both LDFLAGS and NSPR_LDFLAGS

Review of attachment 8390582 [details] [diff] [review]:
-----------------------------------------------------------------

The direct_nspr_config variable was added in this checkin:
http://hg.mozilla.org/mozilla-central/diff/27a6377f1e17/configure.in

So this issue was probably introduced then.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8390565 [details] [diff] [review]
Remove the unused NSPR_LDFLAGS variable from js/src/configure.in

Review of attachment 8390565 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ -1128,5 @@
>      AC_MSG_CHECKING([for -z text option to ld])
>      _SAVE_LDFLAGS=$LDFLAGS
>      LDFLAGS="$LDFLAGS -Wl,-z,text"
> -    AC_TRY_LINK(,,AC_MSG_RESULT([yes])
> -                  [NSPR_LDFLAGS="$NSPR_LDFLAGS -Wl,-z,text"],

bug 975011 is going to use this.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8390582 [details] [diff] [review]
Don't add -Wl,-z,text -Wl,--build-id to both LDFLAGS and NSPR_LDFLAGS

Review of attachment 8390582 [details] [diff] [review]:
-----------------------------------------------------------------

Hum, yeah, that would break the normal case of direct_nspr_config being empty.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8390565 [details] [diff] [review]
Remove the unused NSPR_LDFLAGS variable from js/src/configure.in

Review of attachment 8390565 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ -1128,5 @@
>      AC_MSG_CHECKING([for -z text option to ld])
>      _SAVE_LDFLAGS=$LDFLAGS
>      LDFLAGS="$LDFLAGS -Wl,-z,text"
> -    AC_TRY_LINK(,,AC_MSG_RESULT([yes])
> -                  [NSPR_LDFLAGS="$NSPR_LDFLAGS -Wl,-z,text"],

If bug 975011 is going to use this, then js/src/configure.in
has a different bug: the AC_TRY_LINK statement below should
append -Wl,--build-id to NSPR_LDFLAGS. Do you agree?

Comment 7

[Wan-Teh Chang]

Attachment: js/src/configure.in should append -Wl,--build-id to NSPR_LDFLAGS

Depending on whether js/src/configure.in will use NSPR_LDFLAGS or not,
it seems that one of this patch and attachment 8390565 [details] [diff] [review] is needed.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8390582 [details] [diff] [review]
Don't add -Wl,-z,text -Wl,--build-id to both LDFLAGS and NSPR_LDFLAGS

Marked the patch obsolete because it would break the normal case of
direct_nspr_config being empty. I guess we'll live with the repeated
linker flags when $direct_nspr_config is 1.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8393016 [details] [diff] [review]
js/src/configure.in should append -Wl,--build-id to NSPR_LDFLAGS

Patch pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/776f0b2ed5ad

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/776f0b2ed5ad