diff mbox series

[1/4] libbacktrace: change all pc related variables to uintptr_t

Message ID 20230120105409.54949-1-gcc@hazardy.de
State Accepted
Headers
Series [1/4] libbacktrace: change all pc related variables to uintptr_t |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Björn Schäpers Jan. 20, 2023, 10:54 a.m. UTC 
  From: Björn Schäpers <bjoern@hazardy.de>

It's the right thing to do, since the PC shouldn't go out of the
uintptr_t domain, and in backtrace_pcinfo the pc is uintptr_t.
This is a preparation for a following patch.

Tested on x86_64-linux and i686-w64-mingw32.

-- >8 --

	* dwarf.c: changed variables holding pc values from uint64_t to
	uintptr_t.

Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
---
 libbacktrace/dwarf.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Ian Lance Taylor Jan. 20, 2023, 10:25 p.m. UTC | #1 
On Fri, Jan 20, 2023 at 2:54 AM Björn Schäpers <gcc@hazardy.de> wrote:
>
> From: Björn Schäpers <bjoern@hazardy.de>
>
> It's the right thing to do, since the PC shouldn't go out of the
> uintptr_t domain, and in backtrace_pcinfo the pc is uintptr_t.
> This is a preparation for a following patch.
>
> Tested on x86_64-linux and i686-w64-mingw32.

Thanks.  Committed like so, with some additional tweaks.

For future reference, when pinging a patch, please reply to the
original patch to maintain the thread.  Or at least mention the
original patch.  It was still on my list, I just hadn't gotten to it.
Thanks.

Ian

Change variables holding PC values from uint64_t to uintptr_t.
Patch by Björn Schäpers.
* dwarf.c (struct function_addrs): Change low and high fields to
uintptr_t.
(struct unit_addrs): Likewise.
(resolve_addr_index): Change address parameter to uintptr_t*.
(add_unit_addr): Change lowpc and highpc parameters to uintptr_t.
(add_function_range): Likewise.
(struct pcrange): Change lowpc and highpc fields to uintptr_t.
(add_low_high_range): Change add_range lowpc and highpc parameters
to uintptr_t.
(add_ranges_from_ranges): Likewise.
(add_ranges_from_rnglists): Likewise.
(add_low_high_range): Chnage lowpc and highpc variables to
uintpr_t.
(add_ranges_from_rnglists): Change some local variables to
uintptr_t.
(add_ranges_from_ranges): Change base parameter to uintptr_t.
(add_ranges_from_rnglists): Likewise.
(read_function_entry): Likewise.
(resolve_addr_index): Add explicit casts to uintptr_t.
(update_pcrange): Likewise.
(add_ranges_from_ranges): Likewise.
(add_ranges_from_rnglists): Likewise.
(read_function_entry): Likewise.
0c193cabe1d8f209359f3ccb8e74cf87b38fc4bc
diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index 2d41f3b0397..8ff1fb3ce3d 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -136,7 +136,7 @@ enum attr_val_encoding
   /* An address.  */
   ATTR_VAL_ADDRESS,
   /* An index into the .debug_addr section, whose value is relative to
-   * the DW_AT_addr_base attribute of the compilation unit.  */
+     the DW_AT_addr_base attribute of the compilation unit.  */
   ATTR_VAL_ADDRESS_INDEX,
   /* A unsigned integer.  */
   ATTR_VAL_UINT,
@@ -274,8 +274,8 @@ struct function
 struct function_addrs
 {
   /* Range is LOW <= PC < HIGH.  */
-  uint64_t low;
-  uint64_t high;
+  uintptr_t low;
+  uintptr_t high;
   /* Function for this address range.  */
   struct function *function;
 };
@@ -356,8 +356,8 @@ struct unit
 struct unit_addrs
 {
   /* Range is LOW <= PC < HIGH.  */
-  uint64_t low;
-  uint64_t high;
+  uintptr_t low;
+  uintptr_t high;
   /* Compilation unit for this address range.  */
   struct unit *u;
 };
@@ -1094,7 +1094,7 @@ resolve_addr_index (const struct dwarf_sections *dwarf_sections,
 		    uint64_t addr_base, int addrsize, int is_bigendian,
 		    uint64_t addr_index,
 		    backtrace_error_callback error_callback, void *data,
-		    uint64_t *address)
+		    uintptr_t *address)
 {
   uint64_t offset;
   struct dwarf_buf addr_buf;
@@ -1115,7 +1115,7 @@ resolve_addr_index (const struct dwarf_sections *dwarf_sections,
   addr_buf.data = data;
   addr_buf.reported_underflow = 0;
 
-  *address = read_address (&addr_buf, addrsize);
+  *address = (uintptr_t) read_address (&addr_buf, addrsize);
   return 1;
 }
 
@@ -1194,7 +1194,7 @@ function_addrs_search (const void *vkey, const void *ventry)
 
 static int
 add_unit_addr (struct backtrace_state *state, void *rdata,
-	       uint64_t lowpc, uint64_t highpc,
+	       uintptr_t lowpc, uintptr_t highpc,
 	       backtrace_error_callback error_callback, void *data,
 	       void *pvec)
 {
@@ -1530,10 +1530,10 @@ lookup_abbrev (struct abbrevs *abbrevs, uint64_t code,
    lowpc/highpc is set or ranges is set.  */
 
 struct pcrange {
-  uint64_t lowpc;		/* The low PC value.  */
+  uintptr_t lowpc;             /* The low PC value.  */
   int have_lowpc;		/* Whether a low PC value was found.  */
   int lowpc_is_addr_index;	/* Whether lowpc is in .debug_addr.  */
-  uint64_t highpc;		/* The high PC value.  */
+  uintptr_t highpc;            /* The high PC value.  */
   int have_highpc;		/* Whether a high PC value was found.  */
   int highpc_is_relative;	/* Whether highpc is relative to lowpc.  */
   int highpc_is_addr_index;	/* Whether highpc is in .debug_addr.  */
@@ -1553,12 +1553,12 @@ update_pcrange (const struct attr* attr, const struct attr_val* val,
     case DW_AT_low_pc:
       if (val->encoding == ATTR_VAL_ADDRESS)
 	{
-	  pcrange->lowpc = val->u.uint;
+	  pcrange->lowpc = (uintptr_t) val->u.uint;
 	  pcrange->have_lowpc = 1;
 	}
       else if (val->encoding == ATTR_VAL_ADDRESS_INDEX)
 	{
-	  pcrange->lowpc = val->u.uint;
+	  pcrange->lowpc = (uintptr_t) val->u.uint;
 	  pcrange->have_lowpc = 1;
 	  pcrange->lowpc_is_addr_index = 1;
 	}
@@ -1567,18 +1567,18 @@ update_pcrange (const struct attr* attr, const struct attr_val* val,
     case DW_AT_high_pc:
       if (val->encoding == ATTR_VAL_ADDRESS)
 	{
-	  pcrange->highpc = val->u.uint;
+	  pcrange->highpc = (uintptr_t) val->u.uint;
 	  pcrange->have_highpc = 1;
 	}
       else if (val->encoding == ATTR_VAL_UINT)
 	{
-	  pcrange->highpc = val->u.uint;
+	  pcrange->highpc = (uintptr_t) val->u.uint;
 	  pcrange->have_highpc = 1;
 	  pcrange->highpc_is_relative = 1;
 	}
       else if (val->encoding == ATTR_VAL_ADDRESS_INDEX)
 	{
-	  pcrange->highpc = val->u.uint;
+	  pcrange->highpc = (uintptr_t) val->u.uint;
 	  pcrange->have_highpc = 1;
 	  pcrange->highpc_is_addr_index = 1;
 	}
@@ -1613,16 +1613,16 @@ add_low_high_range (struct backtrace_state *state,
 		    uintptr_t base_address, int is_bigendian,
 		    struct unit *u, const struct pcrange *pcrange,
 		    int (*add_range) (struct backtrace_state *state,
-				      void *rdata, uint64_t lowpc,
-				      uint64_t highpc,
+				      void *rdata, uintptr_t lowpc,
+				      uintptr_t highpc,
 				      backtrace_error_callback error_callback,
 				      void *data, void *vec),
 		    void *rdata,
 		    backtrace_error_callback error_callback, void *data,
 		    void *vec)
 {
-  uint64_t lowpc;
-  uint64_t highpc;
+  uintptr_t lowpc;
+  uintptr_t highpc;
 
   lowpc = pcrange->lowpc;
   if (pcrange->lowpc_is_addr_index)
@@ -1660,10 +1660,10 @@ add_ranges_from_ranges (
     struct backtrace_state *state,
     const struct dwarf_sections *dwarf_sections,
     uintptr_t base_address, int is_bigendian,
-    struct unit *u, uint64_t base,
+    struct unit *u, uintptr_t base,
     const struct pcrange *pcrange,
     int (*add_range) (struct backtrace_state *state, void *rdata,
-		      uint64_t lowpc, uint64_t highpc,
+		      uintptr_t lowpc, uintptr_t highpc,
 		      backtrace_error_callback error_callback, void *data,
 		      void *vec),
     void *rdata,
@@ -1702,12 +1702,12 @@ add_ranges_from_ranges (
 	break;
 
       if (is_highest_address (low, u->addrsize))
-	base = high;
+	base = (uintptr_t) high;
       else
 	{
 	  if (!add_range (state, rdata, 
-			  low + base + base_address,
-			  high + base + base_address,
+			  (uintptr_t) low + base + base_address,
+			  (uintptr_t) high + base + base_address,
 			  error_callback, data, vec))
 	    return 0;
 	}
@@ -1727,10 +1727,10 @@ add_ranges_from_rnglists (
     struct backtrace_state *state,
     const struct dwarf_sections *dwarf_sections,
     uintptr_t base_address, int is_bigendian,
-    struct unit *u, uint64_t base,
+    struct unit *u, uintptr_t base,
     const struct pcrange *pcrange,
     int (*add_range) (struct backtrace_state *state, void *rdata,
-		      uint64_t lowpc, uint64_t highpc,
+		      uintptr_t lowpc, uintptr_t highpc,
 		      backtrace_error_callback error_callback, void *data,
 		      void *vec),
     void *rdata,
@@ -1796,8 +1796,8 @@ add_ranges_from_rnglists (
 	case DW_RLE_startx_endx:
 	  {
 	    uint64_t index;
-	    uint64_t low;
-	    uint64_t high;
+	    uintptr_t low;
+	    uintptr_t high;
 
 	    index = read_uleb128 (&rnglists_buf);
 	    if (!resolve_addr_index (dwarf_sections, u->addr_base,
@@ -1819,8 +1819,8 @@ add_ranges_from_rnglists (
 	case DW_RLE_startx_length:
 	  {
 	    uint64_t index;
-	    uint64_t low;
-	    uint64_t length;
+	    uintptr_t low;
+	    uintptr_t length;
 
 	    index = read_uleb128 (&rnglists_buf);
 	    if (!resolve_addr_index (dwarf_sections, u->addr_base,
@@ -1850,16 +1850,16 @@ add_ranges_from_rnglists (
 	  break;
 
 	case DW_RLE_base_address:
-	  base = read_address (&rnglists_buf, u->addrsize);
+	  base = (uintptr_t) read_address (&rnglists_buf, u->addrsize);
 	  break;
 
 	case DW_RLE_start_end:
 	  {
-	    uint64_t low;
-	    uint64_t high;
+	    uintptr_t low;
+	    uintptr_t high;
 
-	    low = read_address (&rnglists_buf, u->addrsize);
-	    high = read_address (&rnglists_buf, u->addrsize);
+	    low = (uintptr_t) read_address (&rnglists_buf, u->addrsize);
+	    high = (uintptr_t) read_address (&rnglists_buf, u->addrsize);
 	    if (!add_range (state, rdata, low + base_address,
 			    high + base_address, error_callback, data,
 			    vec))
@@ -1869,11 +1869,11 @@ add_ranges_from_rnglists (
 
 	case DW_RLE_start_length:
 	  {
-	    uint64_t low;
-	    uint64_t length;
+	    uintptr_t low;
+	    uintptr_t length;
 
-	    low = read_address (&rnglists_buf, u->addrsize);
-	    length = read_uleb128 (&rnglists_buf);
+	    low = (uintptr_t) read_address (&rnglists_buf, u->addrsize);
+	    length = (uintptr_t) read_uleb128 (&rnglists_buf);
 	    low += base_address;
 	    if (!add_range (state, rdata, low, low + length,
 			    error_callback, data, vec))
@@ -1903,9 +1903,9 @@ static int
 add_ranges (struct backtrace_state *state,
 	    const struct dwarf_sections *dwarf_sections,
 	    uintptr_t base_address, int is_bigendian,
-	    struct unit *u, uint64_t base, const struct pcrange *pcrange,
+	    struct unit *u, uintptr_t base, const struct pcrange *pcrange,
 	    int (*add_range) (struct backtrace_state *state, void *rdata, 
-			      uint64_t lowpc, uint64_t highpc,
+			      uintptr_t lowpc, uintptr_t highpc,
 			      backtrace_error_callback error_callback,
 			      void *data, void *vec),
 	    void *rdata,
@@ -3183,7 +3183,7 @@ read_referenced_name (struct dwarf_data *ddata, struct unit *u,
 
 static int
 add_function_range (struct backtrace_state *state, void *rdata,
-		    uint64_t lowpc, uint64_t highpc,
+		    uintptr_t lowpc, uintptr_t highpc,
 		    backtrace_error_callback error_callback, void *data,
 		    void *pvec)
 {
@@ -3223,7 +3223,7 @@ add_function_range (struct backtrace_state *state, void *rdata,
 
 static int
 read_function_entry (struct backtrace_state *state, struct dwarf_data *ddata,
-		     struct unit *u, uint64_t base, struct dwarf_buf *unit_buf,
+		     struct unit *u, uintptr_t base, struct dwarf_buf *unit_buf,
 		     const struct line_header *lhdr,
 		     backtrace_error_callback error_callback, void *data,
 		     struct function_vector *vec_function,
@@ -3287,7 +3287,7 @@ read_function_entry (struct backtrace_state *state, struct dwarf_data *ddata,
 	      && abbrev->attrs[i].name == DW_AT_low_pc)
 	    {
 	      if (val.encoding == ATTR_VAL_ADDRESS)
-		base = val.u.uint;
+		base = (uintptr_t) val.u.uint;
 	      else if (val.encoding == ATTR_VAL_ADDRESS_INDEX)
 		{
 		  if (!resolve_addr_index (&ddata->dwarf_sections,
Björn Schäpers Jan. 23, 2023, 8:17 p.m. UTC | #2 
Am 20.01.2023 um 23:25 schrieb Ian Lance Taylor:
> On Fri, Jan 20, 2023 at 2:54 AM Björn Schäpers <gcc@hazardy.de> wrote:
>>
>> From: Björn Schäpers <bjoern@hazardy.de>
>>
>> It's the right thing to do, since the PC shouldn't go out of the
>> uintptr_t domain, and in backtrace_pcinfo the pc is uintptr_t.
>> This is a preparation for a following patch.
>>
>> Tested on x86_64-linux and i686-w64-mingw32.
> 
> Thanks.  Committed like so, with some additional tweaks.
> 
> For future reference, when pinging a patch, please reply to the
> original patch to maintain the thread.  Or at least mention the
> original patch.  It was still on my list, I just hadn't gotten to it.
> Thanks.
> 
> Ian
> 

Thanks for the commit, and sorry for the repost. Because of a fault of my own I 
had no copy in my mailbox and thus couldn't reply to the first batch.

Regards,
Björn.
Eli Zaretskii Jan. 24, 2023, 1:11 p.m. UTC | #3 
> Date: Mon, 23 Jan 2023 15:00:56 -0800
> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: Ian Lance Taylor via Gcc <gcc@gcc.gnu.org>
> 
> > +#ifdef HAVE_WINDOWS_H
> > +
> > +static char *
> > +windows_get_executable_path (char *buf, backtrace_error_callback error_callback,
> > +                            void *data)
> > +{
> > +  if (GetModuleFileNameA (NULL, buf, MAX_PATH - 1) == 0)
> > +    {
> > +      error_callback (data,
> > +                     "could not get the filename of the current executable",
> > +                     (int) GetLastError ());
> > +      return NULL;
> > +    }
> > +  return buf;
> > +}
> 
> Thanks, but this seems incomplete.  The docs for GetModuleFileNameA
> say that if the pathname is too long to fit into the buffer it returns
> the size of the buffer and sets the error to
> ERROR_INSUFFICIENT_BUFFER.  It seems to me that in that case we should
> allocate a larger buffer and try again.

This is correct in general, but not in this particular case.

> On Windows it seems that MAX_PATH is not
> a true limit, as an extended length path may be up to 32767 bytes.

The limit of 32767 characters (not bytes, AFAIK) is only applicable
when using the Unicode (a.k.a. "wide") versions of the Windows Win32
APIs, see

  https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation

Since the above code uses GetModuleFileNameA, which is an "ANSI"
single-byte API, it is still subject to the MAX_PATH limitation, and
MAX_PATH is defined as 260 on Windows headers.
Ian Lance Taylor Jan. 24, 2023, 2:35 p.m. UTC | #4 
On Tue, Jan 24, 2023 at 5:11 AM Eli Zaretskii via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > Date: Mon, 23 Jan 2023 15:00:56 -0800
> > Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> > From: Ian Lance Taylor via Gcc <gcc@gcc.gnu.org>
> >
> > > +#ifdef HAVE_WINDOWS_H
> > > +
> > > +static char *
> > > +windows_get_executable_path (char *buf, backtrace_error_callback error_callback,
> > > +                            void *data)
> > > +{
> > > +  if (GetModuleFileNameA (NULL, buf, MAX_PATH - 1) == 0)
> > > +    {
> > > +      error_callback (data,
> > > +                     "could not get the filename of the current executable",
> > > +                     (int) GetLastError ());
> > > +      return NULL;
> > > +    }
> > > +  return buf;
> > > +}
> >
> > Thanks, but this seems incomplete.  The docs for GetModuleFileNameA
> > say that if the pathname is too long to fit into the buffer it returns
> > the size of the buffer and sets the error to
> > ERROR_INSUFFICIENT_BUFFER.  It seems to me that in that case we should
> > allocate a larger buffer and try again.
>
> This is correct in general, but not in this particular case.
>
> > On Windows it seems that MAX_PATH is not
> > a true limit, as an extended length path may be up to 32767 bytes.
>
> The limit of 32767 characters (not bytes, AFAIK) is only applicable
> when using the Unicode (a.k.a. "wide") versions of the Windows Win32
> APIs, see
>
>   https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
>
> Since the above code uses GetModuleFileNameA, which is an "ANSI"
> single-byte API, it is still subject to the MAX_PATH limitation, and
> MAX_PATH is defined as 260 on Windows headers.

Thanks.  Should this code be using GetModuleFileNameW?  Or would that
mean that the later call to open will fail?

260 bytes does not seem like very much for a path name these days.

Ian
Eli Zaretskii Jan. 24, 2023, 4:52 p.m. UTC | #5 
> From: Ian Lance Taylor <iant@golang.org>
> Date: Tue, 24 Jan 2023 06:35:21 -0800
> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> 
> > > On Windows it seems that MAX_PATH is not
> > > a true limit, as an extended length path may be up to 32767 bytes.
> >
> > The limit of 32767 characters (not bytes, AFAIK) is only applicable
> > when using the Unicode (a.k.a. "wide") versions of the Windows Win32
> > APIs, see
> >
> >   https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
> >
> > Since the above code uses GetModuleFileNameA, which is an "ANSI"
> > single-byte API, it is still subject to the MAX_PATH limitation, and
> > MAX_PATH is defined as 260 on Windows headers.
> 
> Thanks.  Should this code be using GetModuleFileNameW?  Or would that
> mean that the later call to open will fail?

We'd need to use _wopen or somesuch, and the file name will have to be
a wchar_t array, not a char array, yes.  So this is not very practical
when file names need to be passed between functions, unless they are
converted to UTF-8 (and back again before using them in Windows APIs).

And note that even then, the 260-byte limit could be lifted only if
the user has a new enough Windows version _and_ has opted in to the
long-name feature by turning it on in the Registry.  Otherwise, file
names used in "wide" APIs can only break the 260-byte limit if they
use the special format "\\?\D:\foo\bar", which means file names
specified by user outside of the program or file names that come from
other programs will need to be reformatted to this special format.

> 260 bytes does not seem like very much for a path name these days.

That's true.  But complications with using longer file names are still
a PITA on Windows, even though they are a step closer to practically
possible.
Ian Lance Taylor Jan. 24, 2023, 5:58 p.m. UTC | #6 
On Tue, Jan 24, 2023 at 8:53 AM Eli Zaretskii via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > From: Ian Lance Taylor <iant@golang.org>
> > Date: Tue, 24 Jan 2023 06:35:21 -0800
> > Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> >
> > > > On Windows it seems that MAX_PATH is not
> > > > a true limit, as an extended length path may be up to 32767 bytes.
> > >
> > > The limit of 32767 characters (not bytes, AFAIK) is only applicable
> > > when using the Unicode (a.k.a. "wide") versions of the Windows Win32
> > > APIs, see
> > >
> > >   https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
> > >
> > > Since the above code uses GetModuleFileNameA, which is an "ANSI"
> > > single-byte API, it is still subject to the MAX_PATH limitation, and
> > > MAX_PATH is defined as 260 on Windows headers.
> >
> > Thanks.  Should this code be using GetModuleFileNameW?  Or would that
> > mean that the later call to open will fail?
>
> We'd need to use _wopen or somesuch, and the file name will have to be
> a wchar_t array, not a char array, yes.  So this is not very practical
> when file names need to be passed between functions, unless they are
> converted to UTF-8 (and back again before using them in Windows APIs).
>
> And note that even then, the 260-byte limit could be lifted only if
> the user has a new enough Windows version _and_ has opted in to the
> long-name feature by turning it on in the Registry.  Otherwise, file
> names used in "wide" APIs can only break the 260-byte limit if they
> use the special format "\\?\D:\foo\bar", which means file names
> specified by user outside of the program or file names that come from
> other programs will need to be reformatted to this special format.
>
> > 260 bytes does not seem like very much for a path name these days.
>
> That's true.  But complications with using longer file names are still
> a PITA on Windows, even though they are a step closer to practically
> possible.


OK, thanks.

I'd rather that the patch look like the appended.  Can someone with a
Windows system test to see what that builds and passes the tests?
Thanks.

Ian
Eli Zaretskii Jan. 24, 2023, 6:11 p.m. UTC | #7 
> From: Ian Lance Taylor <iant@golang.org>
> Date: Tue, 24 Jan 2023 09:58:10 -0800
> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> 
> I'd rather that the patch look like the appended.  Can someone with a
> Windows system test to see what that builds and passes the tests?

ENOPATCH
Ian Lance Taylor Jan. 24, 2023, 6:32 p.m. UTC | #8 
On Tue, Jan 24, 2023 at 10:12 AM Eli Zaretskii via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > From: Ian Lance Taylor <iant@golang.org>
> > Date: Tue, 24 Jan 2023 09:58:10 -0800
> > Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> >
> > I'd rather that the patch look like the appended.  Can someone with a
> > Windows system test to see what that builds and passes the tests?
>
> ENOPATCH

Gah.

Ian
diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in
index 94621c2e385..29d1ad3911a 100644
--- a/libbacktrace/config.h.in
+++ b/libbacktrace/config.h.in
@@ -100,6 +100,9 @@
 /* Define to 1 if you have the <unistd.h> header file. */
 #undef HAVE_UNISTD_H
 
+/* Define to 1 if you have the <windows.h> header file. */
+#undef HAVE_WINDOWS_H
+
 /* Define if -lz is available. */
 #undef HAVE_ZLIB
 
diff --git a/libbacktrace/configure b/libbacktrace/configure
index 6af2c04c81a..0a27cfb7799 100755
--- a/libbacktrace/configure
+++ b/libbacktrace/configure
@@ -13409,6 +13409,19 @@ $as_echo "#define HAVE_LOADQUERY 1" >>confdefs.h
 
 fi
 
+for ac_header in windows.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "windows.h" "ac_cv_header_windows_h" "$ac_includes_default"
+if test "x$ac_cv_header_windows_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_WINDOWS_H 1
+_ACEOF
+
+fi
+
+done
+
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/configure.ac b/libbacktrace/configure.ac
index 39e6bf41e35..e3e10abd7b5 100644
--- a/libbacktrace/configure.ac
+++ b/libbacktrace/configure.ac
@@ -377,6 +377,8 @@ if test "$have_loadquery" = "yes"; then
   AC_DEFINE(HAVE_LOADQUERY, 1, [Define if AIX loadquery is available.])
 fi
 
+AC_CHECK_HEADERS(windows.h)
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/fileline.c b/libbacktrace/fileline.c
index 674bf33cdcf..e110b54ee24 100644
--- a/libbacktrace/fileline.c
+++ b/libbacktrace/fileline.c
@@ -47,6 +47,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include <mach-o/dyld.h>
 #endif
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 #include "backtrace.h"
 #include "internal.h"
 
@@ -155,6 +167,27 @@ macho_get_executable_path (struct backtrace_state *state,
 
 #endif /* !defined (HAVE_MACH_O_DYLD_H) */
 
+#ifdef HAVE_WINDOWS_H
+
+static char *
+windows_get_executable_path (char *buf, backtrace_error_callback error_callback,
+			     void *data)
+{
+  size_t got;
+
+  got = GetModuleFileNameA (NULL, buf, MAX_PATH - 1);
+  if (got == 0
+      || (got == MAX_PATH - 1 && GetLastError () == ERROR_INSUFFICIENT_BUFFER))
+    return NULL;
+  return buf;
+}
+
+#else /* !defined (HAVE_WINDOWS_H) */
+
+#define windows_get_executable_path(buf, error_callback, data) NULL
+
+#endif /* !defined (HAVE_WINDOWS_H) */
+
 /* Initialize the fileline information from the executable.  Returns 1
    on success, 0 on failure.  */
 
@@ -168,7 +201,11 @@ fileline_initialize (struct backtrace_state *state,
   int called_error_callback;
   int descriptor;
   const char *filename;
+#ifdef HAVE_WINDOWS_H
+  char buf[MAX_PATH];
+#else
   char buf[64];
+#endif
 
   if (!state->threaded)
     failed = state->fileline_initialization_failed;
@@ -192,7 +229,7 @@ fileline_initialize (struct backtrace_state *state,
 
   descriptor = -1;
   called_error_callback = 0;
-  for (pass = 0; pass < 8; ++pass)
+  for (pass = 0; pass < 9; ++pass)
     {
       int does_not_exist;
 
@@ -224,6 +261,9 @@ fileline_initialize (struct backtrace_state *state,
 	case 7:
 	  filename = macho_get_executable_path (state, error_callback, data);
 	  break;
+	case 8:
+	  filename = windows_get_executable_path (buf, error_callback, data);
+	  break;
 	default:
 	  abort ();
 	}
Björn Schäpers Feb. 5, 2023, 9:20 a.m. UTC | #9 
Am 24.01.2023 um 19:32 schrieb Ian Lance Taylor:
> On Tue, Jan 24, 2023 at 10:12 AM Eli Zaretskii via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>> From: Ian Lance Taylor <iant@golang.org>
>>> Date: Tue, 24 Jan 2023 09:58:10 -0800
>>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>>>
>>> I'd rather that the patch look like the appended.  Can someone with a
>>> Windows system test to see what that builds and passes the tests?
>>
>> ENOPATCH
> 
> Gah.
> 
> Ian
> 
That seems to be my original patch, right? That one I have tested (and 
am actually using) on x86 and x64 windows.
Ian Lance Taylor Feb. 6, 2023, 12:22 a.m. UTC | #10 
On Sun, Feb 5, 2023 at 1:21 AM Björn Schäpers <gcc@hazardy.de> wrote:
>
> Am 24.01.2023 um 19:32 schrieb Ian Lance Taylor:
> > On Tue, Jan 24, 2023 at 10:12 AM Eli Zaretskii via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>> From: Ian Lance Taylor <iant@golang.org>
> >>> Date: Tue, 24 Jan 2023 09:58:10 -0800
> >>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> >>>
> >>> I'd rather that the patch look like the appended.  Can someone with a
> >>> Windows system test to see what that builds and passes the tests?
> >>
> >> ENOPATCH
> >
> > Gah.
> >
> > Ian
> >
> That seems to be my original patch, right? That one I have tested (and
> am actually using) on x86 and x64 windows.

It's very similar but I changed the windows_get_executable_path function.

Ian
Björn Schäpers Nov. 20, 2023, 7:56 p.m. UTC | #11 
Hi,

this is what I'm using with GCC 12 and 13 on my windows machines, rebased onto 
the current HEAD.

Kind regards,
Björn.

Am 06.02.2023 um 01:22 schrieb Ian Lance Taylor:
> On Sun, Feb 5, 2023 at 1:21 AM Björn Schäpers <gcc@hazardy.de> wrote:
>>
>> Am 24.01.2023 um 19:32 schrieb Ian Lance Taylor:
>>> On Tue, Jan 24, 2023 at 10:12 AM Eli Zaretskii via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>>> From: Ian Lance Taylor <iant@golang.org>
>>>>> Date: Tue, 24 Jan 2023 09:58:10 -0800
>>>>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>>>>>
>>>>> I'd rather that the patch look like the appended.  Can someone with a
>>>>> Windows system test to see what that builds and passes the tests?
>>>>
>>>> ENOPATCH
>>>
>>> Gah.
>>>
>>> Ian
>>>
>> That seems to be my original patch, right? That one I have tested (and
>> am actually using) on x86 and x64 windows.
> 
> It's very similar but I changed the windows_get_executable_path function.
> 
> Ian
From e0ee58b71f726606205aa1f0168a724859162c21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Sch=C3=A4pers?= <bjoern@hazardy.de>
Date: Sun, 30 Apr 2023 23:48:18 +0200
Subject: [PATCH 1/3] libbacktrace: detect executable path on windows
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Tested on x86_64-linux with GCC 12, i686-w64-mingw32 and
x86_64-w64-mingw32 with GCC 12 & 13. This patch is rebased onto the
current HEAD.

-- >8 --

	* configure.ac: Add a check for windows.h.
	* configure, config.h.in: Regenerate.
	* fileline.c: Add windows_get_executable_path.
	* fileline.c (fileline_initialize): Add a pass using
	windows_get_executable_path.

Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
---
 libbacktrace/config.h.in  |  3 +++
 libbacktrace/configure    | 13 +++++++++++
 libbacktrace/configure.ac |  2 ++
 libbacktrace/fileline.c   | 49 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in
index a4f5bddddf6..ee2616335c7 100644
--- a/libbacktrace/config.h.in
+++ b/libbacktrace/config.h.in
@@ -104,6 +104,9 @@
 /* Define to 1 if you have the <unistd.h> header file. */
 #undef HAVE_UNISTD_H
 
+/* Define to 1 if you have the <windows.h> header file. */
+#undef HAVE_WINDOWS_H
+
 /* Define if -lz is available. */
 #undef HAVE_ZLIB
 
diff --git a/libbacktrace/configure b/libbacktrace/configure
index 0ccc060901d..7ade966b54d 100755
--- a/libbacktrace/configure
+++ b/libbacktrace/configure
@@ -13509,6 +13509,19 @@ $as_echo "#define HAVE_LOADQUERY 1" >>confdefs.h
 
 fi
 
+for ac_header in windows.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "windows.h" "ac_cv_header_windows_h" "$ac_includes_default"
+if test "x$ac_cv_header_windows_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_WINDOWS_H 1
+_ACEOF
+
+fi
+
+done
+
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/configure.ac b/libbacktrace/configure.ac
index 71cd50f8cdf..00acb42eb6d 100644
--- a/libbacktrace/configure.ac
+++ b/libbacktrace/configure.ac
@@ -379,6 +379,8 @@ if test "$have_loadquery" = "yes"; then
   AC_DEFINE(HAVE_LOADQUERY, 1, [Define if AIX loadquery is available.])
 fi
 
+AC_CHECK_HEADERS(windows.h)
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/fileline.c b/libbacktrace/fileline.c
index 0e560b44e7a..28d752e2625 100644
--- a/libbacktrace/fileline.c
+++ b/libbacktrace/fileline.c
@@ -47,6 +47,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include <mach-o/dyld.h>
 #endif
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 #include "backtrace.h"
 #include "internal.h"
 
@@ -165,6 +177,34 @@ macho_get_executable_path (struct backtrace_state *state,
 
 #endif /* !HAVE_DECL__PGMPTR */
 
+#ifdef HAVE_WINDOWS_H
+
+static char *
+windows_get_executable_path (char *buf, backtrace_error_callback error_callback,
+			     void *data)
+{
+  size_t got;
+  int error;
+
+  got = GetModuleFileNameA (NULL, buf, MAX_PATH - 1);
+  error = GetLastError ();
+  if (got == 0
+      || (got == MAX_PATH - 1 && error == ERROR_INSUFFICIENT_BUFFER))
+    {
+      error_callback (data,
+		      "could not get the filename of the current executable",
+		      error);
+      return NULL;
+    }
+  return buf;
+}
+
+#else /* !defined (HAVE_WINDOWS_H) */
+
+#define windows_get_executable_path(buf, error_callback, data) NULL
+
+#endif /* !defined (HAVE_WINDOWS_H) */
+
 /* Initialize the fileline information from the executable.  Returns 1
    on success, 0 on failure.  */
 
@@ -178,7 +218,11 @@ fileline_initialize (struct backtrace_state *state,
   int called_error_callback;
   int descriptor;
   const char *filename;
+#ifdef HAVE_WINDOWS_H
+  char buf[MAX_PATH];
+#else
   char buf[64];
+#endif
 
   if (!state->threaded)
     failed = state->fileline_initialization_failed;
@@ -202,7 +246,7 @@ fileline_initialize (struct backtrace_state *state,
 
   descriptor = -1;
   called_error_callback = 0;
-  for (pass = 0; pass < 9; ++pass)
+  for (pass = 0; pass < 10; ++pass)
     {
       int does_not_exist;
 
@@ -239,6 +283,9 @@ fileline_initialize (struct backtrace_state *state,
 	case 8:
 	  filename = macho_get_executable_path (state, error_callback, data);
 	  break;
+	case 9:
+	  filename = windows_get_executable_path (buf, error_callback, data);
+	  break;
 	default:
 	  abort ();
 	}
Ian Lance Taylor Nov. 29, 2023, 10:05 p.m. UTC | #12 
On Mon, Nov 20, 2023 at 11:57 AM Björn Schäpers <gcc@hazardy.de> wrote:
>
> this is what I'm using with GCC 12 and 13 on my windows machines, rebased onto
> the current HEAD.

Thanks.  Committed as follows.

Ian

            * fileline.c: Include <windows.h> if available.
            (windows_get_executable_path): New static function.
            (fileline_initialize): Call windows_get_executable_path.
            * configure.ac: Checked for windows.h
            * configure: Regenerate.
            * config.h.in: Regenerate.
0ee01dfacbcc9bc05d11433a69c0a0ac13afa42f
diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in
index a4f5bddddf6..ee2616335c7 100644
--- a/libbacktrace/config.h.in
+++ b/libbacktrace/config.h.in
@@ -104,6 +104,9 @@
 /* Define to 1 if you have the <unistd.h> header file. */
 #undef HAVE_UNISTD_H
 
+/* Define to 1 if you have the <windows.h> header file. */
+#undef HAVE_WINDOWS_H
+
 /* Define if -lz is available. */
 #undef HAVE_ZLIB
 
diff --git a/libbacktrace/configure b/libbacktrace/configure
index 0ccc060901d..7ade966b54d 100755
--- a/libbacktrace/configure
+++ b/libbacktrace/configure
@@ -13509,6 +13509,19 @@ $as_echo "#define HAVE_LOADQUERY 1" >>confdefs.h
 
 fi
 
+for ac_header in windows.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "windows.h" "ac_cv_header_windows_h" "$ac_includes_default"
+if test "x$ac_cv_header_windows_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_WINDOWS_H 1
+_ACEOF
+
+fi
+
+done
+
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/configure.ac b/libbacktrace/configure.ac
index 71cd50f8cdf..00acb42eb6d 100644
--- a/libbacktrace/configure.ac
+++ b/libbacktrace/configure.ac
@@ -379,6 +379,8 @@ if test "$have_loadquery" = "yes"; then
   AC_DEFINE(HAVE_LOADQUERY, 1, [Define if AIX loadquery is available.])
 fi
 
+AC_CHECK_HEADERS(windows.h)
+
 # Check for the fcntl function.
 if test -n "${with_target_subdir}"; then
    case "${host}" in
diff --git a/libbacktrace/fileline.c b/libbacktrace/fileline.c
index 0e560b44e7a..773f3a92969 100644
--- a/libbacktrace/fileline.c
+++ b/libbacktrace/fileline.c
@@ -47,6 +47,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include <mach-o/dyld.h>
 #endif
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 #include "backtrace.h"
 #include "internal.h"
 
@@ -165,6 +177,37 @@ macho_get_executable_path (struct backtrace_state *state,
 
 #endif /* !HAVE_DECL__PGMPTR */
 
+#ifdef HAVE_WINDOWS_H
+
+#define FILENAME_BUF_SIZE (MAX_PATH)
+
+static char *
+windows_get_executable_path (char *buf, backtrace_error_callback error_callback,
+			     void *data)
+{
+  size_t got;
+  int error;
+
+  got = GetModuleFileNameA (NULL, buf, FILENAME_BUF_SIZE - 1);
+  error = GetLastError ();
+  if (got == 0
+      || (got == FILENAME_BUF_SIZE - 1 && error == ERROR_INSUFFICIENT_BUFFER))
+    {
+      error_callback (data,
+		      "could not get the filename of the current executable",
+		      error);
+      return NULL;
+    }
+  return buf;
+}
+
+#else /* !defined (HAVE_WINDOWS_H) */
+
+#define windows_get_executable_path(buf, error_callback, data) NULL
+#define FILENAME_BUF_SIZE 64
+
+#endif /* !defined (HAVE_WINDOWS_H) */
+
 /* Initialize the fileline information from the executable.  Returns 1
    on success, 0 on failure.  */
 
@@ -178,7 +221,7 @@ fileline_initialize (struct backtrace_state *state,
   int called_error_callback;
   int descriptor;
   const char *filename;
-  char buf[64];
+  char buf[FILENAME_BUF_SIZE];
 
   if (!state->threaded)
     failed = state->fileline_initialization_failed;
@@ -202,7 +245,7 @@ fileline_initialize (struct backtrace_state *state,
 
   descriptor = -1;
   called_error_callback = 0;
-  for (pass = 0; pass < 9; ++pass)
+  for (pass = 0; pass < 10; ++pass)
     {
       int does_not_exist;
 
@@ -239,6 +282,9 @@ fileline_initialize (struct backtrace_state *state,
 	case 8:
 	  filename = macho_get_executable_path (state, error_callback, data);
 	  break;
+	case 9:
+	  filename = windows_get_executable_path (buf, error_callback, data);
+	  break;
 	default:
 	  abort ();
 	}
Ian Lance Taylor Nov. 30, 2023, 7:53 p.m. UTC | #13 
On Fri, Jan 20, 2023 at 2:55 AM Björn Schäpers <gcc@hazardy.de> wrote:
>
> From: Björn Schäpers <bjoern@hazardy.de>
>
> Fixes https://github.com/ianlancetaylor/libbacktrace/issues/53, except
> that libraries loaded after the backtrace_initialize are not handled.
> But as far as I can see that's the same for elf.

Thanks, but I don't want a patch that loops using goto statements.
Please rewrite to avoid that.  It may be simpler to call a function.

Also starting with a module count of 1000 seems like a lot.  Do
typical Windows programs load that many modules?

Ian




> Tested on x86_64-linux and i686-w64-mingw32.
>
> -- >8 --
>
>         * pecoff.c (coff_add): New argument for the module handle of the
>         file, to get the base address.
>         * pecoff.c (backtrace_initialize): Iterate over loaded libraries
>         and call coff_add.
>
> Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
> ---
>  libbacktrace/pecoff.c | 76 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/libbacktrace/pecoff.c b/libbacktrace/pecoff.c
> index 296f1357b5f..40395109e51 100644
> --- a/libbacktrace/pecoff.c
> +++ b/libbacktrace/pecoff.c
> @@ -49,6 +49,7 @@ POSSIBILITY OF SUCH DAMAGE.  */
>  #endif
>
>  #include <windows.h>
> +#include <psapi.h>
>  #endif
>
>  /* Coff file header.  */
> @@ -592,7 +593,8 @@ coff_syminfo (struct backtrace_state *state, uintptr_t addr,
>  static int
>  coff_add (struct backtrace_state *state, int descriptor,
>           backtrace_error_callback error_callback, void *data,
> -         fileline *fileline_fn, int *found_sym, int *found_dwarf)
> +         fileline *fileline_fn, int *found_sym, int *found_dwarf,
> +         uintptr_t module_handle ATTRIBUTE_UNUSED)
>  {
>    struct backtrace_view fhdr_view;
>    off_t fhdr_off;
> @@ -623,7 +625,6 @@ coff_add (struct backtrace_state *state, int descriptor,
>    int is_64;
>    uintptr_t image_base;
>    uintptr_t base_address = 0;
> -  uintptr_t module_handle;
>    struct dwarf_sections dwarf_sections;
>
>    *found_sym = 0;
> @@ -871,7 +872,6 @@ coff_add (struct backtrace_state *state, int descriptor,
>      }
>
>  #ifdef HAVE_WINDOWS_H
> -    module_handle = (uintptr_t) GetModuleHandleW (NULL);
>      base_address = module_handle - image_base;
>  #endif
>
> @@ -914,12 +914,80 @@ backtrace_initialize (struct backtrace_state *state,
>    int found_sym;
>    int found_dwarf;
>    fileline coff_fileline_fn;
> +  uintptr_t module_handle = 0;
> +
> +#ifdef HAVE_WINDOWS_H
> +  DWORD i;
> +  DWORD module_count;
> +  DWORD bytes_needed_for_modules;
> +  HMODULE *modules;
> +  char module_name[MAX_PATH];
> +  int module_found_sym;
> +  fileline module_fileline_fn;
> +
> +  module_handle = (uintptr_t) GetModuleHandleW (NULL);
> +#endif
>
>    ret = coff_add (state, descriptor, error_callback, data,
> -                 &coff_fileline_fn, &found_sym, &found_dwarf);
> +                 &coff_fileline_fn, &found_sym, &found_dwarf, module_handle);
>    if (!ret)
>      return 0;
>
> +#ifdef HAVE_WINDOWS_H
> +  module_count = 1000;
> + alloc_modules:
> +  modules = backtrace_alloc (state, module_count * sizeof(HMODULE),
> +                            error_callback, data);
> +  if (modules == NULL)
> +    goto skip_modules;
> +  if (!EnumProcessModules (GetCurrentProcess (), modules, module_count,
> +                          &bytes_needed_for_modules))
> +    {
> +      error_callback(data, "Could not enumerate process modules",
> +                    (int) GetLastError ());
> +      goto free_modules;
> +    }
> +  if (bytes_needed_for_modules > module_count * sizeof(HMODULE))
> +    {
> +      backtrace_free (state, modules, module_count * sizeof(HMODULE),
> +                     error_callback, data);
> +      // Add an extra of 2, if some module is loaded in another thread.
> +      module_count = bytes_needed_for_modules / sizeof(HMODULE) + 2;
> +      modules = NULL;
> +      goto alloc_modules;
> +    }
> +
> +  for (i = 0; i < bytes_needed_for_modules / sizeof(HMODULE); ++i)
> +    {
> +      if (GetModuleFileNameA (modules[i], module_name, MAX_PATH - 1))
> +       {
> +         if (strcmp (filename, module_name) == 0)
> +           continue;
> +
> +         module_handle = (uintptr_t) GetModuleHandleA (module_name);
> +         if (module_handle == 0)
> +           continue;
> +
> +         descriptor = backtrace_open (module_name, error_callback, data, NULL);
> +         if (descriptor < 0)
> +           continue;
> +
> +         coff_add (state, descriptor, error_callback, data,
> +                   &module_fileline_fn, &module_found_sym, &found_dwarf,
> +                   module_handle);
> +         if (module_found_sym)
> +           found_sym = 1;
> +       }
> +    }
> +
> + free_modules:
> +  if (modules)
> +    backtrace_free(state, modules, module_count * sizeof(HMODULE),
> +                  error_callback, data);
> +  modules = NULL;
> + skip_modules:
> +#endif
> +
>    if (!state->threaded)
>      {
>        if (found_sym)
> --
> 2.38.1
>
Eli Zaretskii Nov. 30, 2023, 8:16 p.m. UTC | #14 
> Date: Thu, 30 Nov 2023 11:53:54 -0800
> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: Ian Lance Taylor via Gcc <gcc@gcc.gnu.org>
> 
> Also starting with a module count of 1000 seems like a lot.  Do
> typical Windows programs load that many modules?

Unlikely.  I'd start with 100.
Björn Schäpers Jan. 2, 2024, 11:12 p.m. UTC | #15 
Am 30.11.2023 um 20:53 schrieb Ian Lance Taylor:
> On Fri, Jan 20, 2023 at 2:55 AM Björn Schäpers <gcc@hazardy.de> wrote:
>>
>> From: Björn Schäpers <bjoern@hazardy.de>
>>
>> Fixes https://github.com/ianlancetaylor/libbacktrace/issues/53, except
>> that libraries loaded after the backtrace_initialize are not handled.
>> But as far as I can see that's the same for elf.
> 
> Thanks, but I don't want a patch that loops using goto statements.
> Please rewrite to avoid that.  It may be simpler to call a function.
> 
> Also starting with a module count of 1000 seems like a lot.  Do
> typical Windows programs load that many modules?
> 
> Ian
> 
> 

Rewritten using a function.

If that is commited, could you attribute that commit to me (--author="Björn 
Schäpers <bjoern@hazardy.de>")?

Thanks and kind regards,
Björn.
From bd552716ee7937cad9d54d4966532d6ea6dbc1bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Sch=C3=A4pers?= <bjoern@hazardy.de>
Date: Sun, 30 Apr 2023 23:54:32 +0200
Subject: [PATCH] libbacktrace: get debug information for loaded dlls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes https://github.com/ianlancetaylor/libbacktrace/issues/53, except
that libraries loaded after the backtrace_initialize are not handled.
But as far as I can see that's the same for elf.

Tested on x86_64-linux and i686-w64-mingw32.

-- >8 --

	* pecoff.c (coff_add): New argument for the module handle of the
	file, to get the base address.
	* pecoff.c (backtrace_initialize): Iterate over loaded libraries
	and call coff_add.

Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
---
 libbacktrace/pecoff.c | 104 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 96 insertions(+), 8 deletions(-)

diff --git a/libbacktrace/pecoff.c b/libbacktrace/pecoff.c
index f976a963bf3..3eb9c4a4853 100644
--- a/libbacktrace/pecoff.c
+++ b/libbacktrace/pecoff.c
@@ -49,6 +49,7 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #endif
 
 #include <windows.h>
+#include <psapi.h>
 #endif
 
 /* Coff file header.  */
@@ -592,7 +593,8 @@ coff_syminfo (struct backtrace_state *state, uintptr_t addr,
 static int
 coff_add (struct backtrace_state *state, int descriptor,
 	  backtrace_error_callback error_callback, void *data,
-	  fileline *fileline_fn, int *found_sym, int *found_dwarf)
+	  fileline *fileline_fn, int *found_sym, int *found_dwarf,
+	  uintptr_t module_handle ATTRIBUTE_UNUSED)
 {
   struct backtrace_view fhdr_view;
   off_t fhdr_off;
@@ -870,12 +872,7 @@ coff_add (struct backtrace_state *state, int descriptor,
     }
 
 #ifdef HAVE_WINDOWS_H
-  {
-    uintptr_t module_handle;
-
-    module_handle = (uintptr_t) GetModuleHandle (NULL);
-    base_address = module_handle - image_base;
-  }
+  base_address = module_handle - image_base;
 #endif
 
   if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
@@ -903,6 +900,53 @@ coff_add (struct backtrace_state *state, int descriptor,
   return 0;
 }
 
+#ifdef HAVE_WINDOWS_H
+static void
+free_modules (struct backtrace_state *state,
+	      backtrace_error_callback error_callback, void *data,
+	      HMODULE **modules, DWORD bytes_allocated)
+{
+  backtrace_free (state, *modules, bytes_allocated, error_callback, data);
+  *modules = NULL;
+}
+
+static void
+get_all_modules (struct backtrace_state *state,
+		 backtrace_error_callback error_callback, void *data, 
+		 HMODULE **modules, DWORD *module_count, DWORD *bytes_allocated)
+{
+  DWORD bytes_needed = 0;
+
+  for (;;)
+    {
+      *bytes_allocated = *module_count * sizeof(HMODULE);
+      *modules = backtrace_alloc (state, *bytes_allocated, error_callback, data);
+
+      if (*modules == NULL)
+	return;
+
+      if (!EnumProcessModules (GetCurrentProcess (), *modules, *module_count,
+			       &bytes_needed))
+	{
+	  error_callback(data, "Could not enumerate process modules",
+			 (int) GetLastError ());
+	  free_modules (state, error_callback, data, modules, *bytes_allocated);
+	  return;
+	}
+
+      *module_count = bytes_needed / sizeof(HMODULE);
+      if (bytes_needed <= *bytes_allocated)
+	{
+	  return;
+	}
+
+      free_modules (state, error_callback, data, modules, *bytes_allocated);
+      // Add an extra of 2, of some module is loaded in another thread.
+      *module_count += 2;
+    }
+}
+#endif
+
 /* Initialize the backtrace data we need from an ELF executable.  At
    the ELF level, all we need to do is find the debug info
    sections.  */
@@ -917,12 +961,56 @@ backtrace_initialize (struct backtrace_state *state,
   int found_sym;
   int found_dwarf;
   fileline coff_fileline_fn;
+  uintptr_t module_handle = 0;
+
+#ifdef HAVE_WINDOWS_H
+  DWORD i;
+  DWORD module_count = 100;
+  DWORD bytes_allocated_for_modules = 0;
+  HMODULE *modules = NULL;
+  char module_name[MAX_PATH];
+  int module_found_sym;
+  fileline module_fileline_fn;
+
+  module_handle = (uintptr_t) GetModuleHandle (NULL);
+#endif
 
   ret = coff_add (state, descriptor, error_callback, data,
-		  &coff_fileline_fn, &found_sym, &found_dwarf);
+		  &coff_fileline_fn, &found_sym, &found_dwarf, module_handle);
   if (!ret)
     return 0;
 
+#ifdef HAVE_WINDOWS_H
+  get_all_modules (state, error_callback, data, &modules, 
+		   &module_count, &bytes_allocated_for_modules);
+
+  for (i = 0; i < module_count; ++i)
+    {
+      if (GetModuleFileNameA (modules[i], module_name, MAX_PATH - 1))
+	{
+	  if (strcmp (filename, module_name) == 0)
+	    continue;
+
+	  module_handle = (uintptr_t) GetModuleHandleA (module_name);
+	  if (module_handle == 0)
+	    continue;
+
+	  descriptor = backtrace_open (module_name, error_callback, data, NULL);
+	  if (descriptor < 0)
+	    continue;
+
+	  coff_add (state, descriptor, error_callback, data,
+		    &module_fileline_fn, &module_found_sym, &found_dwarf,
+		    module_handle);
+	  if (module_found_sym)
+	    found_sym = 1;
+	}
+    }
+
+  if (modules)
+    free_modules (state, error_callback, data, &modules, bytes_allocated_for_modules);
+#endif
+
   if (!state->threaded)
     {
       if (found_sym)
Björn Schäpers Jan. 23, 2024, 9:24 p.m. UTC | #16 
Am 03.01.2024 um 00:12 schrieb Björn Schäpers:
> Am 30.11.2023 um 20:53 schrieb Ian Lance Taylor:
>> On Fri, Jan 20, 2023 at 2:55 AM Björn Schäpers <gcc@hazardy.de> wrote:
>>>
>>> From: Björn Schäpers <bjoern@hazardy.de>
>>>
>>> Fixes https://github.com/ianlancetaylor/libbacktrace/issues/53, except
>>> that libraries loaded after the backtrace_initialize are not handled.
>>> But as far as I can see that's the same for elf.
>>
>> Thanks, but I don't want a patch that loops using goto statements.
>> Please rewrite to avoid that.  It may be simpler to call a function.
>>
>> Also starting with a module count of 1000 seems like a lot.  Do
>> typical Windows programs load that many modules?
>>
>> Ian
>>
>>
> 
> Rewritten using a function.
> 
> If that is commited, could you attribute that commit to me (--author="Björn 
> Schäpers <bjoern@hazardy.de>")?
> 
> Thanks and kind regards,
> Björn.

A friendly ping.
diff mbox series

Patch

diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index 45cc9e77e40..0707ccddd3e 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -274,8 +274,8 @@  struct function
 struct function_addrs
 {
   /* Range is LOW <= PC < HIGH.  */
-  uint64_t low;
-  uint64_t high;
+  uintptr_t low;
+  uintptr_t high;
   /* Function for this address range.  */
   struct function *function;
 };
@@ -356,8 +356,8 @@  struct unit
 struct unit_addrs
 {
   /* Range is LOW <= PC < HIGH.  */
-  uint64_t low;
-  uint64_t high;
+  uintptr_t low;
+  uintptr_t high;
   /* Compilation unit for this address range.  */
   struct unit *u;
 };
@@ -1094,7 +1094,7 @@  resolve_addr_index (const struct dwarf_sections *dwarf_sections,
 		    uint64_t addr_base, int addrsize, int is_bigendian,
 		    uint64_t addr_index,
 		    backtrace_error_callback error_callback, void *data,
-		    uint64_t *address)
+		    uintptr_t *address)
 {
   uint64_t offset;
   struct dwarf_buf addr_buf;
@@ -1194,7 +1194,7 @@  function_addrs_search (const void *vkey, const void *ventry)
 
 static int
 add_unit_addr (struct backtrace_state *state, void *rdata,
-	       uint64_t lowpc, uint64_t highpc,
+	       uintptr_t lowpc, uintptr_t highpc,
 	       backtrace_error_callback error_callback, void *data,
 	       void *pvec)
 {
@@ -1530,10 +1530,10 @@  lookup_abbrev (struct abbrevs *abbrevs, uint64_t code,
    lowpc/highpc is set or ranges is set.  */
 
 struct pcrange {
-  uint64_t lowpc;		/* The low PC value.  */
+  uintptr_t lowpc;		/* The low PC value.  */
   int have_lowpc;		/* Whether a low PC value was found.  */
   int lowpc_is_addr_index;	/* Whether lowpc is in .debug_addr.  */
-  uint64_t highpc;		/* The high PC value.  */
+  uintptr_t highpc;		/* The high PC value.  */
   int have_highpc;		/* Whether a high PC value was found.  */
   int highpc_is_relative;	/* Whether highpc is relative to lowpc.  */
   int highpc_is_addr_index;	/* Whether highpc is in .debug_addr.  */
@@ -1613,16 +1613,16 @@  add_low_high_range (struct backtrace_state *state,
 		    uintptr_t base_address, int is_bigendian,
 		    struct unit *u, const struct pcrange *pcrange,
 		    int (*add_range) (struct backtrace_state *state,
-				      void *rdata, uint64_t lowpc,
-				      uint64_t highpc,
+				      void *rdata, uintptr_t lowpc,
+				      uintptr_t highpc,
 				      backtrace_error_callback error_callback,
 				      void *data, void *vec),
 		    void *rdata,
 		    backtrace_error_callback error_callback, void *data,
 		    void *vec)
 {
-  uint64_t lowpc;
-  uint64_t highpc;
+  uintptr_t lowpc;
+  uintptr_t highpc;
 
   lowpc = pcrange->lowpc;
   if (pcrange->lowpc_is_addr_index)
@@ -1663,7 +1663,7 @@  add_ranges_from_ranges (
     struct unit *u, uint64_t base,
     const struct pcrange *pcrange,
     int (*add_range) (struct backtrace_state *state, void *rdata,
-		      uint64_t lowpc, uint64_t highpc,
+		      uintptr_t lowpc, uintptr_t highpc,
 		      backtrace_error_callback error_callback, void *data,
 		      void *vec),
     void *rdata,
@@ -1727,10 +1727,10 @@  add_ranges_from_rnglists (
     struct backtrace_state *state,
     const struct dwarf_sections *dwarf_sections,
     uintptr_t base_address, int is_bigendian,
-    struct unit *u, uint64_t base,
+    struct unit *u, uintptr_t base,
     const struct pcrange *pcrange,
     int (*add_range) (struct backtrace_state *state, void *rdata,
-		      uint64_t lowpc, uint64_t highpc,
+		      uintptr_t lowpc, uintptr_t highpc,
 		      backtrace_error_callback error_callback, void *data,
 		      void *vec),
     void *rdata,
@@ -1796,8 +1796,8 @@  add_ranges_from_rnglists (
 	case DW_RLE_startx_endx:
 	  {
 	    uint64_t index;
-	    uint64_t low;
-	    uint64_t high;
+	    uintptr_t low;
+	    uintptr_t high;
 
 	    index = read_uleb128 (&rnglists_buf);
 	    if (!resolve_addr_index (dwarf_sections, u->addr_base,
@@ -1819,8 +1819,8 @@  add_ranges_from_rnglists (
 	case DW_RLE_startx_length:
 	  {
 	    uint64_t index;
-	    uint64_t low;
-	    uint64_t length;
+	    uintptr_t low;
+	    uintptr_t length;
 
 	    index = read_uleb128 (&rnglists_buf);
 	    if (!resolve_addr_index (dwarf_sections, u->addr_base,
@@ -1905,7 +1905,7 @@  add_ranges (struct backtrace_state *state,
 	    uintptr_t base_address, int is_bigendian,
 	    struct unit *u, uint64_t base, const struct pcrange *pcrange,
 	    int (*add_range) (struct backtrace_state *state, void *rdata, 
-			      uint64_t lowpc, uint64_t highpc,
+			      uintptr_t lowpc, uintptr_t highpc,
 			      backtrace_error_callback error_callback,
 			      void *data, void *vec),
 	    void *rdata,
@@ -3183,7 +3183,7 @@  read_referenced_name (struct dwarf_data *ddata, struct unit *u,
 
 static int
 add_function_range (struct backtrace_state *state, void *rdata,
-		    uint64_t lowpc, uint64_t highpc,
+		    uintptr_t lowpc, uintptr_t highpc,
 		    backtrace_error_callback error_callback, void *data,
 		    void *pvec)
 {
@@ -3223,7 +3223,7 @@  add_function_range (struct backtrace_state *state, void *rdata,
 
 static int
 read_function_entry (struct backtrace_state *state, struct dwarf_data *ddata,
-		     struct unit *u, uint64_t base, struct dwarf_buf *unit_buf,
+		     struct unit *u, uintptr_t base, struct dwarf_buf *unit_buf,
 		     const struct line_header *lhdr,
 		     backtrace_error_callback error_callback, void *data,
 		     struct function_vector *vec_function,