diff mbox series

[v2] ld/pdb: Report failures to write the PDB file

Message ID 20261002174508.1414-6-oleg.tolmatcev@gmail.com
State New
Headers
Series [v2] ld/pdb: Report failures to write the PDB file |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Oleg Tolmatcev Oct. 2, 2026, 5:45 p.m. UTC 
  The streams of a PDB file are only written out when it is closed, so
ignoring the return value of bfd_close means that write errors go
unreported: ld leaves a truncated PDB file behind and exits
successfully.

ld/
	* pdb.c (create_pdb_file): Check the return value of bfd_close.
---
 bfd/coffcode.h | 5 +++--
 ld/pdb.c       | 9 ++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Jan Beulich Oct. 5, 2026, 8:26 a.m. UTC | #1 
On 02.10.2026 19:45, Oleg Tolmatcev wrote:
> The streams of a PDB file are only written out when it is closed, so
> ignoring the return value of bfd_close means that write errors go
> unreported: ld leaves a truncated PDB file behind and exits
> successfully.

I continue to agree this wants correcting, yet ...

> --- a/bfd/coffcode.h
> +++ b/bfd/coffcode.h
> @@ -4325,8 +4325,9 @@ coff_write_object_contents (bfd * abfd)
>         but before header is written so we can update it to point to debug directory.  */
>      struct pe_tdata *pe = pe_data (abfd);
>  
> -    if (pe->build_id.after_write_object_contents != NULL)
> -      (*pe->build_id.after_write_object_contents) (abfd);
> +    if (pe->build_id.after_write_object_contents != NULL
> +	&& !(*pe->build_id.after_write_object_contents) (abfd))
> +      return false;

... there's more to propagating failure here than just the PDB writing. The
hook first and foremost writes the build ID. Likely it was never a good idea
to ignore the return value here, but the description then needs to cover this
aspect. Alternatively ...

> --- a/ld/pdb.c
> +++ b/ld/pdb.c
> @@ -5407,7 +5407,14 @@ create_pdb_file (bfd *abfd, const char *pdb_name, const unsigned char *guid)
>    ret = true;
>  
>  end:
> -  bfd_close (pdb);
> +  if (!bfd_close (pdb))
> +    {
> +      einfo (_("%P: cannot write PDB file: %E\n"));

... this could remain to be a warning, as v1 had it, just that now ...

> +      ret = false;
> +    }
> +
> +  if (!ret)
> +    unlink_if_ordinary (pdb_name);
>  
>    htab_delete (strings.hashmap);

... you properly delete the broken file. Let me know your preference, I'm
happy to make edits while committing whichever way you prefer.

Jan
Oleg Tolmatcev Oct. 5, 2026, 3:38 p.m. UTC | #2 
пн, 5 окт. 2026 г. в 10:26, Jan Beulich <jbeulich@suse.com>:
>
> On 02.10.2026 19:45, Oleg Tolmatcev wrote:
> > The streams of a PDB file are only written out when it is closed, so
> > ignoring the return value of bfd_close means that write errors go
> > unreported: ld leaves a truncated PDB file behind and exits
> > successfully.
>
> I continue to agree this wants correcting, yet ...
>
> > --- a/bfd/coffcode.h
> > +++ b/bfd/coffcode.h
> > @@ -4325,8 +4325,9 @@ coff_write_object_contents (bfd * abfd)
> >         but before header is written so we can update it to point to debug directory.  */
> >      struct pe_tdata *pe = pe_data (abfd);
> >
> > -    if (pe->build_id.after_write_object_contents != NULL)
> > -      (*pe->build_id.after_write_object_contents) (abfd);
> > +    if (pe->build_id.after_write_object_contents != NULL
> > +     && !(*pe->build_id.after_write_object_contents) (abfd))
> > +      return false;
>
> ... there's more to propagating failure here than just the PDB writing. The
> hook first and foremost writes the build ID. Likely it was never a good idea
> to ignore the return value here, but the description then needs to cover this
> aspect. Alternatively ...
>
> > --- a/ld/pdb.c
> > +++ b/ld/pdb.c
> > @@ -5407,7 +5407,14 @@ create_pdb_file (bfd *abfd, const char *pdb_name, const unsigned char *guid)
> >    ret = true;
> >
> >  end:
> > -  bfd_close (pdb);
> > +  if (!bfd_close (pdb))
> > +    {
> > +      einfo (_("%P: cannot write PDB file: %E\n"));
>
> ... this could remain to be a warning, as v1 had it, just that now ...
>
> > +      ret = false;
> > +    }
> > +
> > +  if (!ret)
> > +    unlink_if_ordinary (pdb_name);
> >
> >    htab_delete (strings.hashmap);
>
> ... you properly delete the broken file. Let me know your preference, I'm
> happy to make edits while committing whichever way you prefer.
>
> Jan

Thanks, I'd prefer to keep it an error.

Oleg
diff mbox series

Patch

diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index b291ac8d052..1984a0dc9c6 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -4325,8 +4325,9 @@  coff_write_object_contents (bfd * abfd)
        but before header is written so we can update it to point to debug directory.  */
     struct pe_tdata *pe = pe_data (abfd);
 
-    if (pe->build_id.after_write_object_contents != NULL)
-      (*pe->build_id.after_write_object_contents) (abfd);
+    if (pe->build_id.after_write_object_contents != NULL
+	&& !(*pe->build_id.after_write_object_contents) (abfd))
+      return false;
   }
 #endif
 
diff --git a/ld/pdb.c b/ld/pdb.c
index 46236fcbd4f..4b994579dd6 100644
--- a/ld/pdb.c
+++ b/ld/pdb.c
@@ -5407,7 +5407,14 @@  create_pdb_file (bfd *abfd, const char *pdb_name, const unsigned char *guid)
   ret = true;
 
 end:
-  bfd_close (pdb);
+  if (!bfd_close (pdb))
+    {
+      einfo (_("%P: cannot write PDB file: %E\n"));
+      ret = false;
+    }
+
+  if (!ret)
+    unlink_if_ordinary (pdb_name);
 
   htab_delete (strings.hashmap);