Failure with PR16566 new test on Mingw hosts

Failure with PR16566 new test on Mingw hosts

[Clément Chigot]

Hi Nick,

I've a weird failure on the new test introduced by 496917ce46 and its
-print-map-locals. On mingw, when cross-linking to any ELF targets,
adding the option results in a crash when writing the output
executable:
  | $ ./ld-new.exe   -o tmpdir/map-address  -T .../map-address.t
tmpdir/map-address.o  \
  |    -Map=tmpdir/map-locals.map --print-map-locals
  | ld-new.exe: tmpdir/map-address: final close failed: No space left on device

It seems that a call to fwrite is failing with EPIPE, when writing the strtab.
  | (gdb) bt
  | #0  cache_bwrite (abfd=0x1014120,
  |     from=0x7ff769108395 <__PRETTY_FUNCTION__.0+85>, nbytes=1)
  |     at ./binutils-gdb/bfd/cache.c:383
  | #1  0x00007ff768f2c88b in bfd_bwrite (
  |     ptr=0x7ff769108395 <__PRETTY_FUNCTION__.0+85>, size=1, abfd=0x1014120)
  |     at ./binutils-gdb/bfd/bfdio.c:316
  | #2  0x00007ff768fa4265 in _bfd_elf_strtab_emit (abfd=0x1014120,
tab=0x102c9a0)
  |     at ./binutils-gdb/bfd/elf-strtab.c:318
  | #3  0x00007ff768f6f9fd in _bfd_elf_write_object_contents (abfd=0x1014120)
  |     at ./binutils-gdb/bfd/elf.c:6743
  | #4  0x00007ff768f3a8d4 in bfd_close (abfd=0x1014120)
  |     at ./binutils-gdb/bfd/opncls.c:887
  | #5  0x00007ff768f019f4 in main (argc=8, argv=0x37c3dd0)
  |     at ./binutils-gdb/ld/ldmain.c:568

I don't know why but this seems to be triggered by the new call to
bfd_canonicalize_symtab introduced when we are logging the local
symbols [1].
Any idea about what's going on ?

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/ldlang.c;hb=HEAD#l4918

Thanks,
Clément

Re: Failure with PR16566 new test on Mingw hosts

[Nick Clifton]

Hi Clément,

> I've a weird failure on the new test introduced by 496917ce46 and its -print-map-locals. On mingw, when cross-linking to any ELF targets,

So this happens when the *host* is a mingw system, rather than, say,
a Linux system.  That is strange.  Why would the host matter ?

> adding the option results in a crash when writing the output executable:

> It seems that a call to fwrite is failing with EPIPE, when writing the strtab.

> I don't know why but this seems to be triggered by the new call to bfd_canonicalize_symtab introduced when we are logging the local symbols [1]. Any idea about what's
> going on ?

No. :-(  It is definitely weird.

Do you know if the problem is target specific, ie only happening
for certain ELF targets ?

Maybe this is a memory problem.  Possibly the code is using freed
memory, or something like that.  And on Linux hosts the freed memory
is untouched but on mingw hosts the memory is recycled and used for
something else ?  If you run the command under a program like valgrind
to you get any more information ?  Or maybe compiling with address
sanitization enabled ?

Cheers
   Nick

Re: Failure with PR16566 new test on Mingw hosts

[Clément Chigot]

Hi Nick,

On Tue, May 23, 2023 at 1:06 PM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi Clément,
>
> > I've a weird failure on the new test introduced by 496917ce46 and its -print-map-locals. On mingw, when cross-linking to any ELF targets,
>
> So this happens when the *host* is a mingw system, rather than, say,
> a Linux system.  That is strange.  Why would the host matter ?

No idea. But it can be many things: a syscall failing somewhere but
where we aren't checking the return code, a bfd feature having Windows
specific code, whatever.

It can be also linked to my setup which is a mix between mingw and
cygwin. By the way, would it be possible for you to check that you're
able to reproduce it as well please ? That would allow me to clear
this possibility.

> > adding the option results in a crash when writing the output executable:
>
> > It seems that a call to fwrite is failing with EPIPE, when writing the strtab.
>
> > I don't know why but this seems to be triggered by the new call to bfd_canonicalize_symtab introduced when we are logging the local symbols [1]. Any idea about what's
> > going on ?
>
> No. :-(  It is definitely weird.
>
> Do you know if the problem is target specific, ie only happening
> for certain ELF targets ?

It's happening on all my ELF targets (at least arm/aarch64, ppc/ppc64,
leon3, riscv32/64).

> Maybe this is a memory problem.  Possibly the code is using freed
> memory, or something like that.  And on Linux hosts the freed memory
> is untouched but on mingw hosts the memory is recycled and used for
> something else ?  If you run the command under a program like valgrind
> to you get any more information ?  Or maybe compiling with address
> sanitization enabled ?

I'll try that and continue investigating. Thanks for the feedback.

Clément

Re: Failure with PR16566 new test on Mingw hosts

[Alan Modra]

On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> I don't know why but this seems to be triggered by the new call to
> bfd_canonicalize_symtab introduced when we are logging the local
> symbols [1].
> Any idea about what's going on ?

The output file is opened "w+b".  ldwrite() will cause some of the
file to be written, but not all is output until bfd_close().  So
reading the symbols means the file IO is write, read, write.  Wild
guess: no fseek or similar between changes in direction?  bfd_seek
doesn't call fseek if the file position doesn't move.

> [1] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/ldlang.c;hb=HEAD#l4918
> 
> Thanks,
> Clément

-- 
Alan Modra
Australia Development Lab, IBM

Re: Failure with PR16566 new test on Mingw hosts

[Clément Chigot]

Hey Alan

On Tue, May 23, 2023 at 3:11 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> > I don't know why but this seems to be triggered by the new call to
> > bfd_canonicalize_symtab introduced when we are logging the local
> > symbols [1].
> > Any idea about what's going on ?
>
> The output file is opened "w+b".  ldwrite() will cause some of the
> file to be written, but not all is output until bfd_close().  So
> reading the symbols means the file IO is write, read, write.  Wild
> guess: no fseek or similar between changes in direction?  bfd_seek
> doesn't call fseek if the file position doesn't move.

Nice guess ! It seems to be the issue. Forcing a fseek is fixing it.
I have a quick workaround which resets the value of
"link_info.output_bfd->where" after the new bfd_canonicalize_symtab
call.

However, I'm not sure what that means. Is it possible to write/read a
bfd without using bfd_fwrite, bfd_fread ? I didn't find any. But that
would create a gap between the "where" field and the actual file
pointer.
Otherwise, we should probably enforce a fseek when the direction has
changed. Even if Windows docs says that both fread and fwrite should
update the file pointer something might be wrong here.

Another interesting thing I've discovered. The call to
bfd_canonicalize_symtab allocates some contents for the strtab section
header which is then written when the bfd is being closed. In
elf.c:_bfd_elf_write_object_contents, i_shdrp[<strtab_count>].contents
is not anymore NULL.
AFAICT, that part was already written before during the final link
part. I'm not sure if it has any impact, we are probably writing the
same stuff again. But I think it's worth mentioning it.

Clément

Re: Failure with PR16566 new test on Mingw hosts

[Alan Modra]

On Tue, May 23, 2023 at 05:31:37PM +0200, Clément Chigot wrote:
> Hey Alan
> 
> On Tue, May 23, 2023 at 3:11 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> > > I don't know why but this seems to be triggered by the new call to
> > > bfd_canonicalize_symtab introduced when we are logging the local
> > > symbols [1].
> > > Any idea about what's going on ?
> >
> > The output file is opened "w+b".  ldwrite() will cause some of the
> > file to be written, but not all is output until bfd_close().  So
> > reading the symbols means the file IO is write, read, write.  Wild
> > guess: no fseek or similar between changes in direction?  bfd_seek
> > doesn't call fseek if the file position doesn't move.
> 
> Nice guess ! It seems to be the issue. Forcing a fseek is fixing it.
> I have a quick workaround which resets the value of
> "link_info.output_bfd->where" after the new bfd_canonicalize_symtab
> call.

The following is a better fix, I think.  I didn't see much impact on
"time make check" by doing this.

diff --git a/bfd/bfdio.c b/bfd/bfdio.c
index 75a3309c582..22c39a7b0cc 100644
--- a/bfd/bfdio.c
+++ b/bfd/bfdio.c
@@ -456,10 +456,6 @@ bfd_seek (bfd *abfd, file_ptr position, int direction)
   if (direction != SEEK_CUR)
     position += offset;
 
-  if ((direction == SEEK_CUR && position == 0)
-      || (direction == SEEK_SET && (ufile_ptr) position == abfd->where))
-    return 0;
-
   result = abfd->iovec->bseek (abfd, position, direction);
   if (result != 0)
     {

> However, I'm not sure what that means. Is it possible to write/read a
> bfd without using bfd_fwrite, bfd_fread ? I didn't find any. But that
> would create a gap between the "where" field and the actual file
> pointer.
> Otherwise, we should probably enforce a fseek when the direction has
> changed. Even if Windows docs says that both fread and fwrite should
> update the file pointer something might be wrong here.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen
says fseek (or fsetpos or rewind) is necessary when changing from
reading to writing, and the same or fflush when reading after writing.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
says the same.

> Another interesting thing I've discovered. The call to
> bfd_canonicalize_symtab allocates some contents for the strtab section
> header which is then written when the bfd is being closed. In
> elf.c:_bfd_elf_write_object_contents, i_shdrp[<strtab_count>].contents
> is not anymore NULL.
> AFAICT, that part was already written before during the final link
> part. I'm not sure if it has any impact, we are probably writing the
> same stuff again. But I think it's worth mentioning it.

Yes, I think it would be safer to close the output file and reopen for
lang_map if reporting local syms per section in the map is important.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Failure with PR16566 new test on Mingw hosts

[Alan Modra]

On Wed, May 24, 2023 at 09:37:25AM +0930, Alan Modra wrote:
> Yes, I think it would be safer to close the output file and reopen for
> lang_map

Huh, and that won't work due to all the pointers into the output bfd
from linker statement lists.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Failure with PR16566 new test on Mingw hosts

[Clément Chigot]

On Wed, May 24, 2023 at 2:07 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Tue, May 23, 2023 at 05:31:37PM +0200, Clément Chigot wrote:
> > Hey Alan
> >
> > On Tue, May 23, 2023 at 3:11 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> > > > I don't know why but this seems to be triggered by the new call to
> > > > bfd_canonicalize_symtab introduced when we are logging the local
> > > > symbols [1].
> > > > Any idea about what's going on ?
> > >
> > > The output file is opened "w+b".  ldwrite() will cause some of the
> > > file to be written, but not all is output until bfd_close().  So
> > > reading the symbols means the file IO is write, read, write.  Wild
> > > guess: no fseek or similar between changes in direction?  bfd_seek
> > > doesn't call fseek if the file position doesn't move.
> >
> > Nice guess ! It seems to be the issue. Forcing a fseek is fixing it.
> > I have a quick workaround which resets the value of
> > "link_info.output_bfd->where" after the new bfd_canonicalize_symtab
> > call.
>
> The following is a better fix, I think.  I didn't see much impact on
> "time make check" by doing this.
>
> diff --git a/bfd/bfdio.c b/bfd/bfdio.c
> index 75a3309c582..22c39a7b0cc 100644
> --- a/bfd/bfdio.c
> +++ b/bfd/bfdio.c
> @@ -456,10 +456,6 @@ bfd_seek (bfd *abfd, file_ptr position, int direction)
>    if (direction != SEEK_CUR)
>      position += offset;
>
> -  if ((direction == SEEK_CUR && position == 0)
> -      || (direction == SEEK_SET && (ufile_ptr) position == abfd->where))
> -    return 0;
> -
>    result = abfd->iovec->bseek (abfd, position, direction);
>    if (result != 0)
>      {

That was my first fix too, but I thought it would have reduced the
performance too much. If you say that it looks ok then fine.
Do you think it makes sense to wrap it by a #ifdef __WIN32 ? Linux
(and probably other Unix) doesn't have the issue.

I'm also wondering if by removing this code, we could not remove
"abfd->where" as well. It looks like its first aim was to mimic the
file pointer. But if we force a fseek for every fwrite/fread we might
not have needs of it anymore.

> > However, I'm not sure what that means. Is it possible to write/read a
> > bfd without using bfd_fwrite, bfd_fread ? I didn't find any. But that
> > would create a gap between the "where" field and the actual file
> > pointer.
> > Otherwise, we should probably enforce a fseek when the direction has
> > changed. Even if Windows docs says that both fread and fwrite should
> > update the file pointer something might be wrong here.
>
> https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen
> says fseek (or fsetpos or rewind) is necessary when changing from
> reading to writing, and the same or fflush when reading after writing.
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
> says the same.
>
> > Another interesting thing I've discovered. The call to
> > bfd_canonicalize_symtab allocates some contents for the strtab section
> > header which is then written when the bfd is being closed. In
> > elf.c:_bfd_elf_write_object_contents, i_shdrp[<strtab_count>].contents
> > is not anymore NULL.
> > AFAICT, that part was already written before during the final link
> > part. I'm not sure if it has any impact, we are probably writing the
> > same stuff again. But I think it's worth mentioning it.
>
> Yes, I think it would be safer to close the output file and reopen for
> lang_map if reporting local syms per section in the map is important.
>
> --
> Alan Modra
> Australia Development Lab, IBM

Re: Failure with PR16566 new test on Mingw hosts

[Alan Modra]

On Wed, May 24, 2023 at 10:56:22AM +0200, Clément Chigot wrote:
> On Wed, May 24, 2023 at 2:07 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Tue, May 23, 2023 at 05:31:37PM +0200, Clément Chigot wrote:
> > > Hey Alan
> > >
> > > On Tue, May 23, 2023 at 3:11 PM Alan Modra <amodra@gmail.com> wrote:
> > > >
> > > > On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> > > > > I don't know why but this seems to be triggered by the new call to
> > > > > bfd_canonicalize_symtab introduced when we are logging the local
> > > > > symbols [1].
> > > > > Any idea about what's going on ?
> > > >
> > > > The output file is opened "w+b".  ldwrite() will cause some of the
> > > > file to be written, but not all is output until bfd_close().  So
> > > > reading the symbols means the file IO is write, read, write.  Wild
> > > > guess: no fseek or similar between changes in direction?  bfd_seek
> > > > doesn't call fseek if the file position doesn't move.
> > >
> > > Nice guess ! It seems to be the issue. Forcing a fseek is fixing it.
> > > I have a quick workaround which resets the value of
> > > "link_info.output_bfd->where" after the new bfd_canonicalize_symtab
> > > call.
> >
> > The following is a better fix, I think.  I didn't see much impact on
> > "time make check" by doing this.
> >
> > diff --git a/bfd/bfdio.c b/bfd/bfdio.c
> > index 75a3309c582..22c39a7b0cc 100644
> > --- a/bfd/bfdio.c
> > +++ b/bfd/bfdio.c
> > @@ -456,10 +456,6 @@ bfd_seek (bfd *abfd, file_ptr position, int direction)
> >    if (direction != SEEK_CUR)
> >      position += offset;
> >
> > -  if ((direction == SEEK_CUR && position == 0)
> > -      || (direction == SEEK_SET && (ufile_ptr) position == abfd->where))
> > -    return 0;
> > -
> >    result = abfd->iovec->bseek (abfd, position, direction);
> >    if (result != 0)
> >      {
> 
> That was my first fix too, but I thought it would have reduced the
> performance too much. If you say that it looks ok then fine.
> Do you think it makes sense to wrap it by a #ifdef __WIN32 ? Linux
> (and probably other Unix) doesn't have the issue.

No, it could well be a unix problem too.

> I'm also wondering if by removing this code, we could not remove
> "abfd->where" as well. It looks like its first aim was to mimic the
> file pointer. But if we force a fseek for every fwrite/fread we might
> not have needs of it anymore.

I think we might need abfd->where for cache.c closing and reopening of
files, so that reads and writes continue on from the same file
position as if the file was not closed.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Failure with PR16566 new test on Mingw hosts

[Clément Chigot]

On Wed, May 24, 2023 at 11:33 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, May 24, 2023 at 10:56:22AM +0200, Clément Chigot wrote:
> > On Wed, May 24, 2023 at 2:07 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Tue, May 23, 2023 at 05:31:37PM +0200, Clément Chigot wrote:
> > > > Hey Alan
> > > >
> > > > On Tue, May 23, 2023 at 3:11 PM Alan Modra <amodra@gmail.com> wrote:
> > > > >
> > > > > On Tue, May 23, 2023 at 10:48:06AM +0200, Clément Chigot via Binutils wrote:
> > > > > > I don't know why but this seems to be triggered by the new call to
> > > > > > bfd_canonicalize_symtab introduced when we are logging the local
> > > > > > symbols [1].
> > > > > > Any idea about what's going on ?
> > > > >
> > > > > The output file is opened "w+b".  ldwrite() will cause some of the
> > > > > file to be written, but not all is output until bfd_close().  So
> > > > > reading the symbols means the file IO is write, read, write.  Wild
> > > > > guess: no fseek or similar between changes in direction?  bfd_seek
> > > > > doesn't call fseek if the file position doesn't move.
> > > >
> > > > Nice guess ! It seems to be the issue. Forcing a fseek is fixing it.
> > > > I have a quick workaround which resets the value of
> > > > "link_info.output_bfd->where" after the new bfd_canonicalize_symtab
> > > > call.
> > >
> > > The following is a better fix, I think.  I didn't see much impact on
> > > "time make check" by doing this.
> > >
> > > diff --git a/bfd/bfdio.c b/bfd/bfdio.c
> > > index 75a3309c582..22c39a7b0cc 100644
> > > --- a/bfd/bfdio.c
> > > +++ b/bfd/bfdio.c
> > > @@ -456,10 +456,6 @@ bfd_seek (bfd *abfd, file_ptr position, int direction)
> > >    if (direction != SEEK_CUR)
> > >      position += offset;
> > >
> > > -  if ((direction == SEEK_CUR && position == 0)
> > > -      || (direction == SEEK_SET && (ufile_ptr) position == abfd->where))
> > > -    return 0;
> > > -
> > >    result = abfd->iovec->bseek (abfd, position, direction);
> > >    if (result != 0)
> > >      {
> >
> > That was my first fix too, but I thought it would have reduced the
> > performance too much. If you say that it looks ok then fine.
> > Do you think it makes sense to wrap it by a #ifdef __WIN32 ? Linux
> > (and probably other Unix) doesn't have the issue.
>
> No, it could well be a unix problem too.

Indeed.
Thanks for the patch anyway.

> > I'm also wondering if by removing this code, we could not remove
> > "abfd->where" as well. It looks like its first aim was to mimic the
> > file pointer. But if we force a fseek for every fwrite/fread we might
> > not have needs of it anymore.
>
> I think we might need abfd->where for cache.c closing and reopening of
> files, so that reads and writes continue on from the same file
> position as if the file was not closed.

Alright, better keep it as is then.