[PATCH] basename buffer gets spammed in `ld'

[PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

There is a bug in `ld' that causes it to record crtn.o as a shared
library dependency in some cases on FreeBSD.

Permission to apply to head and 2.11 branch?

-- 
-- David  (obrien@FreeBSD.org)


Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/ld/ChangeLog,v
retrieving revision 1.423
diff -u -r1.423 ChangeLog
--- ChangeLog	2001/07/03 23:22:19	1.423
+++ ChangeLog	2001/07/09 23:31:29
@@ -1,3 +1,8 @@
+2001-07-09  David O'Brien  <obrien@FreeBSD.org>
+
+	* emultempl/elf32.em: Do not assuming that contents of the buffer returned
+	from basename function will remain unchanged accross other function calls.
+
 2001-07-03  H.J. Lu  <hjl@gnu.org>
 
 	* scripttempl/elf.sc (DYNAMIC_PAD): Revert the change made on
Index: emultempl/elf32.em
===================================================================
RCS file: /cvs/src/src/ld/emultempl/elf32.em,v
retrieving revision 1.49
diff -u -r1.49 elf32.em
--- elf32.em	2001/06/18 22:20:57	1.49
+++ elf32.em	2001/07/09 23:31:30
@@ -360,6 +360,9 @@
      DT_NEEDED entry for this file.  */
   bfd_elf_set_dt_needed_name (abfd, "");
 
+  /* Previos basename call was clobbered in lang_for_each_input_file.  */
+  soname = basename (abfd->filename);
+
   /* Tell the ELF backend that the output file needs a DT_NEEDED
      entry for this file if it is used to resolve the reference in
      a regular object.  */

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

Have you tried replacing basename() with libiberty's new lbasename()?
lbasename() always returns a pointer to the string you give it, which
shouldn't be clobbered unless someone renames a bfd.

If that works, it would be a preferrable solution.

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Mon, Jul 09, 2001 at 04:36:42PM -0700, David O'Brien wrote:
> There is a bug in `ld' that causes it to record crtn.o as a shared
> library dependency in some cases on FreeBSD.
> 
> Permission to apply to head and 2.11 branch?
> 

I am not against this patch. But there may be other places in the
soourceware tree where it assumes basename doesn't use a buffer.
I think libibery should check if the system's basename behaves
the same as the one in libibery before using the system one. I
can try to write a test for it.


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Mon, Jul 09, 2001 at 07:48:03PM -0400, DJ Delorie wrote:
> 
> Have you tried replacing basename() with libiberty's new lbasename()?

Nope.  I stayed consistent with the existing style of using basename().
(especially since I'd like to also commit this to the 2.11 branch)

I'd rather commit my patch and leave trying lbasename() to a
basename->lbasename sweep.

-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

> I think libibery should check if the system's basename behaves
> the same as the one in libibery before using the system one. I
> can try to write a test for it.

Libiberty provides lbasename() for when you know you want a fixed set
of semantics.  You can't test for functionality in libiberty, only for
the presence or absense of a function (think "cross compiler").

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Mon, Jul 09, 2001 at 08:19:20PM -0400, DJ Delorie wrote:
> 
> > I think libibery should check if the system's basename behaves
> > the same as the one in libibery before using the system one. I
> > can try to write a test for it.
> 
> Libiberty provides lbasename() for when you know you want a fixed set
> of semantics.  You can't test for functionality in libiberty, only for
> the presence or absense of a function (think "cross compiler").

It has been done all the time. For cross compile, just make a safer
choice. See

dnl AC_TRY_RUN(PROGRAM, [ACTION-IF-TRUE [, ACTION-IF-FALSE
dnl            [, ACTION-IF-CROSS-COMPILING]]])

I have been adding the missing ACTION-IF-CROSS-COMPILING to many
packacages.


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

Ok, then.  Approved for head.  I don't recall the branch checkin
rules, but if I'm allowed to approve it, it's approved for that also.
Nick - can we add a "current branches and checkin rules" section on
the binutils web page?

> Nope.  I stayed consistent with the existing style of using basename().
> (especially since I'd like to also commit this to the 2.11 branch)
> 
> I'd rather commit my patch and leave trying lbasename() to a
> basename->lbasename sweep.

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

> It has been done all the time. For cross compile, just make a safer
> choice.

I suppose.  In this case, though, we'd just use lbasename().

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Mon, Jul 09, 2001 at 08:34:35PM -0400, DJ Delorie wrote:
> 
> > It has been done all the time. For cross compile, just make a safer
> > choice.
> 
> I suppose.  In this case, though, we'd just use lbasename().

Ok with me.


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Mon, Jul 09, 2001 at 07:48:03PM -0400, DJ Delorie wrote:
> Have you tried replacing basename() with libiberty's new lbasename()?
> lbasename() always returns a pointer to the string you give it, which
> shouldn't be clobbered unless someone renames a bfd.

I'd like to update libiberty in the 2.11 branch based on top of tree (to
bring in lbasename).  Is that something you could do?  (IIRC you usually
handle the updates of libiberty w/in Binutils)
 
-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

> I'd like to update libiberty in the 2.11 branch based on top of tree (to
> bring in lbasename).  Is that something you could do?  (IIRC you usually
> handle the updates of libiberty w/in Binutils)

I don't normally keep head in sync with branches.  Usually, branches
are changed for specific reasons, not "just because".  The head sync
with gcc is done by a script, and I wouldn't unleash that on a branch.

Re: [PATCH] basename buffer gets spammed in `ld'

[Philip Blundell]

>Ok, then.  Approved for head.  I don't recall the branch checkin
>rules, but if I'm allowed to approve it, it's approved for that also.
>Nick - can we add a "current branches and checkin rules" section on
>the binutils web page?

Yes, it's okay for the branch.

p.


-- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.5 (GNU/Linux)
Comment: Exmh version 2.1.1 10/15/1999 (debian)

iD8DBQE7Sq+mVTLPJe9CT30RAjY6AJ95f4Q9RULF8EvbhvidoM+lVqF8wwCbB6Ie
VtPyaV93w55RFmFtu6U8kWU=
=9q5n
-----END PGP SIGNATURE-----

Re: [PATCH] basename buffer gets spammed in `ld'

[Philip Blundell]

>I'd like to update libiberty in the 2.11 branch based on top of tree (to
>bring in lbasename).  Is that something you could do?  (IIRC you usually
>handle the updates of libiberty w/in Binutils)

That would make me a bit nervous.  Is there any particular issue you want to 
address?  If it's just adding lbasename, I'd have thought there would be a 
less intrusive way to achieve that without updating the whole of libiberty.

p.


-- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.5 (GNU/Linux)
Comment: Exmh version 2.1.1 10/15/1999 (debian)

iD8DBQE7Sq/hVTLPJe9CT30RAtM4AJ4hdW0fKJUN8Enf/m5g2+0DIBCTFQCfZSY7
2DvEqDQOuu8K7XFRKwOI/CM=
=TYV0
-----END PGP SIGNATURE-----

Re: [PATCH] basename buffer gets spammed in `ld'

[Nick Clifton]

Hi DJ,

> Nick - can we add a "current branches and checkin rules" section on
> the binutils web page?

A good idea.  How about something like this:

	       --------- Branch Checkins ---------

    If a patch is approved for check in to the mainline sources,
    it can also be checked into the current release branch.
    Normally however only bug fixes should be applied to the branch.
    New features, new ports, etc, should be restricted to the
    mainline.  (Otherwise the burden of maintaing the branch in
    sync with the mainline becomes too great).  If you are
    uncertain as to whether a patch is appropriate for the branch,
    ask the branch maintainer.  This is:

        Philip Blundell  <philb@gnu.org>

Phil, do you agree ?  Would you prefer to have patches always
submiited for your approval before they are applied to the branch ?


Cheers
        Nick

Re: [PATCH] basename buffer gets spammed in `ld'

[Philip Blundell]

>A good idea.  How about something like this:

This seems fine.  No, there is no need for maintainers to ask me before 
checking patches in, if they're confident that the change is appropriate.

p.

-- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.5 (GNU/Linux)
Comment: Exmh version 2.1.1 10/15/1999 (debian)

iD8DBQE7Sv3MVTLPJe9CT30RAgAiAJ97FeATORTlQ/8AV+HDrxZJj0GYrwCgwmM1
KuScNO1BQty+2qyWavfDoy4=
=dD/+
-----END PGP SIGNATURE-----

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Tue, Jul 10, 2001 at 08:33:54AM +0100, Philip Blundell wrote:
> >I'd like to update libiberty in the 2.11 branch based on top of tree (to
> >bring in lbasename).  Is that something you could do?  (IIRC you usually
> >handle the updates of libiberty w/in Binutils)
> 
> That would make me a bit nervous.  Is there any particular issue you want to 
> address?  If it's just adding lbasename, I'd have thought there would be a 
> less intrusive way to achieve that without updating the whole of libiberty.

I didn't realize syncing libiberty from top of tree would be considered
risky.

Would you be open to me adding lbasename.c to the 2.11 branch + adding
the prototype to libiberty.h and libiberty/Makefile* ?
 
I have a better patch for the problem that uses lbasename() as DJ
recommended.

-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[Philip Blundell]

>I didn't realize syncing libiberty from top of tree would be considered
>risky.

It's not, particularly.  But it's bound to introduce changes that haven't been 
thoroughly tested, so it does carry some risk.  I'd rather not do that unless 
it brings a clear and compelling benefit.

>Would you be open to me adding lbasename.c to the 2.11 branch + adding
>the prototype to libiberty.h and libiberty/Makefile* ?

Yes.

p.


-- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.5 (GNU/Linux)
Comment: Exmh version 2.1.1 10/15/1999 (debian)

iD8DBQE7Sz7DVTLPJe9CT30RAkwEAJwIRApAnddM4WmESDbaV5fiHHJQ1ACguSBa
WafmJ05Mz/3E65av9HMdrA4=
=3One
-----END PGP SIGNATURE-----

Re: [PATCH] basename buffer gets spammed in `ld'

[Andrew Cagney]

> On Mon, Jul 09, 2001 at 07:48:03PM -0400, DJ Delorie wrote:
> 
>> 
>> Have you tried replacing basename() with libiberty's new lbasename()?
> 
> 
> Nope.  I stayed consistent with the existing style of using basename().
> (especially since I'd like to also commit this to the 2.11 branch)
> 
> I'd rather commit my patch and leave trying lbasename() to a
> basename->lbasename sweep.


Just a heads up.  I've approval in principal to change the lbasename() 
signature to:

	const char *lbasename (const char *);

If you're doing a s/basename/lbasename/ sweep please keep this in mind.

	Andrew

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Wed, Jul 11, 2001 at 01:55:57AM -0400, Andrew Cagney wrote:
> Just a heads up.  I've approval in principal to change the lbasename() 
> signature to:
> 
> 	const char *lbasename (const char *);
> 
> If you're doing a s/basename/lbasename/ sweep please keep this in mind.

I am (testing patch now).  When do you plan on changing the signature?
 
-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

I would like to check this patch into head and the 2.11 branch.
It is a better patch than my last one (which still had a "whole" in it).

-- 
-- David  (obrien@FreeBSD.org)

2001-07-11  David O'Brien  <obrien@FreeBSD.org>

	* emultempl/elf32.em: Make all basename calls safe by using
	lbasename() with non-changing buffers.


Index: ld/emultempl/elf32.em
===================================================================
RCS file: /cvs/src/src/ld/emultempl/elf32.em,v
retrieving revision 1.50
diff -u -r1.50 elf32.em
--- elf32.em	2001/07/10 00:38:16	1.50
+++ elf32.em	2001/07/12 18:35:01
@@ -155,7 +155,7 @@
 
   soname = bfd_elf_get_dt_soname (s->the_bfd);
   if (soname == NULL)
-    soname = basename (bfd_get_filename (s->the_bfd));
+    soname = lbasename (bfd_get_filename (s->the_bfd));
 
   for (l = global_vercheck_needed; l != NULL; l = l->next)
     {
@@ -237,7 +237,7 @@
 
   soname = bfd_elf_get_dt_soname (s->the_bfd);
   if (soname == NULL)
-    soname = basename (s->filename);
+    soname = lbasename (s->filename);
 
   if (strncmp (soname, global_needed->name,
 	       suffix - global_needed->name) == 0)
@@ -342,7 +342,7 @@
     einfo ("%F%P:%B: bfd_stat failed: %E\n", abfd);
 
   /* First strip off everything before the last '/'.  */
-  soname = basename (abfd->filename);
+  soname = lbasename (abfd->filename);
 
   if (trace_file_tries)
     info_msg (_("found %s at %s\n"), soname, name);
@@ -360,9 +360,6 @@
      DT_NEEDED entry for this file.  */
   bfd_elf_set_dt_needed_name (abfd, "");
 
-  /* Previos basename call was clobbered in lang_for_each_input_file.  */
-  soname = basename (abfd->filename);
-
   /* Tell the ELF backend that the output file needs a DT_NEEDED
      entry for this file if it is used to resolve the reference in
      a regular object.  */
@@ -967,7 +964,7 @@
       /* Rather than duplicating the logic above.  Just use the
 	 filename we recorded earlier.  */
 
-      filename = xstrdup (basename (entry->filename));
+      filename = lbasename (entry->filename);
       bfd_elf_set_dt_needed_name (entry->the_bfd, filename);
     }
 

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Thu, Jul 12, 2001 at 11:43:19AM -0700, David O'Brien wrote:
> @@ -967,7 +964,7 @@
>        /* Rather than duplicating the logic above.  Just use the
>  	 filename we recorded earlier.  */
>  
> -      filename = xstrdup (basename (entry->filename));
> +      filename = lbasename (entry->filename);
>        bfd_elf_set_dt_needed_name (entry->the_bfd, filename);
>      }
>  

Are you sure it is 100% safe to remove xstrdup?


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[DJ Delorie]

> 2001-07-11  David O'Brien  <obrien@FreeBSD.org>
> 
> 	* emultempl/elf32.em: Make all basename calls safe by using
> 	lbasename() with non-changing buffers.

Make sure the branch has lbasename in libiberty, else OK (unless Phil
B disagrees, of course ;).

> -      filename = xstrdup (basename (entry->filename));
> +      filename = lbasename (entry->filename);

Are you sure the xstrdup is no longer needed with lbasename?

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Thu, Jul 12, 2001 at 02:49:13PM -0400, DJ Delorie wrote:
> 
> > 2001-07-11  David O'Brien  <obrien@FreeBSD.org>
> > 
> > 	* emultempl/elf32.em: Make all basename calls safe by using
> > 	lbasename() with non-changing buffers.
> 
> Make sure the branch has lbasename in libiberty, else OK (unless Phil
> B disagrees, of course ;).
> 
> > -      filename = xstrdup (basename (entry->filename));
> > +      filename = lbasename (entry->filename);
> 
> Are you sure the xstrdup is no longer needed with lbasename?

Depends on why the xstrdup was there.  If it was there because of
basename()'s internal non-reentrant buff, then it is not needed.  As you
know that is how lbasename() differs from basename().

If entry->filename's contents are not static across the run, then it
would still be needed.  My read and test cases showed the patch was fine
(with no test regression for i386-known-freebsd4.3.  Since all the other
basename() calls needed to be wrapped with xstrdup it appeared to me this
basename() call was wrapped in xstrdup for the same reason.

HOWEVER, I would not mind at all a second set of eyes looking into it.

-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Thu, Jul 12, 2001 at 11:57:53AM -0700, David O'Brien wrote:
> would still be needed.  My read and test cases showed the patch was fine
> (with no test regression for i386-known-freebsd4.3.  Since all the other
> basename() calls needed to be wrapped with xstrdup it appeared to me this
> basename() call was wrapped in xstrdup for the same reason.

That said, if it is more expedient (or will put minds at rest), I'll
leave this line as-is and commit the other changes [with permission].

-- 
-- David  (obrien@FreeBSD.org)

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Thu, Jul 12, 2001 at 11:59:27AM -0700, David O'Brien wrote:
> On Thu, Jul 12, 2001 at 11:57:53AM -0700, David O'Brien wrote:
> > would still be needed.  My read and test cases showed the patch was fine
> > (with no test regression for i386-known-freebsd4.3.  Since all the other

It doesn't mean much.

> > basename() calls needed to be wrapped with xstrdup it appeared to me this
> > basename() call was wrapped in xstrdup for the same reason.
> 

I don't like such kind of change. Unless it is broken, don't fix
it :-).


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Thu, Jul 12, 2001 at 12:15:39PM -0700, H . J . Lu wrote:
> On Thu, Jul 12, 2001 at 11:59:27AM -0700, David O'Brien wrote:
> > On Thu, Jul 12, 2001 at 11:57:53AM -0700, David O'Brien wrote:
> > > would still be needed.  My read and test cases showed the patch was fine
> > > (with no test regression for i386-known-freebsd4.3.  Since all the other
> 
> It doesn't mean much.
> 
> > > basename() calls needed to be wrapped with xstrdup it appeared to me this
> > > basename() call was wrapped in xstrdup for the same reason.
> > 
> 
> I don't like such kind of change. Unless it is broken, don't fix
> it :-).

The current code is broken (ie, has bugs that are biting FreeBSD users).
Since the existing xstrdup(basename(...)) call isn't one of the problems,
and I changed it just for consistancy; I've taken it out.  Here is the
patch I would now like to commit.

-- 
-- David  (obrien@FreeBSD.org)


Index: elf32.em
===================================================================
RCS file: /cvs/src/src/ld/emultempl/elf32.em,v
retrieving revision 1.51
diff -u -r1.51 elf32.em
--- elf32.em	2001/07/13 07:25:18	1.51
+++ elf32.em	2001/07/14 16:33:04
@@ -155,7 +155,7 @@
 
   soname = bfd_elf_get_dt_soname (s->the_bfd);
   if (soname == NULL)
-    soname = basename (bfd_get_filename (s->the_bfd));
+    soname = lbasename (bfd_get_filename (s->the_bfd));
 
   for (l = global_vercheck_needed; l != NULL; l = l->next)
     {
@@ -237,7 +237,7 @@
 
   soname = bfd_elf_get_dt_soname (s->the_bfd);
   if (soname == NULL)
-    soname = basename (s->filename);
+    soname = lbasename (s->filename);
 
   if (strncmp (soname, global_needed->name,
 	       suffix - global_needed->name) == 0)
@@ -342,7 +342,7 @@
     einfo ("%F%P:%B: bfd_stat failed: %E\n", abfd);
 
   /* First strip off everything before the last '/'.  */
-  soname = basename (abfd->filename);
+  soname = lbasename (abfd->filename);
 
   if (trace_file_tries)
     info_msg (_("found %s at %s\n"), soname, name);
@@ -360,9 +360,6 @@
      DT_NEEDED entry for this file.  */
   bfd_elf_set_dt_needed_name (abfd, "");
 
-  /* Previos basename call was clobbered in lang_for_each_input_file.  */
-  soname = basename (abfd->filename);
-
   /* Tell the ELF backend that the output file needs a DT_NEEDED
      entry for this file if it is used to resolve the reference in
      a regular object.  */

Re: [PATCH] basename buffer gets spammed in `ld'

[H . J . Lu]

On Sat, Jul 14, 2001 at 09:39:34AM -0700, David O'Brien wrote:
> > 
> > > > basename() calls needed to be wrapped with xstrdup it appeared to me this
> > > > basename() call was wrapped in xstrdup for the same reason.
> > > 
> > 
> > I don't like such kind of change. Unless it is broken, don't fix
> > it :-).
> 
> The current code is broken (ie, has bugs that are biting FreeBSD users).

My comment was for the xstrdup change, which is not broken. If you go
back, you can see it is the part of your patch we are discussing. 


H.J.

Re: [PATCH] basename buffer gets spammed in `ld'

[David O'Brien]

On Sat, Jul 14, 2001 at 09:47:50AM -0700, H . J . Lu wrote:
> On Sat, Jul 14, 2001 at 09:39:34AM -0700, David O'Brien wrote:
> > > 
> > > > > basename() calls needed to be wrapped with xstrdup it appeared to me this
> > > > > basename() call was wrapped in xstrdup for the same reason.
> > > > 
> > > 
> > > I don't like such kind of change. Unless it is broken, don't fix
> > > it :-).
> > 
> > The current code is broken (ie, has bugs that are biting FreeBSD users).
> 
> My comment was for the xstrdup change, which is not broken. If you go
> back, you can see it is the part of your patch we are discussing. 

Why are you still discussing the older patch vs. the one that was at the
bottom of the email you replied to?
 
-- 
-- David  (obrien@FreeBSD.org)