[RESEND][PATCH] ld: -rpath must search under sysroot

[RESEND][PATCH] ld: -rpath must search under sysroot

[Andy Ross]

The -rpath argument would search the host filesystem for libraries,
even when a sysroot was defined.  For cross toolchains with targets
compatible with the host architecture this can find incorrect
libraries.  Leave -rpath-link unmodified, as build systems in the wild
are already using this to point to host directories.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 ld/emultempl/elf32.em | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
index de51050..35e0e7e 100644
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1263,9 +1263,13 @@ fragment <<EOF
 EOF
 if [ "x${USE_LIBPATH}" = xyes ] ; then
 fragment <<EOF
-	  if (gld${EMULATION_NAME}_search_needed (command_line.rpath,
-						  &n, force))
-	    break;
+	  if (command_line.rpath) {
+	    char *tmprp = gld${EMULATION_NAME}_add_sysroot (command_line.rpath);
+	    found = gld${EMULATION_NAME}_search_needed (tmprp, &n, force);
+	    free(tmprp);
+	    if (found)
+	      break;
+	  }
 EOF
 fi
 if [ "x${NATIVE}" = xyes ] ; then
-- 
1.7.11.4

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Joseph S. Myers]

On Thu, 6 Sep 2012, Andy Ross wrote:

> diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
> index de51050..35e0e7e 100644
> --- a/ld/emultempl/elf32.em
> +++ b/ld/emultempl/elf32.em
> @@ -1263,9 +1263,13 @@ fragment <<EOF
>  EOF
>  if [ "x${USE_LIBPATH}" = xyes ] ; then
>  fragment <<EOF
> -	  if (gld${EMULATION_NAME}_search_needed (command_line.rpath,
> -						  &n, force))
> -	    break;
> +	  if (command_line.rpath) {
> +	    char *tmprp = gld${EMULATION_NAME}_add_sysroot (command_line.rpath);
> +	    found = gld${EMULATION_NAME}_search_needed (tmprp, &n, force);
> +	    free(tmprp);
> +	    if (found)
> +	      break;
> +	  }

This looks like it may change what happens when there is no RPATH option 
at all.  Previously, gld${EMULATION_NAME}_search_needed would still be 
called even if command_line.rpath is NULL, and it in turn has logic:

  if (name[0] == '/')
    return gld${EMULATION_NAME}_try_needed (n, force);

  if (path == NULL || *path == '\0')
    return FALSE;

which handles the name[0] == '/' case *before* returning in the NULL rpath 
case.  Is this an intentional change?  If so, I think you need to give 
more explanation of why it's OK to change what the function does in this 
case (no -rpath option, name[0] == '/').

Note for coding style that the opening '{' goes on the line after the 
"if", appropriately indented, and that a space is needed after "free".

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Joseph S. Myers]

On Wed, 12 Sep 2012, Andy Ross wrote:

> > which handles the name[0] == '/' case *before* returning in the NULL rpath
> > case.  Is this an intentional change?
> 
> Indeed it was not, thanks.  Fixed to restore original behavior absent
> an rpath argument, and hopefully for whitespace conventions.

Thanks.  This revised version looks OK to me, but I can't approve it.  
(And it doesn't seem to have reached the mailing list yet for some 
reason.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Joseph S. Myers]

On Wed, 12 Sep 2012, Andy Ross wrote:

> On 09/12/2012 11:57 AM, Joseph S. Myers wrote:
> > Thanks.  This revised version looks OK to me, but I can't approve it.
> > (And it doesn't seem to have reached the mailing list yet for some
> > reason.)
> 
> Hm...
> 
>     $ dig -t MX sourceware.org
> 
>     ; <<>> DiG 9.5.0-P2.1 <<>> -t MX sourceware.org
>     ;; global options:  printcmd
>     ;; Got answer:
>     ;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 13352
>     ;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0
> 
>     ;; QUESTION SECTION:
>     ;sourceware.org.			IN	MX
> 
>     ;; Query time: 24 msec
>     ;; SERVER: 127.0.0.1#53(127.0.0.1)
>     ;; WHEN: Wed Sep 12 12:31:33 2012
>     ;; MSG SIZE  rcvd: 32
> 
> Needless to say, it's still sitting in the queue on my mail host with
> a DNS error.  Does this need to be reported to someone?

All four name servers appear to be reporting the MX record correctly for 
me.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Mike Frysinger]

On Thu, Sep 6, 2012 at 12:37 PM, Andy Ross wrote:
> The -rpath argument would search the host filesystem for libraries,
> even when a sysroot was defined.  For cross toolchains with targets
> compatible with the host architecture this can find incorrect
> libraries.  Leave -rpath-link unmodified, as build systems in the wild
> are already using this to point to host directories.

thanks.  this has been causing me some headaches as documented here:
http://crosbug.com/21469
might be good to note that gold is behaving differently from ld.bfd.
-mike

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Andy Ross]

On 09/12/2012 11:57 AM, Joseph S. Myers wrote:
> Thanks.  This revised version looks OK to me, but I can't approve it.
> (And it doesn't seem to have reached the mailing list yet for some
> reason.)

Hm...

     $ dig -t MX sourceware.org

     ; <<>> DiG 9.5.0-P2.1 <<>> -t MX sourceware.org
     ;; global options:  printcmd
     ;; Got answer:
     ;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 13352
     ;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0

     ;; QUESTION SECTION:
     ;sourceware.org.			IN	MX

     ;; Query time: 24 msec
     ;; SERVER: 127.0.0.1#53(127.0.0.1)
     ;; WHEN: Wed Sep 12 12:31:33 2012
     ;; MSG SIZE  rcvd: 32

Needless to say, it's still sitting in the queue on my mail host with
a DNS error.  Does this need to be reported to someone?

Andy

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Andy Ross]

[Apologies for the delay, I missed this over the weekend.]

On 09/07/2012 02:31 PM, Joseph S. Myers wrote:
> On Thu, 6 Sep 2012, Andy Ross wrote:
> This looks like it may change what happens when there is no RPATH option
> at all.  Previously, gld${EMULATION_NAME}_search_needed would still be
> called even if command_line.rpath is NULL, and it in turn has logic:
> [...]
>
> which handles the name[0] == '/' case *before* returning in the NULL rpath
> case.  Is this an intentional change?

Indeed it was not, thanks.  Fixed to restore original behavior absent
an rpath argument, and hopefully for whitespace conventions.

Andy

 From 45397794df0b8daf45c924838c406a6d70224db9 Mon Sep 17 00:00:00 2001
From: Andy Ross <andy.ross@windriver.com>
Date: Tue, 21 Aug 2012 10:50:55 -0700
Subject: [PATCH] ld: -rpath must search under sysroot

The -rpath argument would search the host filesystem for libraries,
even when a sysroot was defined.  For cross toolchains with targets
compatible with the host architecture this can find incorrect
libraries.  Leave -rpath-link unmodified, as build systems in the wild
are already using this to point to host directories.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
  ld/emultempl/elf32.em | 13 ++++++++++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
index de51050..36d08f2 100644
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1263,9 +1263,16 @@ fragment <<EOF
  EOF
  if [ "x${USE_LIBPATH}" = xyes ] ; then
  fragment <<EOF
-	  if (gld${EMULATION_NAME}_search_needed (command_line.rpath,
-						  &n, force))
-	    break;
+	  if (command_line.rpath)
+	    {
+	      char *tmprp = gld${EMULATION_NAME}_add_sysroot (command_line.rpath);
+	      found = gld${EMULATION_NAME}_search_needed (tmprp, &n, force);
+	      free (tmprp);
+	      if (found)
+		break;
+	    }
+	  else if (gld${EMULATION_NAME}_search_needed (NULL, &n, force))
+	      break;
  EOF
  fi
  if [ "x${NATIVE}" = xyes ] ; then
-- 
1.7.11.4

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Andy Ross]

On 09/12/2012 12:36 PM, Joseph S. Myers wrote:
>> Needless to say, it's still sitting in the queue on my mail host with
>> a DNS error.  Does this need to be reported to someone?
>
> All four name servers appear to be reporting the MX record correctly for
> me.

Odd.  I just checked from three networks (Linode, Amazon EC2, and my
home box on Comcast) and got nothing on any.  Maybe you have it
cached?  They're in the queue anyway and will hopefully pop out
eventually.

Andy

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Alan Modra]

On Wed, Sep 12, 2012 at 11:20:48AM -0700, Andy Ross wrote:
> The -rpath argument would search the host filesystem for libraries,
> even when a sysroot was defined.  For cross toolchains with targets
> compatible with the host architecture this can find incorrect
> libraries.  Leave -rpath-link unmodified, as build systems in the wild
> are already using this to point to host directories.

I agree that your patch makes sense, but I hesitate to apply the
change for two reasons:
a) It's odd to have -rpath add the sysroot but -rpath-link not do so.
b) I wonder how many people already add the sysroot to their -rpath
argument, and would have their builds broken by this change.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1729 bytes --]

On Monday 22 October 2012 01:29:59 Alan Modra wrote:
> On Wed, Sep 12, 2012 at 11:20:48AM -0700, Andy Ross wrote:
> > The -rpath argument would search the host filesystem for libraries,
> > even when a sysroot was defined.  For cross toolchains with targets
> > compatible with the host architecture this can find incorrect
> > libraries.  Leave -rpath-link unmodified, as build systems in the wild
> > are already using this to point to host directories.
> 
> I agree that your patch makes sense, but I hesitate to apply the
> change for two reasons:
> a) It's odd to have -rpath add the sysroot but -rpath-link not do so.

agreed.  we should update -rpath-link to search the sysroot too.

> b) I wonder how many people already add the sysroot to their -rpath
> argument, and would have their builds broken by this change.

i think we already have that problem since gold is behaving the correct way.

i'm also not sure who would actually be relying on this behavior.  i'd hazard 
a guess that the vast majority of people using a sysroot are cross-compiling 
which means the host paths contain incompatible formats.

in the case where they're linking code with the full sysroot path in -rpath 
(which doesn't make much sense in the first place since the sysroot path 
generally has no meaning in the target system), it's trivial for them to add a 
-L flag to the full sysroot path, and doing so would be backwards compatible.

i think the cases where people are doing it The Right Way are much more common 
than people doing it the wrong way.  especially since people doing the right 
way have no recourse (other than to not use -rpath) while people doing the 
wrong way do (just add -L).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RESEND][PATCH] ld: -rpath must search under sysroot

[Andy Ross]

On 10/21/2012 11:14 PM, Mike Frysinger wrote:
> On Monday 22 October 2012 01:29:59 Alan Modra wrote:
>> I agree that your patch makes sense, but I hesitate to apply the
>> change for two reasons:
>> a) It's odd to have -rpath add the sysroot but -rpath-link not do so.
>
> agreed.  we should update -rpath-link to search the sysroot too.

Actually, this verifiably breaks builds in the wild.  The first
version I posted did exactly that, but I quickly found spots where
projects are passing a relative path to -rpath-link (e.g. ../lib)
which will pick up even more host-polluting libraries.  Basically
people seem to have figured out that -rpath-link doesn't honor sysroot
and have already worked around it.  Changing this seems like it would
be a needless (if perhaps less surprising) incompatibility.

>> b) I wonder how many people already add the sysroot to their -rpath
>> argument, and would have their builds broken by this change.
>
> i think we already have that problem since gold is behaving the correct way.
>
> i'm also not sure who would actually be relying on this behavior.  i'd hazard
> a guess that the vast majority of people using a sysroot are cross-compiling
> which means the host paths contain incompatible formats.

The case I hit was Yocto/Openembedded, where everything is built with
a cross toolchain but for an architecture which often matches the
host.  Another post in this thread pointed out that ChromiumOS hit the
bug for similar reasons.

Really, the current behavior can't possibly work reliably in the face
of these toolchains.  Consider the case of a project that drops
RPATH-located libraries in "/my/plugin/dir".  Now install it on the
host, and try to link a different version using a cross toolchian.
You can't, because you can't set the RPATH without hitting the wrong
libraries.

Andy