public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ld: Add --pdb option
@ 2022-10-03  1:43 Mark Harmstone
  2022-10-03  1:43 ` [PATCH 2/2] ld: Add minimal pdb generation Mark Harmstone
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Mark Harmstone @ 2022-10-03  1:43 UTC (permalink / raw)
  To: binutils; +Cc: Mark Harmstone

This patch adds the --pdb option to ld when linking PE files, which
augments the existing CodeView record used for build IDs by adding a PDB
filename. If no filename is provided, this defaults to the image name
with the extension replaced by "pdb".

---
 bfd/libpei.h               |  6 +++--
 bfd/peXXigen.c             | 30 ++++++++++++++++-----
 bfd/peicode.h              |  2 +-
 ld/emultempl/pe.em         | 49 +++++++++++++++++++++++++++++++----
 ld/emultempl/pep.em        | 47 ++++++++++++++++++++++++++++++---
 ld/testsuite/ld-pe/pdb.exp | 53 ++++++++++++++++++++++++++++++++++++++
 ld/testsuite/ld-pe/pdb1.s  |  5 ++++
 7 files changed, 173 insertions(+), 19 deletions(-)
 create mode 100644 ld/testsuite/ld-pe/pdb.exp
 create mode 100644 ld/testsuite/ld-pe/pdb1.s

diff --git a/bfd/libpei.h b/bfd/libpei.h
index 4aca024192c..8b53bd90e84 100644
--- a/bfd/libpei.h
+++ b/bfd/libpei.h
@@ -388,9 +388,11 @@ void _bfd_XX_get_symbol_info (bfd *, asymbol *, symbol_info *);
 bool _bfd_XXi_final_link_postscript (bfd *, struct coff_final_link_info *);
 void _bfd_XXi_swap_debugdir_in (bfd *, void *, void *);
 unsigned _bfd_XXi_swap_debugdir_out (bfd *, void *, void *);
-unsigned _bfd_XXi_write_codeview_record (bfd *, file_ptr, CODEVIEW_INFO *);
+unsigned _bfd_XXi_write_codeview_record
+  (bfd *, file_ptr, CODEVIEW_INFO *, const char *);
 CODEVIEW_INFO *_bfd_XXi_slurp_codeview_record
-  (bfd * abfd, file_ptr where, unsigned long length, CODEVIEW_INFO *cvinfo);
+  (bfd * abfd, file_ptr where, unsigned long length, CODEVIEW_INFO *cvinfo,
+   char **pdb);
 
 /* The following are needed only for ONE of pe or pei, but don't
    otherwise vary; peicode.h fixes up ifdefs but we provide the
diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index 5ab09387e72..8db188ce036 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -1134,7 +1134,8 @@ _bfd_XXi_swap_debugdir_out (bfd * abfd, void * inp, void * extp)
 }
 
 CODEVIEW_INFO *
-_bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length, CODEVIEW_INFO *cvinfo)
+_bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length, CODEVIEW_INFO *cvinfo,
+				char **pdb)
 {
   char buffer[256+1];
   bfd_size_type nread;
@@ -1174,6 +1175,9 @@ _bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length
       cvinfo->SignatureLength = CV_INFO_SIGNATURE_LENGTH;
       /* cvinfo->PdbFileName = cvinfo70->PdbFileName;  */
 
+      if (pdb)
+	*pdb = xstrdup (cvinfo70->PdbFileName);
+
       return cvinfo;
     }
   else if ((cvinfo->CVSignature == CVINFO_PDB20_CVSIGNATURE)
@@ -1185,6 +1189,9 @@ _bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length
       cvinfo->SignatureLength = 4;
       /* cvinfo->PdbFileName = cvinfo20->PdbFileName;  */
 
+      if (pdb)
+	*pdb = xstrdup (cvinfo20->PdbFileName);
+
       return cvinfo;
     }
 
@@ -1192,9 +1199,11 @@ _bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length
 }
 
 unsigned int
-_bfd_XXi_write_codeview_record (bfd * abfd, file_ptr where, CODEVIEW_INFO *cvinfo)
+_bfd_XXi_write_codeview_record (bfd * abfd, file_ptr where, CODEVIEW_INFO *cvinfo,
+				const char *pdb)
 {
-  const bfd_size_type size = sizeof (CV_INFO_PDB70) + 1;
+  size_t pdb_len = pdb ? strlen (pdb) : 0;
+  const bfd_size_type size = sizeof (CV_INFO_PDB70) + pdb_len + 1;
   bfd_size_type written;
   CV_INFO_PDB70 *cvinfo70;
   char * buffer;
@@ -1217,7 +1226,11 @@ _bfd_XXi_write_codeview_record (bfd * abfd, file_ptr where, CODEVIEW_INFO *cvinf
   memcpy (&(cvinfo70->Signature[8]), &(cvinfo->Signature[8]), 8);
 
   H_PUT_32 (abfd, cvinfo->Age, cvinfo70->Age);
-  cvinfo70->PdbFileName[0] = '\0';
+
+  if (pdb == NULL)
+    cvinfo70->PdbFileName[0] = '\0';
+  else
+    memcpy (cvinfo70->PdbFileName, pdb, pdb_len + 1);
 
   written = bfd_bwrite (buffer, size, abfd);
 
@@ -2615,22 +2628,25 @@ pe_print_debugdata (bfd * abfd, void * vfile)
 	     We need to use a 32-bit aligned buffer
 	     to safely read in a codeview record.  */
 	  char buffer[256 + 1] ATTRIBUTE_ALIGNED_ALIGNOF (CODEVIEW_INFO);
+	  char *pdb;
 
 	  CODEVIEW_INFO *cvinfo = (CODEVIEW_INFO *) buffer;
 
 	  /* The debug entry doesn't have to have to be in a section,
 	     in which case AddressOfRawData is 0, so always use PointerToRawData.  */
 	  if (!_bfd_XXi_slurp_codeview_record (abfd, (file_ptr) idd.PointerToRawData,
-					       idd.SizeOfData, cvinfo))
+					       idd.SizeOfData, cvinfo, &pdb))
 	    continue;
 
 	  for (j = 0; j < cvinfo->SignatureLength; j++)
 	    sprintf (&signature[j*2], "%02x", cvinfo->Signature[j] & 0xff);
 
 	  /* xgettext:c-format */
-	  fprintf (file, _("(format %c%c%c%c signature %s age %ld)\n"),
+	  fprintf (file, _("(format %c%c%c%c signature %s age %ld pdb %s)\n"),
 		   buffer[0], buffer[1], buffer[2], buffer[3],
-		   signature, cvinfo->Age);
+		   signature, cvinfo->Age, pdb[0] ? pdb : "(none)");
+
+	  free (pdb);
 	}
     }
 
diff --git a/bfd/peicode.h b/bfd/peicode.h
index 02573c84694..326e9f9a8ca 100644
--- a/bfd/peicode.h
+++ b/bfd/peicode.h
@@ -1383,7 +1383,7 @@ pe_bfd_read_buildid (bfd *abfd)
 	  */
 	  if (_bfd_XXi_slurp_codeview_record (abfd,
 					      (file_ptr) idd.PointerToRawData,
-					      idd.SizeOfData, cvinfo))
+					      idd.SizeOfData, cvinfo, NULL))
 	    {
 	      struct bfd_build_id* build_id = bfd_alloc (abfd,
 			 sizeof (struct bfd_build_id) + cvinfo->SignatureLength);
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index 892bf70b7a6..ef15d9ee4ca 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -146,6 +146,8 @@ static lang_assignment_statement_type *image_base_statement = 0;
 static unsigned short pe_dll_characteristics = DEFAULT_DLL_CHARACTERISTICS;
 static bool insert_timestamp = true;
 static const char *emit_build_id;
+static int pdb;
+static char *pdb_name;
 
 #ifdef DLL_SUPPORT
 static int pe_enable_stdcall_fixup = -1; /* 0=disable 1=enable.  */
@@ -284,7 +286,8 @@ fragment <<EOF
 #define OPTION_INSERT_TIMESTAMP		(OPTION_TERMINAL_SERVER_AWARE + 1)
 #define OPTION_NO_INSERT_TIMESTAMP	(OPTION_INSERT_TIMESTAMP + 1)
 #define OPTION_BUILD_ID			(OPTION_NO_INSERT_TIMESTAMP + 1)
-#define OPTION_ENABLE_RELOC_SECTION	(OPTION_BUILD_ID + 1)
+#define OPTION_PDB			(OPTION_BUILD_ID + 1)
+#define OPTION_ENABLE_RELOC_SECTION	(OPTION_PDB + 1)
 #define OPTION_DISABLE_RELOC_SECTION	(OPTION_ENABLE_RELOC_SECTION + 1)
 /* DLL Characteristics flags.  */
 #define OPTION_DISABLE_DYNAMIC_BASE	(OPTION_DISABLE_RELOC_SECTION + 1)
@@ -383,6 +386,7 @@ gld${EMULATION_NAME}_add_options
     {"tsaware", no_argument, NULL, OPTION_TERMINAL_SERVER_AWARE},
     {"disable-tsaware", no_argument, NULL, OPTION_DISABLE_TERMINAL_SERVER_AWARE},
     {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
+    {"pdb", optional_argument, NULL, OPTION_PDB},
     {"enable-reloc-section", no_argument, NULL, OPTION_ENABLE_RELOC_SECTION},
     {"disable-reloc-section", no_argument, NULL, OPTION_DISABLE_RELOC_SECTION},
     {NULL, no_argument, NULL, 0}
@@ -532,6 +536,7 @@ gld${EMULATION_NAME}_list_options (FILE *file)
   fprintf (file, _("  --[disable-]wdmdriver              Driver uses the WDM model\n"));
   fprintf (file, _("  --[disable-]tsaware                Image is Terminal Server aware\n"));
   fprintf (file, _("  --build-id[=STYLE]                 Generate build ID\n"));
+  fprintf (file, _("  --pdb[=FILENAME]                   Generate PDB file\n"));
 }
 
 
@@ -955,6 +960,13 @@ gld${EMULATION_NAME}_handle_option (int optc)
       if (strcmp (optarg, "none"))
 	emit_build_id = xstrdup (optarg);
       break;
+    case OPTION_PDB:
+      if (emit_build_id == NULL)
+	emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
+      pdb = 1;
+      if (optarg)
+	pdb_name = xstrdup (optarg);
+      break;
     }
 
   /*  Set DLLCharacteristics bits  */
@@ -1256,6 +1268,7 @@ write_build_id (bfd *abfd)
   bfd_size_type size;
   bfd_size_type build_id_size;
   unsigned char *build_id;
+  const char *pdb_base_name = NULL;
 
   /* Find the section the .buildid output section has been merged info.  */
   for (asec = abfd->sections; asec != NULL; asec = asec->next)
@@ -1295,6 +1308,9 @@ write_build_id (bfd *abfd)
 
   bfd_vma ib = pe_data (link_info.output_bfd)->pe_opthdr.ImageBase;
 
+  if (pdb_name)
+    pdb_base_name = lbasename (pdb_name);
+
   /* Construct a debug directory entry which points to an immediately following CodeView record.  */
   struct internal_IMAGE_DEBUG_DIRECTORY idd;
   idd.Characteristics = 0;
@@ -1302,7 +1318,7 @@ write_build_id (bfd *abfd)
   idd.MajorVersion = 0;
   idd.MinorVersion = 0;
   idd.Type = PE_IMAGE_DEBUG_TYPE_CODEVIEW;
-  idd.SizeOfData = sizeof (CV_INFO_PDB70) + 1;
+  idd.SizeOfData = sizeof (CV_INFO_PDB70) + (pdb_base_name ? strlen (pdb_base_name) : 0) + 1;
   idd.AddressOfRawData = asec->vma - ib + link_order->offset
     + sizeof (struct external_IMAGE_DEBUG_DIRECTORY);
   idd.PointerToRawData = asec->filepos + link_order->offset
@@ -1331,7 +1347,8 @@ write_build_id (bfd *abfd)
   free (build_id);
 
   /* Write the codeview record.  */
-  if (_bfd_XXi_write_codeview_record (abfd, idd.PointerToRawData, &cvinfo) == 0)
+  if (_bfd_XXi_write_codeview_record (abfd, idd.PointerToRawData, &cvinfo,
+				      pdb_base_name) == 0)
     return 0;
 
   /* Record the location of the debug directory in the data directory.  */
@@ -1368,11 +1385,14 @@ setup_build_id (bfd *ibfd)
 
       /* Section is a fixed size:
 	 One IMAGE_DEBUG_DIRECTORY entry, of type IMAGE_DEBUG_TYPE_CODEVIEW,
-	 pointing at a CV_INFO_PDB70 record containing the build-id, with a
-	 null byte for PdbFileName.  */
+	 pointing at a CV_INFO_PDB70 record containing the build-id, followed by
+	 PdbFileName if relevant.  */
       s->size = sizeof (struct external_IMAGE_DEBUG_DIRECTORY)
 	+ sizeof (CV_INFO_PDB70) + 1;
 
+      if (pdb_name)
+	s->size += strlen (pdb_name);
+
       return true;
     }
 
@@ -1403,6 +1423,25 @@ gld${EMULATION_NAME}_after_open (void)
     }
 #endif
 
+  if (pdb && !pdb_name)
+    {
+      const char *base = lbasename (bfd_get_filename (link_info.output_bfd));
+      size_t len = strlen (base);
+      static const char suffix[] = ".pdb";
+
+      while (len > 0 && base[len] != '.')
+	{
+	  len--;
+	}
+
+      if (len == 0)
+	len = strlen (base);
+
+      pdb_name = xmalloc (len + sizeof (suffix));
+      memcpy (pdb_name, base, len);
+      memcpy (pdb_name + len, suffix, sizeof (suffix));
+    }
+
   if (emit_build_id != NULL)
     {
       bfd *abfd;
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index e68d1e69f17..78b36de49e7 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -157,6 +157,8 @@ static lang_assignment_statement_type *image_base_statement = 0;
 static unsigned short pe_dll_characteristics = DEFAULT_DLL_CHARACTERISTICS;
 static bool insert_timestamp = true;
 static const char *emit_build_id;
+static int pdb;
+static char *pdb_name;
 
 #ifdef DLL_SUPPORT
 static int    pep_enable_stdcall_fixup = 1; /* 0=disable 1=enable (default).  */
@@ -255,6 +257,7 @@ enum options
   OPTION_NO_INSERT_TIMESTAMP,
   OPTION_TERMINAL_SERVER_AWARE,
   OPTION_BUILD_ID,
+  OPTION_PDB,
   OPTION_ENABLE_RELOC_SECTION,
   OPTION_DISABLE_RELOC_SECTION,
   OPTION_DISABLE_HIGH_ENTROPY_VA,
@@ -343,6 +346,7 @@ gld${EMULATION_NAME}_add_options
     {"insert-timestamp", no_argument, NULL, OPTION_INSERT_TIMESTAMP},
     {"no-insert-timestamp", no_argument, NULL, OPTION_NO_INSERT_TIMESTAMP},
     {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
+    {"pdb", optional_argument, NULL, OPTION_PDB},
     {"enable-reloc-section", no_argument, NULL, OPTION_ENABLE_RELOC_SECTION},
     {"disable-reloc-section", no_argument, NULL, OPTION_DISABLE_RELOC_SECTION},
     {"disable-high-entropy-va", no_argument, NULL, OPTION_DISABLE_HIGH_ENTROPY_VA},
@@ -490,6 +494,7 @@ gld${EMULATION_NAME}_list_options (FILE *file)
   fprintf (file, _("  --[disable-]wdmdriver              Driver uses the WDM model\n"));
   fprintf (file, _("  --[disable-]tsaware                Image is Terminal Server aware\n"));
   fprintf (file, _("  --build-id[=STYLE]                 Generate build ID\n"));
+  fprintf (file, _("  --pdb[=FILENAME]                   Generate PDB file\n"));
 #endif
 }
 
@@ -898,6 +903,13 @@ gld${EMULATION_NAME}_handle_option (int optc)
       if (strcmp (optarg, "none"))
 	emit_build_id = xstrdup (optarg);
       break;
+    case OPTION_PDB:
+      if (emit_build_id == NULL)
+	emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
+      pdb = 1;
+      if (optarg)
+	pdb_name = xstrdup (optarg);
+      break;
     }
 
   /*  Set DLLCharacteristics bits  */
@@ -1240,6 +1252,7 @@ write_build_id (bfd *abfd)
   bfd_size_type size;
   bfd_size_type build_id_size;
   unsigned char *build_id;
+  const char *pdb_base_name = NULL;
 
   /* Find the section the .buildid output section has been merged info.  */
   for (asec = abfd->sections; asec != NULL; asec = asec->next)
@@ -1279,6 +1292,9 @@ write_build_id (bfd *abfd)
 
   bfd_vma ib = pe_data (link_info.output_bfd)->pe_opthdr.ImageBase;
 
+  if (pdb_name)
+    pdb_base_name = lbasename (pdb_name);
+
   /* Construct a debug directory entry which points to an immediately following CodeView record.  */
   struct internal_IMAGE_DEBUG_DIRECTORY idd;
   idd.Characteristics = 0;
@@ -1286,7 +1302,7 @@ write_build_id (bfd *abfd)
   idd.MajorVersion = 0;
   idd.MinorVersion = 0;
   idd.Type = PE_IMAGE_DEBUG_TYPE_CODEVIEW;
-  idd.SizeOfData = sizeof (CV_INFO_PDB70) + 1;
+  idd.SizeOfData = sizeof (CV_INFO_PDB70) + (pdb_base_name ? strlen (pdb_base_name) : 0) + 1;
   idd.AddressOfRawData = asec->vma - ib + link_order->offset
     + sizeof (struct external_IMAGE_DEBUG_DIRECTORY);
   idd.PointerToRawData = asec->filepos + link_order->offset
@@ -1315,7 +1331,8 @@ write_build_id (bfd *abfd)
   free (build_id);
 
   /* Write the codeview record.  */
-  if (_bfd_XXi_write_codeview_record (abfd, idd.PointerToRawData, &cvinfo) == 0)
+  if (_bfd_XXi_write_codeview_record (abfd, idd.PointerToRawData, &cvinfo,
+				      pdb_base_name) == 0)
     return 0;
 
   /* Record the location of the debug directory in the data directory.  */
@@ -1352,11 +1369,14 @@ setup_build_id (bfd *ibfd)
 
       /* Section is a fixed size:
 	 One IMAGE_DEBUG_DIRECTORY entry, of type IMAGE_DEBUG_TYPE_CODEVIEW,
-	 pointing at a CV_INFO_PDB70 record containing the build-id, with a
-	 null byte for PdbFileName.  */
+	 pointing at a CV_INFO_PDB70 record containing the build-id, followed by
+	 PdbFileName if relevant.  */
       s->size = sizeof (struct external_IMAGE_DEBUG_DIRECTORY)
 	+ sizeof (CV_INFO_PDB70) + 1;
 
+      if (pdb_name)
+	s->size += strlen (pdb_name);
+
       return true;
     }
 
@@ -1388,6 +1408,25 @@ gld${EMULATION_NAME}_after_open (void)
     }
 #endif
 
+  if (pdb && !pdb_name)
+    {
+      const char *base = lbasename (bfd_get_filename (link_info.output_bfd));
+      size_t len = strlen (base);
+      static const char suffix[] = ".pdb";
+
+      while (len > 0 && base[len] != '.')
+	{
+	  len--;
+	}
+
+      if (len == 0)
+	len = strlen (base);
+
+      pdb_name = xmalloc (len + sizeof (suffix));
+      memcpy (pdb_name, base, len);
+      memcpy (pdb_name + len, suffix, sizeof (suffix));
+    }
+
   if (emit_build_id != NULL)
     {
       bfd *abfd;
diff --git a/ld/testsuite/ld-pe/pdb.exp b/ld/testsuite/ld-pe/pdb.exp
new file mode 100644
index 00000000000..1560241cdb8
--- /dev/null
+++ b/ld/testsuite/ld-pe/pdb.exp
@@ -0,0 +1,53 @@
+# Expect script for creating PDB files when linking.
+#   Copyright (C) 2022 Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+if {![istarget i*86-*-mingw*]
+  && ![istarget x86_64-*-mingw*]} {
+    return
+}
+
+proc get_pdb_name { pe } {
+    global OBJDUMP
+
+    set exec_output [run_host_cmd "$OBJDUMP" "-p $pe"]
+
+    if ![regexp -line "^\\(format RSDS signature (\[0-9a-fA-F\]{32}) age 1 pdb (.*)\\)$" $exec_output full sig pdb] {
+	return ""
+    }
+
+    return $pdb
+}
+
+if ![ld_assemble $as $srcdir/$subdir/pdb1.s tmpdir/pdb1.o] {
+    unsupported "Build pdb1.o"
+    return
+}
+
+if ![ld_link $ld "tmpdir/pdb1.exe" "--pdb=tmpdir/pdb1.pdb tmpdir/pdb1.o"] {
+    fail "Could not create a PE image with a PDB file"
+    return
+}
+
+if ![string equal [get_pdb_name "tmpdir/pdb1.exe"] "pdb1.pdb"] {
+    fail "PDB filename not found in CodeView debug info"
+    return
+}
+
+pass "PDB filename present in CodeView debug info"
diff --git a/ld/testsuite/ld-pe/pdb1.s b/ld/testsuite/ld-pe/pdb1.s
new file mode 100644
index 00000000000..30a8cfcca2c
--- /dev/null
+++ b/ld/testsuite/ld-pe/pdb1.s
@@ -0,0 +1,5 @@
+.text
+
+.global foo
+foo:
+	.long 0x12345678
-- 
2.35.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ld: Add minimal pdb generation
  2022-10-03  1:43 [PATCH 1/2] ld: Add --pdb option Mark Harmstone
@ 2022-10-03  1:43 ` Mark Harmstone
  2022-10-03  5:12 ` [PATCH 1/2] ld: Add --pdb option Martin Storsjö
  2022-10-05  4:20 ` Alan Modra
  2 siblings, 0 replies; 11+ messages in thread
From: Mark Harmstone @ 2022-10-03  1:43 UTC (permalink / raw)
  To: binutils; +Cc: Mark Harmstone

This changes ld's --pdb option so that it also a minimal PDB file, i.e.
one that windbg or Visual Studio will load but which doesn't yet contain
anything useful.

See also https://github.com/microsoft/microsoft-pdb/raw/master/cvdump/cvdump.exe,
which can be fed the produced file.

---
 ld/Makefile.am             |  10 +-
 ld/Makefile.in             |  13 +-
 ld/emultempl/pe.em         |   7 +
 ld/emultempl/pep.em        |   7 +
 ld/pdb.c                   | 516 +++++++++++++++++++++++++++++++++++++
 ld/pdb.h                   | 111 ++++++++
 ld/testsuite/ld-pe/pdb.exp | 267 +++++++++++++++++++
 7 files changed, 922 insertions(+), 9 deletions(-)
 create mode 100644 ld/pdb.c
 create mode 100644 ld/pdb.h

diff --git a/ld/Makefile.am b/ld/Makefile.am
index d31021c13e2..43114372989 100644
--- a/ld/Makefile.am
+++ b/ld/Makefile.am
@@ -475,12 +475,14 @@ ALL_64_EMUL_EXTRA_OFILES = \
 CFILES = ldctor.c ldemul.c ldexp.c ldfile.c ldlang.c \
 	ldmain.c ldmisc.c ldver.c ldwrite.c lexsup.c \
 	mri.c ldcref.c pe-dll.c pep-dll.c ldlex-wrapper.c \
-	plugin.c ldbuildid.c ldelf.c ldelfgen.c
+	plugin.c ldbuildid.c ldelf.c ldelfgen.c \
+	pdb.c
 
 HFILES = ld.h ldctor.h ldemul.h ldexp.h ldfile.h \
 	ldlang.h ldlex.h ldmain.h ldmisc.h ldver.h \
 	ldwrite.h mri.h deffile.h pe-dll.h pep-dll.h \
-	elf-hints-local.h plugin.h ldbuildid.h ldelf.h ldelfgen.h
+	elf-hints-local.h plugin.h ldbuildid.h ldelf.h ldelfgen.h \
+	pdb.h
 
 GENERATED_CFILES = ldgram.c ldlex.c deffilep.c
 GENERATED_HFILES = ldgram.h ldemul-list.h deffilep.h
@@ -493,7 +495,7 @@ OFILES = ldgram.@OBJEXT@ ldlex-wrapper.@OBJEXT@ lexsup.@OBJEXT@ ldlang.@OBJEXT@
 	mri.@OBJEXT@ ldctor.@OBJEXT@ ldmain.@OBJEXT@ plugin.@OBJEXT@ \
 	ldwrite.@OBJEXT@ ldexp.@OBJEXT@  ldemul.@OBJEXT@ ldver.@OBJEXT@ ldmisc.@OBJEXT@ \
 	ldfile.@OBJEXT@ ldcref.@OBJEXT@ ${EMULATION_OFILES} ${EMUL_EXTRA_OFILES} \
-	ldbuildid.@OBJEXT@
+	ldbuildid.@OBJEXT@ pdb.@OBJEXT@
 
 STAGESTUFF = *.@OBJEXT@ ldscripts/* e*.c
 
@@ -956,7 +958,7 @@ EXTRA_ld_new_SOURCES += pep-dll.c pe-dll.c ldelf.c ldelfgen.c
 
 ld_new_SOURCES = ldgram.y ldlex-wrapper.c lexsup.c ldlang.c mri.c ldctor.c ldmain.c \
 	ldwrite.c ldexp.c ldemul.c ldver.c ldmisc.c ldfile.c ldcref.c plugin.c \
-	ldbuildid.c
+	ldbuildid.c pdb.c
 ld_new_DEPENDENCIES = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) \
 		      $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL_DEP) $(JANSSON_LIBS)
 ld_new_LDADD = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL) $(ZLIB) $(JANSSON_LIBS)
diff --git a/ld/Makefile.in b/ld/Makefile.in
index ee0c98f65b0..7ce4704c66f 100644
--- a/ld/Makefile.in
+++ b/ld/Makefile.in
@@ -211,7 +211,7 @@ am_ld_new_OBJECTS = ldgram.$(OBJEXT) ldlex-wrapper.$(OBJEXT) \
 	ldctor.$(OBJEXT) ldmain.$(OBJEXT) ldwrite.$(OBJEXT) \
 	ldexp.$(OBJEXT) ldemul.$(OBJEXT) ldver.$(OBJEXT) \
 	ldmisc.$(OBJEXT) ldfile.$(OBJEXT) ldcref.$(OBJEXT) \
-	plugin.$(OBJEXT) ldbuildid.$(OBJEXT)
+	plugin.$(OBJEXT) ldbuildid.$(OBJEXT) pdb.$(OBJEXT)
 ld_new_OBJECTS = $(am_ld_new_OBJECTS)
 am__DEPENDENCIES_1 =
 @ENABLE_LIBCTF_TRUE@am__DEPENDENCIES_2 = ../libctf/libctf.la
@@ -970,12 +970,14 @@ ALL_64_EMUL_EXTRA_OFILES = \
 CFILES = ldctor.c ldemul.c ldexp.c ldfile.c ldlang.c \
 	ldmain.c ldmisc.c ldver.c ldwrite.c lexsup.c \
 	mri.c ldcref.c pe-dll.c pep-dll.c ldlex-wrapper.c \
-	plugin.c ldbuildid.c ldelf.c ldelfgen.c
+	plugin.c ldbuildid.c ldelf.c ldelfgen.c \
+	pdb.c
 
 HFILES = ld.h ldctor.h ldemul.h ldexp.h ldfile.h \
 	ldlang.h ldlex.h ldmain.h ldmisc.h ldver.h \
 	ldwrite.h mri.h deffile.h pe-dll.h pep-dll.h \
-	elf-hints-local.h plugin.h ldbuildid.h ldelf.h ldelfgen.h
+	elf-hints-local.h plugin.h ldbuildid.h ldelf.h ldelfgen.h \
+	pdb.h
 
 GENERATED_CFILES = ldgram.c ldlex.c deffilep.c
 GENERATED_HFILES = ldgram.h ldemul-list.h deffilep.h
@@ -987,7 +989,7 @@ OFILES = ldgram.@OBJEXT@ ldlex-wrapper.@OBJEXT@ lexsup.@OBJEXT@ ldlang.@OBJEXT@
 	mri.@OBJEXT@ ldctor.@OBJEXT@ ldmain.@OBJEXT@ plugin.@OBJEXT@ \
 	ldwrite.@OBJEXT@ ldexp.@OBJEXT@  ldemul.@OBJEXT@ ldver.@OBJEXT@ ldmisc.@OBJEXT@ \
 	ldfile.@OBJEXT@ ldcref.@OBJEXT@ ${EMULATION_OFILES} ${EMUL_EXTRA_OFILES} \
-	ldbuildid.@OBJEXT@
+	ldbuildid.@OBJEXT@ pdb.@OBJEXT@
 
 STAGESTUFF = *.@OBJEXT@ ldscripts/* e*.c
 SRC_POTFILES = $(CFILES) $(HFILES)
@@ -1006,7 +1008,7 @@ EXTRA_ld_new_SOURCES = deffilep.y ldlex.l pep-dll.c pe-dll.c ldelf.c \
 	$(ALL_64_EMULATION_SOURCES)
 ld_new_SOURCES = ldgram.y ldlex-wrapper.c lexsup.c ldlang.c mri.c ldctor.c ldmain.c \
 	ldwrite.c ldexp.c ldemul.c ldver.c ldmisc.c ldfile.c ldcref.c plugin.c \
-	ldbuildid.c
+	ldbuildid.c pdb.c
 
 ld_new_DEPENDENCIES = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) \
 		      $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL_DEP) $(JANSSON_LIBS)
@@ -1569,6 +1571,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/libldtestplug4_la-testplug4.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/libldtestplug_la-testplug.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/mri.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pdb.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pe-dll.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pep-dll.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin.Po@am__quote@
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index ef15d9ee4ca..5f0ac13d455 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -66,6 +66,7 @@ fragment <<EOF
 #include "ldctor.h"
 #include "ldbuildid.h"
 #include "coff/internal.h"
+#include "pdb.h"
 
 /* FIXME: See bfd/peXXigen.c for why we include an architecture specific
    header in generic PE code.  */
@@ -1334,6 +1335,12 @@ write_build_id (bfd *abfd)
   if (bfd_bwrite (contents, size, abfd) != size)
     return 0;
 
+  if (pdb)
+    {
+      if (!create_pdb_file (abfd, pdb_name, build_id))
+	return 0;
+    }
+
   /* Construct the CodeView record.  */
   CODEVIEW_INFO cvinfo;
   cvinfo.CVSignature = CVINFO_PDB70_CVSIGNATURE;
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 78b36de49e7..b2241afb46f 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -69,6 +69,7 @@ fragment <<EOF
 #include "ldctor.h"
 #include "ldbuildid.h"
 #include "coff/internal.h"
+#include "pdb.h"
 
 /* FIXME: See bfd/peXXigen.c for why we include an architecture specific
    header in generic PE code.  */
@@ -1318,6 +1319,12 @@ write_build_id (bfd *abfd)
   if (bfd_bwrite (contents, size, abfd) != size)
     return 0;
 
+  if (pdb)
+    {
+      if (!create_pdb_file (abfd, pdb_name, build_id))
+	return 0;
+    }
+
   /* Construct the CodeView record.  */
   CODEVIEW_INFO cvinfo;
   cvinfo.CVSignature = CVINFO_PDB70_CVSIGNATURE;
diff --git a/ld/pdb.c b/ld/pdb.c
new file mode 100644
index 00000000000..a425408c0b3
--- /dev/null
+++ b/ld/pdb.c
@@ -0,0 +1,516 @@
+/* Support for generating PDB CodeView debugging files.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of the GNU Binutils.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+#include "pdb.h"
+#include "bfdlink.h"
+#include "ld.h"
+#include "ldmisc.h"
+#include "libbfd.h"
+#include "libiberty.h"
+#include "coff/i386.h"
+#include "coff/external.h"
+#include "coff/internal.h"
+#include "coff/pe.h"
+#include "libcoff.h"
+#include <time.h>
+
+struct public
+{
+  struct public *next;
+  uint32_t offset;
+  uint32_t hash;
+  unsigned int index;
+  uint16_t section;
+  uint32_t address;
+};
+
+/* Add a new stream to the PDB archive, and return its BFD.  */
+static bfd *
+add_stream (bfd *pdb, const char *name, uint16_t *stream_num)
+{
+  bfd *stream;
+  uint16_t num;
+
+  stream = bfd_create (name ? name : "", pdb);
+  if (!stream)
+    return NULL;
+
+  if (!bfd_make_writable (stream))
+    {
+      bfd_close (stream);
+      return false;
+    }
+
+  if (!pdb->archive_head)
+    {
+      bfd_set_archive_head (pdb, stream);
+      num = 0;
+    }
+  else
+    {
+      bfd *b = pdb->archive_head;
+
+      num = 1;
+
+      while (b->archive_next)
+	{
+	  num++;
+	  b = b->archive_next;
+	}
+
+      b->archive_next = stream;
+    }
+
+  if (stream_num)
+    *stream_num = num;
+
+  return stream;
+}
+
+/* Stream 0 ought to be a copy of the MSF directory from the last
+   time the PDB file was written.  Because we don't do incremental
+   writes this isn't applicable to us, but we fill it with a dummy
+   value so as not to confuse radare.  */
+static bool
+create_old_directory_stream (bfd *pdb)
+{
+  bfd *stream;
+  char buf[sizeof (uint32_t)];
+
+  stream = add_stream (pdb, NULL, NULL);
+  if (!stream)
+    return false;
+
+  bfd_putl32 (0, buf);
+
+  return bfd_bwrite (buf, sizeof (uint32_t), stream) == sizeof (uint32_t);
+}
+
+/* Calculate the hash of a given string.  */
+static uint32_t
+calc_hash (const char *data, size_t len)
+{
+  uint32_t hash = 0;
+
+  while (len >= 4)
+    {
+      hash ^= *(uint32_t *) data;
+      data += 4;
+      len -= 4;
+    }
+
+  if (len >= 2)
+    {
+      hash ^= *(uint16_t *) data;
+      data += 2;
+      len -= 2;
+    }
+
+  if (len != 0)
+    hash ^= *data;
+
+  hash |= 0x20202020;
+  hash ^= (hash >> 11);
+
+  return hash ^ (hash >> 16);
+}
+
+/* Stream 1 is the PDB info stream - see
+   https://llvm.org/docs/PDB/PdbStream.html.  */
+static bool
+populate_info_stream (bfd *pdb, bfd *info_stream, const unsigned char *guid)
+{
+  bool ret = false;
+  struct pdb_stream_70 h;
+  uint32_t num_entries, num_buckets;
+  uint32_t names_length, stream_num;
+  char int_buf[sizeof (uint32_t)];
+
+  struct hash_entry
+  {
+    uint32_t offset;
+    uint32_t value;
+  };
+
+  struct hash_entry **buckets = NULL;
+
+  /* Write header.  */
+
+  bfd_putl32 (PDB_STREAM_VERSION_VC70, &h.version);
+  bfd_putl32 (time (NULL), &h.signature);
+  bfd_putl32 (1, &h.age);
+
+  bfd_putl32 (bfd_getb32 (guid), h.guid);
+  bfd_putl16 (bfd_getb16 (&guid[4]), &h.guid[4]);
+  bfd_putl16 (bfd_getb16 (&guid[6]), &h.guid[6]);
+  memcpy (&h.guid[8], &guid[8], 8);
+
+  if (bfd_bwrite (&h, sizeof (h), info_stream) != sizeof (h))
+    return false;
+
+  /* Write hash list of named streams.  This is a "rollover" hash, i.e.
+     if a bucket is filled an entry gets placed in the next free
+     slot.  */
+
+  num_entries = 0;
+  for (bfd *b = pdb->archive_head; b; b = b->archive_next)
+    {
+      if (strcmp (b->filename, ""))
+	num_entries++;
+    }
+
+  num_buckets = num_entries * 2;
+
+  names_length = 0;
+  stream_num = 0;
+
+  if (num_buckets > 0)
+    {
+      buckets = xmalloc (sizeof (struct hash_entry *) * num_buckets);
+      memset (buckets, 0, sizeof (struct hash_entry *) * num_buckets);
+
+      for (bfd *b = pdb->archive_head; b; b = b->archive_next)
+	{
+	  if (strcmp (b->filename, ""))
+	    {
+	      size_t len = strlen (b->filename);
+	      uint32_t hash = (uint16_t) calc_hash (b->filename, len);
+	      uint32_t bucket_num = hash % num_buckets;
+
+	      while (buckets[bucket_num])
+		{
+		  bucket_num++;
+
+		  if (bucket_num == num_buckets)
+		    bucket_num = 0;
+		}
+
+	      buckets[bucket_num] = xmalloc (sizeof (struct hash_entry));
+
+	      buckets[bucket_num]->offset = names_length;
+	      buckets[bucket_num]->value = stream_num;
+
+	      names_length += len + 1;
+	    }
+
+	  stream_num++;
+	}
+    }
+
+  /* Write the strings list - the hash keys are indexes into this.  */
+
+  bfd_putl32 (names_length, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  for (bfd *b = pdb->archive_head; b; b = b->archive_next)
+    {
+      if (!strcmp (b->filename, ""))
+	continue;
+
+      size_t len = strlen (b->filename) + 1;
+
+      if (bfd_bwrite (b->filename, len, info_stream) != len)
+	goto end;
+    }
+
+  /* Write the number of entries and buckets.  */
+
+  bfd_putl32 (num_entries, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  bfd_putl32 (num_buckets, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  /* Write the present bitmap.  */
+
+  bfd_putl32 ((num_buckets + 31) / 32, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  for (unsigned int i = 0; i < num_buckets; i += 32)
+    {
+      uint32_t v = 0;
+
+      for (unsigned int j = 0; j < 32; j++)
+	{
+	  if (i + j >= num_buckets)
+	    break;
+
+	  if (buckets[i + j])
+	    v |= 1 << j;
+	}
+
+      bfd_putl32 (v, int_buf);
+
+      if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+	  sizeof (uint32_t))
+	goto end;
+    }
+
+  /* Write the (empty) deleted bitmap.  */
+
+  bfd_putl32 (0, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  /* Write the buckets.  */
+
+  for (unsigned int i = 0; i < num_buckets; i++)
+    {
+      if (buckets[i])
+	{
+	  bfd_putl32 (buckets[i]->offset, int_buf);
+
+	  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+	      sizeof (uint32_t))
+	    goto end;
+
+	  bfd_putl32 (buckets[i]->value, int_buf);
+
+	  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+	      sizeof (uint32_t))
+	    goto end;
+	}
+    }
+
+  bfd_putl32 (0, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  bfd_putl32 (PDB_STREAM_VERSION_VC140, int_buf);
+
+  if (bfd_bwrite (int_buf, sizeof (uint32_t), info_stream) !=
+      sizeof (uint32_t))
+    goto end;
+
+  ret = true;
+
+end:
+  for (unsigned int i = 0; i < num_buckets; i++)
+    {
+      if (buckets[i])
+	free (buckets[i]);
+    }
+
+  free (buckets);
+
+  return ret;
+}
+
+/* Stream 2 is the type information (TPI) stream, and stream 4 is
+   the ID information (IPI) stream.  They differ only in which records
+   go in which stream. */
+static bool
+create_type_stream (bfd *pdb)
+{
+  bfd *stream;
+  struct pdb_tpi_stream_header h;
+
+  stream = add_stream (pdb, NULL, NULL);
+  if (!stream)
+    return false;
+
+  bfd_putl32 (TPI_STREAM_VERSION_80, &h.version);
+  bfd_putl32 (sizeof (h), &h.header_size);
+  bfd_putl32 (TPI_FIRST_INDEX, &h.type_index_begin);
+  bfd_putl32 (TPI_FIRST_INDEX, &h.type_index_end);
+  bfd_putl32 (0, &h.type_record_bytes);
+  bfd_putl16 (0xffff, &h.hash_stream_index);
+  bfd_putl16 (0xffff, &h.hash_aux_stream_index);
+  bfd_putl32 (4, &h.hash_key_size);
+  bfd_putl32 (0x3ffff, &h.num_hash_buckets);
+  bfd_putl32 (0, &h.hash_value_buffer_offset);
+  bfd_putl32 (0, &h.hash_value_buffer_length);
+  bfd_putl32 (0, &h.index_offset_buffer_offset);
+  bfd_putl32 (0, &h.index_offset_buffer_length);
+  bfd_putl32 (0, &h.hash_adj_buffer_offset);
+  bfd_putl32 (0, &h.hash_adj_buffer_length);
+
+  if (bfd_bwrite (&h, sizeof (h), stream) != sizeof (h))
+    return false;
+
+  return true;
+}
+
+/* Return the PE architecture number for the image.  */
+static uint16_t
+get_arch_number (bfd *abfd)
+{
+  if (abfd->arch_info->arch != bfd_arch_i386)
+    return 0;
+
+  if (abfd->arch_info->mach & bfd_mach_x86_64)
+    return IMAGE_FILE_MACHINE_AMD64;
+
+  return IMAGE_FILE_MACHINE_I386;
+}
+
+/* Stream 4 is the debug information (DBI) stream.  */
+static bool
+populate_dbi_stream (bfd *stream, bfd *abfd)
+{
+  struct pdb_dbi_stream_header h;
+  struct optional_dbg_header opt;
+
+  bfd_putl32 (0xffffffff, &h.version_signature);
+  bfd_putl32 (DBI_STREAM_VERSION_70, &h.version_header);
+  bfd_putl32 (1, &h.age);
+  bfd_putl16 (0xffff, &h.global_stream_index);
+  bfd_putl16 (0x8e1d, &h.build_number); // MSVC 14.29
+  bfd_putl16 (0xffff, &h.public_stream_index);
+  bfd_putl16 (0, &h.pdb_dll_version);
+  bfd_putl16 (0xffff, &h.sym_record_stream);
+  bfd_putl16 (0, &h.pdb_dll_rbld);
+  bfd_putl32 (0, &h.mod_info_size);
+  bfd_putl32 (0, &h.section_contribution_size);
+  bfd_putl32 (0, &h.section_map_size);
+  bfd_putl32 (0, &h.source_info_size);
+  bfd_putl32 (0, &h.type_server_map_size);
+  bfd_putl32 (0, &h.mfc_type_server_index);
+  bfd_putl32 (sizeof (opt), &h.optional_dbg_header_size);
+  bfd_putl32 (0, &h.ec_substream_size);
+  bfd_putl16 (0, &h.flags);
+  bfd_putl16 (get_arch_number (abfd), &h.machine);
+  bfd_putl32 (0, &h.padding);
+
+  if (bfd_bwrite (&h, sizeof (h), stream) != sizeof (h))
+    return false;
+
+  bfd_putl16 (0xffff, &opt.fpo_stream);
+  bfd_putl16 (0xffff, &opt.exception_stream);
+  bfd_putl16 (0xffff, &opt.fixup_stream);
+  bfd_putl16 (0xffff, &opt.omap_to_src_stream);
+  bfd_putl16 (0xffff, &opt.omap_from_src_stream);
+  bfd_putl16 (0xffff, &opt.section_header_stream);
+  bfd_putl16 (0xffff, &opt.token_map_stream);
+  bfd_putl16 (0xffff, &opt.xdata_stream);
+  bfd_putl16 (0xffff, &opt.pdata_stream);
+  bfd_putl16 (0xffff, &opt.new_fpo_stream);
+  bfd_putl16 (0xffff, &opt.orig_section_header_stream);
+
+  if (bfd_bwrite (&opt, sizeof (opt), stream) != sizeof (opt))
+    return false;
+
+  return true;
+}
+
+/* Create a PDB debugging file for the PE image file abfd with the build ID
+   guid, stored at pdb_name.  */
+bool
+create_pdb_file (bfd *abfd, const char *pdb_name, const unsigned char *guid)
+{
+  bfd *pdb;
+  bool ret = false;
+  bfd *info_stream, *dbi_stream, *names_stream;
+
+  pdb = bfd_openw (pdb_name, "pdb");
+  if (!pdb)
+    {
+      einfo (_("%P: warning: cannot create PDB file: %s\n"),
+	     bfd_errmsg (bfd_get_error ()));
+      return false;
+    }
+
+  bfd_set_format (pdb, bfd_archive);
+
+  if (!create_old_directory_stream (pdb))
+    {
+      einfo (_("%P: warning: cannot create old directory stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  info_stream = add_stream (pdb, NULL, NULL);
+
+  if (!info_stream)
+    {
+      einfo (_("%P: warning: cannot create info stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  if (!create_type_stream (pdb))
+    {
+      einfo (_("%P: warning: cannot create TPI stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  dbi_stream = add_stream (pdb, NULL, NULL);
+
+  if (!dbi_stream)
+    {
+      einfo (_("%P: warning: cannot create DBI stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  if (!create_type_stream (pdb))
+    {
+      einfo (_("%P: warning: cannot create IPI stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  names_stream = add_stream (pdb, "/names", NULL);
+
+  if (!names_stream)
+    {
+      einfo (_("%P: warning: cannot create /names stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  if (!populate_dbi_stream (dbi_stream, abfd))
+    {
+      einfo (_("%P: warning: cannot populate DBI stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  if (!populate_info_stream (pdb, info_stream, guid))
+    {
+      einfo (_("%P: warning: cannot populate info stream "
+	       "in PDB file: %s\n"), bfd_errmsg (bfd_get_error ()));
+      goto end;
+    }
+
+  ret = true;
+
+end:
+  bfd_close (pdb);
+
+  return ret;
+}
diff --git a/ld/pdb.h b/ld/pdb.h
new file mode 100644
index 00000000000..e5f53b44f39
--- /dev/null
+++ b/ld/pdb.h
@@ -0,0 +1,111 @@
+/* pdb.h - header file for generating PDB CodeView debugging files.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of the GNU Binutils.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+/* Header files referred to below can be found in Microsoft's PDB
+   repository: https://github.com/microsoft/microsoft-pdb.  */
+
+#ifndef PDB_H
+#define PDB_H
+
+#include "sysdep.h"
+#include "bfd.h"
+#include <stdbool.h>
+
+/* PDBStream70 in pdb1.h */
+struct pdb_stream_70
+{
+  uint32_t version;
+  uint32_t signature;
+  uint32_t age;
+  uint8_t guid[16];
+};
+
+#define PDB_STREAM_VERSION_VC70		20000404
+#define PDB_STREAM_VERSION_VC140	20140508
+
+/* HDR in tpi.h */
+struct pdb_tpi_stream_header
+{
+  uint32_t version;
+  uint32_t header_size;
+  uint32_t type_index_begin;
+  uint32_t type_index_end;
+  uint32_t type_record_bytes;
+  uint16_t hash_stream_index;
+  uint16_t hash_aux_stream_index;
+  uint32_t hash_key_size;
+  uint32_t num_hash_buckets;
+  uint32_t hash_value_buffer_offset;
+  uint32_t hash_value_buffer_length;
+  uint32_t index_offset_buffer_offset;
+  uint32_t index_offset_buffer_length;
+  uint32_t hash_adj_buffer_offset;
+  uint32_t hash_adj_buffer_length;
+};
+
+#define TPI_STREAM_VERSION_80		20040203
+
+#define TPI_FIRST_INDEX			0x1000
+
+/* NewDBIHdr in dbi.h */
+struct pdb_dbi_stream_header
+{
+  uint32_t version_signature;
+  uint32_t version_header;
+  uint32_t age;
+  uint16_t global_stream_index;
+  uint16_t build_number;
+  uint16_t public_stream_index;
+  uint16_t pdb_dll_version;
+  uint16_t sym_record_stream;
+  uint16_t pdb_dll_rbld;
+  uint32_t mod_info_size;
+  uint32_t section_contribution_size;
+  uint32_t section_map_size;
+  uint32_t source_info_size;
+  uint32_t type_server_map_size;
+  uint32_t mfc_type_server_index;
+  uint32_t optional_dbg_header_size;
+  uint32_t ec_substream_size;
+  uint16_t flags;
+  uint16_t machine;
+  uint32_t padding;
+};
+
+#define DBI_STREAM_VERSION_70		19990903
+
+struct optional_dbg_header
+{
+  uint16_t fpo_stream;
+  uint16_t exception_stream;
+  uint16_t fixup_stream;
+  uint16_t omap_to_src_stream;
+  uint16_t omap_from_src_stream;
+  uint16_t section_header_stream;
+  uint16_t token_map_stream;
+  uint16_t xdata_stream;
+  uint16_t pdata_stream;
+  uint16_t new_fpo_stream;
+  uint16_t orig_section_header_stream;
+};
+
+extern bool create_pdb_file (bfd *, const char *, const unsigned char *);
+
+#endif
diff --git a/ld/testsuite/ld-pe/pdb.exp b/ld/testsuite/ld-pe/pdb.exp
index 1560241cdb8..b62ce6da6f8 100644
--- a/ld/testsuite/ld-pe/pdb.exp
+++ b/ld/testsuite/ld-pe/pdb.exp
@@ -35,6 +35,249 @@ proc get_pdb_name { pe } {
     return $pdb
 }
 
+proc get_pdb_guid { pe } {
+    global OBJDUMP
+
+    set exec_output [run_host_cmd "$OBJDUMP" "-p $pe"]
+
+    if ![regexp -line "^\\(format RSDS signature (\[0-9a-fA-F\]{32}) age 1 pdb (.*)\\)$" $exec_output full sig pdb] {
+	return ""
+    }
+
+    return $sig
+}
+
+proc check_pdb_info_stream { pdb guid } {
+    global ar
+
+    set exec_output [run_host_cmd "$ar" "x --output tmpdir $pdb 0001"]
+
+    if ![string match "" $exec_output] {
+	return 0
+    }
+
+    set fi [open tmpdir/0001]
+    fconfigure $fi -translation binary
+
+    # check version
+
+    set data [read $fi 4]
+    binary scan $data i version
+
+    if { $version != 20000404 } {
+	close $fi
+	return 0
+    }
+
+    # skip signature (timestamp)
+    read $fi 4
+
+    # check age
+
+    set data [read $fi 4]
+    binary scan $data i age
+
+    if { $age != 1 } {
+	close $fi
+	return 0
+    }
+
+    # check GUID
+
+    set data [read $fi 16]
+    binary scan $data H2H2H2H2H2H2H2H2H* guid1 guid2 guid3 guid4 guid5 guid6 guid7 guid8 guid9
+
+    set data "$guid4$guid3$guid2$guid1$guid6$guid5$guid8$guid7$guid9"
+
+    if { $data ne $guid } {
+	close $fi
+	return 0
+    }
+
+    # skip names string
+
+    set data [read $fi 4]
+    binary scan $data i names_length
+    read $fi $names_length
+
+    # read number of names entries
+
+    set data [read $fi 4]
+    binary scan $data i num_entries
+
+    # skip number of buckets
+    read $fi 4
+
+    # skip present bitmap
+
+    set data [read $fi 4]
+    binary scan $data i bitmap_length
+    read $fi [expr $bitmap_length * 4]
+
+    # skip deleted bitmap
+
+    set data [read $fi 4]
+    binary scan $data i bitmap_length
+    read $fi [expr $bitmap_length * 4]
+
+    # skip names entries
+    read $fi [expr $num_entries * 8]
+
+    # skip uint32_t
+    read $fi 4
+
+    # read second version
+
+    set data [read $fi 4]
+    binary scan $data i version2
+
+    if { $version2 != 20140508 } {
+	close $fi
+	return 0
+    }
+
+    close $fi
+
+    return 1
+}
+
+proc check_type_stream { pdb stream } {
+    global ar
+
+    set exec_output [run_host_cmd "$ar" "x --output tmpdir $pdb $stream"]
+
+    if ![string match "" $exec_output] {
+	return 0
+    }
+
+    set fi [open tmpdir/$stream]
+    fconfigure $fi -translation binary
+
+    # check version
+
+    set data [read $fi 4]
+    binary scan $data i version
+
+    if { $version != 20040203 } {
+	close $fi
+	return 0
+    }
+
+    # check header size
+
+    set data [read $fi 4]
+    binary scan $data i header_size
+
+    if { $header_size != 0x38 } {
+	close $fi
+	return 0
+    }
+
+    # skip type_index_begin and type_index_end
+    read $fi 8
+
+    # read type_record_bytes
+
+    set data [read $fi 4]
+    binary scan $data i type_record_bytes
+
+    close $fi
+
+    # check stream length
+
+    set stream_length [file size tmpdir/$stream]
+
+    if { $stream_length != [ expr $header_size + $type_record_bytes ] } {
+	return 0
+    }
+
+    return 1
+}
+
+proc check_dbi_stream { pdb } {
+    global ar
+
+    set exec_output [run_host_cmd "$ar" "x --output tmpdir $pdb 0003"]
+
+    if ![string match "" $exec_output] {
+	return 0
+    }
+
+    set fi [open tmpdir/0003]
+    fconfigure $fi -translation binary
+
+    # check signature
+
+    set data [read $fi 4]
+    binary scan $data i signature
+
+    if { $signature != -1 } {
+	close $fi
+	return 0
+    }
+
+    # check version
+
+    set data [read $fi 4]
+    binary scan $data i version
+
+    if { $version != 19990903 } {
+	close $fi
+	return 0
+    }
+
+    # check age
+
+    set data [read $fi 4]
+    binary scan $data i age
+
+    if { $age != 1 } {
+	close $fi
+	return 0
+    }
+
+    # skip fields
+    read $fi 12
+
+    # read substream sizes
+
+    set data [read $fi 4]
+    binary scan $data i mod_info_size
+
+    set data [read $fi 4]
+    binary scan $data i section_contribution_size
+
+    set data [read $fi 4]
+    binary scan $data i section_map_size
+
+    set data [read $fi 4]
+    binary scan $data i source_info_size
+
+    set data [read $fi 4]
+    binary scan $data i type_server_map_size
+
+    set data [read $fi 4]
+    binary scan $data i mfc_type_server_index
+
+    set data [read $fi 4]
+    binary scan $data i optional_dbg_header_size
+
+    set data [read $fi 4]
+    binary scan $data i ec_substream_size
+
+    close $fi
+
+    # check stream length
+
+    set stream_length [file size tmpdir/0003]
+
+    if { $stream_length != [expr 0x40 + $mod_info_size + $section_contribution_size + $section_map_size + $source_info_size + $type_server_map_size + $mfc_type_server_index + $optional_dbg_header_size + $ec_substream_size] } {
+	return 0
+    }
+
+    return 1
+}
+
 if ![ld_assemble $as $srcdir/$subdir/pdb1.s tmpdir/pdb1.o] {
     unsupported "Build pdb1.o"
     return
@@ -51,3 +294,27 @@ if ![string equal [get_pdb_name "tmpdir/pdb1.exe"] "pdb1.pdb"] {
 }
 
 pass "PDB filename present in CodeView debug info"
+
+if [check_pdb_info_stream tmpdir/pdb1.pdb [get_pdb_guid "tmpdir/pdb1.exe"]] {
+    pass "Valid PDB info stream"
+} else {
+    fail "Invalid PDB info stream"
+}
+
+if [check_type_stream tmpdir/pdb1.pdb "0002"] {
+    pass "Valid TPI stream"
+} else {
+    fail "Invalid TPI stream"
+}
+
+if [check_type_stream tmpdir/pdb1.pdb "0004"] {
+    pass "Valid IPI stream"
+} else {
+    fail "Invalid IPI stream"
+}
+
+if [check_dbi_stream tmpdir/pdb1.pdb] {
+    pass "Valid DBI stream"
+} else {
+    fail "Invalid DBI stream"
+}
-- 
2.35.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-03  1:43 [PATCH 1/2] ld: Add --pdb option Mark Harmstone
  2022-10-03  1:43 ` [PATCH 2/2] ld: Add minimal pdb generation Mark Harmstone
@ 2022-10-03  5:12 ` Martin Storsjö
  2022-10-03 16:57   ` Mark Harmstone
  2022-10-05  4:20 ` Alan Modra
  2 siblings, 1 reply; 11+ messages in thread
From: Martin Storsjö @ 2022-10-03  5:12 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

Hi Mark,

On Mon, 3 Oct 2022, Mark Harmstone wrote:

> This patch adds the --pdb option to ld when linking PE files, which
> augments the existing CodeView record used for build IDs by adding a PDB
> filename. If no filename is provided, this defaults to the image name
> with the extension replaced by "pdb".

As I assume you're aware, lld's mingw port also supports PDB generation - 
and the description of this option also sounds like it's chosen to match 
lld's option for outputting PDB files - that's good!

The testcase only seemed to exercise the form --pdb=<explicitname>, so I 
thought I'd ask just for clarity: I guess the other forms of specifying 
the option, e.g. "--pdb <explicitname>" also does the same - same thing 
for setting the option with just one leading dash, "-pdb=<explicitname>" 
(I guess it's a getopt feature that allows that as long as it isn't 
ambiguous with single-letter options?), as well as the form "-pdb=" or 
"--pdb=" for requesting it to set the default name?

// Martin


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-03  5:12 ` [PATCH 1/2] ld: Add --pdb option Martin Storsjö
@ 2022-10-03 16:57   ` Mark Harmstone
  2022-10-03 18:58     ` Martin Storsjö
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Harmstone @ 2022-10-03 16:57 UTC (permalink / raw)
  To: Martin Storsjö; +Cc: binutils

Hi Martin,

> As I assume you're aware, lld's mingw port also supports PDB generation - and the description of this option also sounds like it's chosen to match lld's option for outputting PDB files - that's good!

Yes, that's right. One notable difference is that the parameter here is optional, unlike with lld, making it a lot easier to fit this into e.g. CMake toolchain files or LDFLAGS.

> The testcase only seemed to exercise the form --pdb=<explicitname>, so I thought I'd ask just for clarity: I guess the other forms of specifying the option, e.g. "--pdb <explicitname>" also does the same - same thing for setting the option with just one leading dash, "-pdb=<explicitname>" (I guess it's a getopt feature that allows that as long as it isn't ambiguous with single-letter options?), as well as the form "-pdb=" or "--pdb=" for requesting it to set the default name?

Apparently provided that the option doesn't begin with an O, you can use either single dashes or double dashes. It looks like the equals sign is mandatory when providing optional parameters, otherwise it interprets the filename as another parameter. But it does mean that the form "-pdb=out.pdb" will work on both ld and lld, which I think is the most important thing.

Mark


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-03 16:57   ` Mark Harmstone
@ 2022-10-03 18:58     ` Martin Storsjö
  2022-10-07 12:16       ` Martin Storsjö
  0 siblings, 1 reply; 11+ messages in thread
From: Martin Storsjö @ 2022-10-03 18:58 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

On Mon, 3 Oct 2022, Mark Harmstone wrote:

> Hi Martin,
>
>> As I assume you're aware, lld's mingw port also supports PDB generation - 
>> and the description of this option also sounds like it's chosen to match 
>> lld's option for outputting PDB files - that's good!
>
> Yes, that's right. One notable difference is that the parameter here is 
> optional, unlike with lld, making it a lot easier to fit this into e.g. CMake 
> toolchain files or LDFLAGS.

LLD also has got that behaviour, since 
https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 
in 2019. That's in particular why I wanted to make sure that this case 
works the same in binutils too.

> It looks like the equals sign is mandatory when providing optional 
> parameters, otherwise it interprets the filename as another parameter.

Yep, that's the case in LLD too.

Unfortunately I didn't think of this behaviour initially when I first 
added this option - otherwise we could have had e.g. --pdb as a boolean 
option to just output to the default name, and e.g. --output-pdb=<name> if 
you wanted to specify the name. But oh well, "-pdb=" works, and I guess it 
isn't the worst thing in the world.

> But it does mean that the form "-pdb=out.pdb" will work on both ld and 
> lld, which I think is the most important thing.

TBH, I consider the "-pdb=" case equally important too - that's what most 
people would use in the end.

// Martin


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-03  1:43 [PATCH 1/2] ld: Add --pdb option Mark Harmstone
  2022-10-03  1:43 ` [PATCH 2/2] ld: Add minimal pdb generation Mark Harmstone
  2022-10-03  5:12 ` [PATCH 1/2] ld: Add --pdb option Martin Storsjö
@ 2022-10-05  4:20 ` Alan Modra
  2 siblings, 0 replies; 11+ messages in thread
From: Alan Modra @ 2022-10-05  4:20 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

On Mon, Oct 03, 2022 at 02:43:12AM +0100, Mark Harmstone wrote:
> @@ -955,6 +960,13 @@ gld${EMULATION_NAME}_handle_option (int optc)
>        if (strcmp (optarg, "none"))
>  	emit_build_id = xstrdup (optarg);
>        break;
> +    case OPTION_PDB:
> +      if (emit_build_id == NULL)
> +	emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
> +      pdb = 1;
> +      if (optarg)
> +	pdb_name = xstrdup (optarg);
> +      break;

This will result in "--emit-build-id=none --pdb" enabling build-id
while "--pdb --emit-build-id=none" will disable build-id.  Is that
what you want?

If the intent is to always force build-id on then you should do so in
after_parse instead.  Otherwise both patches look OK to me.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-03 18:58     ` Martin Storsjö
@ 2022-10-07 12:16       ` Martin Storsjö
  2022-10-09 23:46         ` Mark Harmstone
  0 siblings, 1 reply; 11+ messages in thread
From: Martin Storsjö @ 2022-10-07 12:16 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

[-- Attachment #1: Type: text/plain, Size: 4163 bytes --]

On Mon, 3 Oct 2022, Martin Storsjö wrote:

> On Mon, 3 Oct 2022, Mark Harmstone wrote:
>
>> Hi Martin,
>> 
>>> As I assume you're aware, lld's mingw port also supports PDB generation - 
>>> and the description of this option also sounds like it's chosen to match 
>>> lld's option for outputting PDB files - that's good!
>> 
>> Yes, that's right. One notable difference is that the parameter here is 
>> optional, unlike with lld, making it a lot easier to fit this into e.g. 
>> CMake toolchain files or LDFLAGS.
>
> LLD also has got that behaviour, since 
> https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 
> in 2019. That's in particular why I wanted to make sure that this case works 
> the same in binutils too.
>
>> It looks like the equals sign is mandatory when providing optional 
>> parameters, otherwise it interprets the filename as another parameter.
>
> Yep, that's the case in LLD too.
>
> Unfortunately I didn't think of this behaviour initially when I first added 
> this option - otherwise we could have had e.g. --pdb as a boolean option to 
> just output to the default name, and e.g. --output-pdb=<name> if you wanted 
> to specify the name. But oh well, "-pdb=" works, and I guess it isn't the 
> worst thing in the world.
>
>> But it does mean that the form "-pdb=out.pdb" will work on both ld and lld, 
>> which I think is the most important thing.
>
> TBH, I consider the "-pdb=" case equally important too - that's what most 
> people would use in the end.

FWIW, I'm actually a bit concerned about the interop between binutils and 
lld here. I don't want interop between binutils and lld to work only for 
some subset of the used parameter forms, I'd like it to work for all 
commonly used forms.


First off, the (slightly awkward) syntax that lld uses for an optional 
empty output name, "-pdb=" really should be handled by binutils too - 
handling that doesn't conflict with anything else and should be simple to 
support.

This is the format of the option that I've been recommending people to 
use, and this has been in use in third party projects for years already - 
e.g. this: 
https://code.videolan.org/videolan/vlc/-/blob/master/configure.ac#L429

This should be trivial to support in your patch:

diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 11216830dd3..538fdf5054b 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -926,7 +926,7 @@ gld${EMULATION_NAME}_handle_option (int optc)
        if (emit_build_id == NULL)
         emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
        pdb = 1;
-      if (optarg)
+      if (optarg && optarg[0])
         pdb_name = xstrdup (optarg);
        break;
      }

(And the same for pe.em.)


Secondly, for explicitly naming an output file, I've documented to end 
users that they can use either -Wl,-pdb=<filename> or -Wl,-pdb,<filename> 
- see 
https://github.com/mstorsjo/llvm-mingw/blob/master/README.md?plain=1#L175.

In the original implementation in the mingw frontend in lld in 2018, the 
"-pdb <output>" format was the only format for the option: 
https://github.com/llvm/llvm-project/commit/b7d50115ba4900da6db7afb6460ad42ff19ba6a2

Only one year later with the implicit output name, the "-pdb=<output>" and 
"-pdb=" form was added: 
https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3

In one of my test scripts, I use the initial form of the option, 
-Wl,-pdb,<filename>:
https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L234

It seems like Wine has picked up on the -Wl,-pdb,<name> form:
https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/tools/winegcc/winegcc.c#L467

Also here are a couple of other cases I found that all seem to use that 
form:
https://youtrack.jetbrains.com/issue/KT-47175/How-to-generate-kotlin-native-debug-info-filesPDB-on-windows-platform
https://git.kernel.dk/?p=fio.git;a=commitdiff;h=76bc30ca118fda404f19c17d97bafdba9779c4c2

So with all these users, I'd be kinda hesitant to change lld's 
interpretation of this option form, and to have binutils ld in parallel 
interpreting that form differently. What do you think?


// Martin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-07 12:16       ` Martin Storsjö
@ 2022-10-09 23:46         ` Mark Harmstone
  2022-10-10 10:27           ` Martin Storsjö
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Harmstone @ 2022-10-09 23:46 UTC (permalink / raw)
  To: Martin Storsjö; +Cc: binutils

On 7/10/22 13:16, Martin Storsjö wrote:
> On Mon, 3 Oct 2022, Martin Storsjö wrote:
>
>> On Mon, 3 Oct 2022, Mark Harmstone wrote:
>>
>>> Hi Martin,
>>>
>>>> As I assume you're aware, lld's mingw port also supports PDB generation - and the description of this option also sounds like it's chosen to match lld's option for outputting PDB files - that's good!
>>>
>>> Yes, that's right. One notable difference is that the parameter here is optional, unlike with lld, making it a lot easier to fit this into e.g. CMake toolchain files or LDFLAGS.
>>
>> LLD also has got that behaviour, since https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 in 2019. That's in particular why I wanted to make sure that this case works the same in binutils too.
>>
>>> It looks like the equals sign is mandatory when providing optional parameters, otherwise it interprets the filename as another parameter.
>>
>> Yep, that's the case in LLD too.
>>
>> Unfortunately I didn't think of this behaviour initially when I first added this option - otherwise we could have had e.g. --pdb as a boolean option to just output to the default name, and e.g. --output-pdb=<name> if you wanted to specify the name. But oh well, "-pdb=" works, and I guess it isn't the worst thing in the world.
>>
>>> But it does mean that the form "-pdb=out.pdb" will work on both ld and lld, which I think is the most important thing.
>>
>> TBH, I consider the "-pdb=" case equally important too - that's what most people would use in the end.
>
> FWIW, I'm actually a bit concerned about the interop between binutils and lld here. I don't want interop between binutils and lld to work only for some subset of the used parameter forms, I'd like it to work for all commonly used forms.
>
>
> First off, the (slightly awkward) syntax that lld uses for an optional empty output name, "-pdb=" really should be handled by binutils too - handling that doesn't conflict with anything else and should be simple to support.
>
> This is the format of the option that I've been recommending people to use, and this has been in use in third party projects for years already - e.g. this: https://code.videolan.org/videolan/vlc/-/blob/master/configure.ac#L429
>
> This should be trivial to support in your patch:
>
> diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
> index 11216830dd3..538fdf5054b 100644
> --- a/ld/emultempl/pep.em
> +++ b/ld/emultempl/pep.em
> @@ -926,7 +926,7 @@ gld${EMULATION_NAME}_handle_option (int optc)
>        if (emit_build_id == NULL)
>         emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
>        pdb = 1;
> -      if (optarg)
> +      if (optarg && optarg[0])
>         pdb_name = xstrdup (optarg);
>        break;
>      }
>
> (And the same for pe.em.)
>
>
> Secondly, for explicitly naming an output file, I've documented to end users that they can use either -Wl,-pdb=<filename> or -Wl,-pdb,<filename> - see https://github.com/mstorsjo/llvm-mingw/blob/master/README.md?plain=1#L175.
>
> In the original implementation in the mingw frontend in lld in 2018, the "-pdb <output>" format was the only format for the option: https://github.com/llvm/llvm-project/commit/b7d50115ba4900da6db7afb6460ad42ff19ba6a2
>
> Only one year later with the implicit output name, the "-pdb=<output>" and "-pdb=" form was added: https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3
>
> In one of my test scripts, I use the initial form of the option, -Wl,-pdb,<filename>:
> https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L234
>
> It seems like Wine has picked up on the -Wl,-pdb,<name> form:
> https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/tools/winegcc/winegcc.c#L467
>
> Also here are a couple of other cases I found that all seem to use that form:
> https://youtrack.jetbrains.com/issue/KT-47175/How-to-generate-kotlin-native-debug-info-filesPDB-on-windows-platform
> https://git.kernel.dk/?p=fio.git;a=commitdiff;h=76bc30ca118fda404f19c17d97bafdba9779c4c2
>
> So with all these users, I'd be kinda hesitant to change lld's interpretation of this option form, and to have binutils ld in parallel interpreting that form differently. What do you think?
>
>
> // Martin
Hi Martin,

Fair enough - I'm not overly wedded to this, and will change it if, as you say, it'll cause issues elsewhere.

Mark



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-09 23:46         ` Mark Harmstone
@ 2022-10-10 10:27           ` Martin Storsjö
  2022-10-10 16:55             ` Mark Harmstone
  0 siblings, 1 reply; 11+ messages in thread
From: Martin Storsjö @ 2022-10-10 10:27 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

[-- Attachment #1: Type: text/plain, Size: 5332 bytes --]

On Mon, 10 Oct 2022, Mark Harmstone wrote:

> On 7/10/22 13:16, Martin Storsjö wrote:
>> On Mon, 3 Oct 2022, Martin Storsjö wrote:
>> 
>>> On Mon, 3 Oct 2022, Mark Harmstone wrote:
>>> 
>>>> Hi Martin,
>>>> 
>>>>> As I assume you're aware, lld's mingw port also supports PDB generation 
>>>>> - and the description of this option also sounds like it's chosen to 
>>>>> match lld's option for outputting PDB files - that's good!
>>>> 
>>>> Yes, that's right. One notable difference is that the parameter here is 
>>>> optional, unlike with lld, making it a lot easier to fit this into e.g. 
>>>> CMake toolchain files or LDFLAGS.
>>> 
>>> LLD also has got that behaviour, since 
>>> https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 
>>> in 2019. That's in particular why I wanted to make sure that this case 
>>> works the same in binutils too.
>>> 
>>>> It looks like the equals sign is mandatory when providing optional 
>>>> parameters, otherwise it interprets the filename as another parameter.
>>> 
>>> Yep, that's the case in LLD too.
>>> 
>>> Unfortunately I didn't think of this behaviour initially when I first 
>>> added this option - otherwise we could have had e.g. --pdb as a boolean 
>>> option to just output to the default name, and e.g. --output-pdb=<name> if 
>>> you wanted to specify the name. But oh well, "-pdb=" works, and I guess it 
>>> isn't the worst thing in the world.
>>> 
>>>> But it does mean that the form "-pdb=out.pdb" will work on both ld and 
>>>> lld, which I think is the most important thing.
>>> 
>>> TBH, I consider the "-pdb=" case equally important too - that's what most 
>>> people would use in the end.
>> 
>> FWIW, I'm actually a bit concerned about the interop between binutils and 
>> lld here. I don't want interop between binutils and lld to work only for 
>> some subset of the used parameter forms, I'd like it to work for all 
>> commonly used forms.
>> 
>> 
>> First off, the (slightly awkward) syntax that lld uses for an optional 
>> empty output name, "-pdb=" really should be handled by binutils too - 
>> handling that doesn't conflict with anything else and should be simple to 
>> support.
>> 
>> This is the format of the option that I've been recommending people to use, 
>> and this has been in use in third party projects for years already - e.g. 
>> this: 
>> https://code.videolan.org/videolan/vlc/-/blob/master/configure.ac#L429
>> 
>> This should be trivial to support in your patch:
>> 
>> diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
>> index 11216830dd3..538fdf5054b 100644
>> --- a/ld/emultempl/pep.em
>> +++ b/ld/emultempl/pep.em
>> @@ -926,7 +926,7 @@ gld${EMULATION_NAME}_handle_option (int optc)
>>        if (emit_build_id == NULL)
>>         emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
>>        pdb = 1;
>> -      if (optarg)
>> +      if (optarg && optarg[0])
>>         pdb_name = xstrdup (optarg);
>>        break;
>>      }
>> 
>> (And the same for pe.em.)
>> 
>> 
>> Secondly, for explicitly naming an output file, I've documented to end 
>> users that they can use either -Wl,-pdb=<filename> or -Wl,-pdb,<filename> - 
>> see 
>> https://github.com/mstorsjo/llvm-mingw/blob/master/README.md?plain=1#L175.
>> 
>> In the original implementation in the mingw frontend in lld in 2018, the 
>> "-pdb <output>" format was the only format for the option: 
>> https://github.com/llvm/llvm-project/commit/b7d50115ba4900da6db7afb6460ad42ff19ba6a2
>> 
>> Only one year later with the implicit output name, the "-pdb=<output>" and 
>> "-pdb=" form was added: 
>> https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3
>> 
>> In one of my test scripts, I use the initial form of the option, 
>> -Wl,-pdb,<filename>:
>> https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L234
>> 
>> It seems like Wine has picked up on the -Wl,-pdb,<name> form:
>> https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/tools/winegcc/winegcc.c#L467
>> 
>> Also here are a couple of other cases I found that all seem to use that 
>> form:
>> https://youtrack.jetbrains.com/issue/KT-47175/How-to-generate-kotlin-native-debug-info-filesPDB-on-windows-platform
>> https://git.kernel.dk/?p=fio.git;a=commitdiff;h=76bc30ca118fda404f19c17d97bafdba9779c4c2
>> 
>> So with all these users, I'd be kinda hesitant to change lld's 
>> interpretation of this option form, and to have binutils ld in parallel 
>> interpreting that form differently. What do you think?
>> 
>> 
>> // Martin
> Hi Martin,
>
> Fair enough - I'm not overly wedded to this, and will change it if, as you 
> say, it'll cause issues elsewhere.

Ok, great, thanks!

However this patchset also lost the ability to get an automatically chosen 
output file name, which currently is used via the slightly awkward syntax 
"--pdb=" without an empty parameter.

I see you refactored a bit of code in this revision of the patch, which 
lost that ability. With the patch I'm attaching, applied on top of v1 of 
your patch, I think it behaves as a reasonable compromise; getopt's 
required_argument does allow the --pdb=<name> form too (which I think is 
the one we still should recommend going forward), and passing "--pdb=" 
allows implying the automatic naming behaviour.

// Martin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-squash-ld-pdb-Switch-the-pdb-option-to-required_argu.patch, Size: 3629 bytes --]

From d8c64c3fb8dacbbf7c8dcbc2e394a2f9cfc8bd20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin@martin.st>
Date: Mon, 10 Oct 2022 13:23:26 +0300
Subject: [PATCH] squash: ld: pdb: Switch the --pdb option to required_argument

Allow passing an empty parameter to it, to signal an automatically
chosen file name.
---
 ld/emultempl/pe.em  | 6 +++---
 ld/emultempl/pep.em | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index abbee62fd66..2f05303826d 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -388,7 +388,7 @@ gld${EMULATION_NAME}_add_options
     {"tsaware", no_argument, NULL, OPTION_TERMINAL_SERVER_AWARE},
     {"disable-tsaware", no_argument, NULL, OPTION_DISABLE_TERMINAL_SERVER_AWARE},
     {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
-    {"pdb", optional_argument, NULL, OPTION_PDB},
+    {"pdb", required_argument, NULL, OPTION_PDB},
     {"enable-reloc-section", no_argument, NULL, OPTION_ENABLE_RELOC_SECTION},
     {"disable-reloc-section", no_argument, NULL, OPTION_DISABLE_RELOC_SECTION},
     {NULL, no_argument, NULL, 0}
@@ -538,7 +538,7 @@ gld${EMULATION_NAME}_list_options (FILE *file)
   fprintf (file, _("  --[disable-]wdmdriver              Driver uses the WDM model\n"));
   fprintf (file, _("  --[disable-]tsaware                Image is Terminal Server aware\n"));
   fprintf (file, _("  --build-id[=STYLE]                 Generate build ID\n"));
-  fprintf (file, _("  --pdb[=FILENAME]                   Generate PDB file\n"));
+  fprintf (file, _("  --pdb=[FILENAME]                   Generate PDB file\n"));
 }
 
 /* A case insensitive comparison, regardless of the host platform, used for
@@ -983,7 +983,7 @@ gld${EMULATION_NAME}_handle_option (int optc)
       if (emit_build_id == NULL)
 	emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
       pdb = 1;
-      if (optarg)
+      if (optarg && optarg[0])
 	pdb_name = xstrdup (optarg);
       break;
     }
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 11216830dd3..266ea9692c2 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -365,7 +365,7 @@ gld${EMULATION_NAME}_add_options
     {"insert-timestamp", no_argument, NULL, OPTION_INSERT_TIMESTAMP},
     {"no-insert-timestamp", no_argument, NULL, OPTION_NO_INSERT_TIMESTAMP},
     {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
-    {"pdb", optional_argument, NULL, OPTION_PDB},
+    {"pdb", required_argument, NULL, OPTION_PDB},
     {"enable-reloc-section", no_argument, NULL, OPTION_ENABLE_RELOC_SECTION},
     {"disable-reloc-section", no_argument, NULL, OPTION_DISABLE_RELOC_SECTION},
     {"disable-high-entropy-va", no_argument, NULL, OPTION_DISABLE_HIGH_ENTROPY_VA},
@@ -513,7 +513,7 @@ gld${EMULATION_NAME}_list_options (FILE *file)
   fprintf (file, _("  --[disable-]wdmdriver              Driver uses the WDM model\n"));
   fprintf (file, _("  --[disable-]tsaware                Image is Terminal Server aware\n"));
   fprintf (file, _("  --build-id[=STYLE]                 Generate build ID\n"));
-  fprintf (file, _("  --pdb[=FILENAME]                   Generate PDB file\n"));
+  fprintf (file, _("  --pdb=[FILENAME]                   Generate PDB file\n"));
 #endif
 }
 
@@ -926,7 +926,7 @@ gld${EMULATION_NAME}_handle_option (int optc)
       if (emit_build_id == NULL)
 	emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE);
       pdb = 1;
-      if (optarg)
+      if (optarg && optarg[0])
 	pdb_name = xstrdup (optarg);
       break;
     }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-10 10:27           ` Martin Storsjö
@ 2022-10-10 16:55             ` Mark Harmstone
  2022-10-10 20:58               ` Martin Storsjö
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Harmstone @ 2022-10-10 16:55 UTC (permalink / raw)
  To: Martin Storsjö; +Cc: binutils

Sorry Martin, I didn't quite get what you meant before. Yes, that's fine; I'll
resubmit with your changes.

Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ld: Add --pdb option
  2022-10-10 16:55             ` Mark Harmstone
@ 2022-10-10 20:58               ` Martin Storsjö
  0 siblings, 0 replies; 11+ messages in thread
From: Martin Storsjö @ 2022-10-10 20:58 UTC (permalink / raw)
  To: Mark Harmstone; +Cc: binutils

On Mon, 10 Oct 2022, Mark Harmstone wrote:

> Sorry Martin, I didn't quite get what you meant before. Yes, that's 
> fine; I'll resubmit with your changes.

Ok, great!

So after this, both lld and binutils would support these usage forms:

Preferred ones:
     -Wl,--pdb=<filename>
     -Wl,--pdb=     (implying the PDB filename from the output name)

Also supported (and used across third party code) but less ideal:
     -Wl,--pdb,<filename>

We could try to discourage (soft-deprecate?) the latter form and try to 
change most accessible third party projects to use the former form which 
is less ambiguous.

If we get most projects switched to using the preferred forms, and enough 
time passes (say a year or two?) we could maybe consider to remove support 
for the other form, leaving the door open for taking that into use as 
"-Wl,--pdb" as a less awkward form for the implicit output name, at some 
point in the future.

// Martin


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-10-10 20:58 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03  1:43 [PATCH 1/2] ld: Add --pdb option Mark Harmstone
2022-10-03  1:43 ` [PATCH 2/2] ld: Add minimal pdb generation Mark Harmstone
2022-10-03  5:12 ` [PATCH 1/2] ld: Add --pdb option Martin Storsjö
2022-10-03 16:57   ` Mark Harmstone
2022-10-03 18:58     ` Martin Storsjö
2022-10-07 12:16       ` Martin Storsjö
2022-10-09 23:46         ` Mark Harmstone
2022-10-10 10:27           ` Martin Storsjö
2022-10-10 16:55             ` Mark Harmstone
2022-10-10 20:58               ` Martin Storsjö
2022-10-05  4:20 ` Alan Modra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).