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