gdb: use std::vector instead of alloca in core_target::get_core_register_section

Message ID 20200112201729.489317-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • gdb: use std::vector instead of alloca in core_target::get_core_register_section
Related show

Commit Message

Simon Marchi Jan. 12, 2020, 8:17 p.m.
As I was trying to compile gdb for an m68k host, I got this error:

  CXX    corelow.o
In file included from /binutils-gdb/gdb/gdbsupport/common-defs.h:120,
                 from /binutils-gdb/gdb/defs.h:28,
                 from /binutils-gdb/gdb/corelow.c:20:
/binutils-gdb/gdb/corelow.c: In member function 'void core_target::get_core_register_section(regcache*, const regset*, const char*, int, int, const char*, bool)':
/binutils-gdb/gdb/../include/libiberty.h:727:36: error: 'alloca' bound is unknown [-Werror=alloca-larger-than=]
  727 | # define alloca(x) __builtin_alloca(x)
      |                    ~~~~~~~~~~~~~~~~^~~
/binutils-gdb/gdb/corelow.c:625:23: note: in expansion of macro 'alloca'
  625 |   contents = (char *) alloca (size);
      |                       ^~~~~~

We are using alloca to hold the contents of a the core register
sections.  These sections are typically fairly small, but there is no
realy guarantee, so I think it would be more reasonable to just use
dynamic allocation here.

gdb/ChangeLog:

	* corelow.c (core_target::get_core_register_section): Use
	  std::vector instead of alloca.
---
 gdb/corelow.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.24.1

Comments

Christian Biesinger via gdb-patches Jan. 12, 2020, 11:20 p.m. | #1
On Sun, Jan 12, 2020, 15:17 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> As I was trying to compile gdb for an m68k host, I got this error:

>

>   CXX    corelow.o

> In file included from /binutils-gdb/gdb/gdbsupport/common-defs.h:120,

>                  from /binutils-gdb/gdb/defs.h:28,

>                  from /binutils-gdb/gdb/corelow.c:20:

> /binutils-gdb/gdb/corelow.c: In member function 'void

> core_target::get_core_register_section(regcache*, const regset*, const

> char*, int, int, const char*, bool)':

> /binutils-gdb/gdb/../include/libiberty.h:727:36: error: 'alloca' bound is

> unknown [-Werror=alloca-larger-than=]

>   727 | # define alloca(x) __builtin_alloca(x)

>       |                    ~~~~~~~~~~~~~~~~^~~

> /binutils-gdb/gdb/corelow.c:625:23: note: in expansion of macro 'alloca'

>   625 |   contents = (char *) alloca (size);

>       |                       ^~~~~~

>

> We are using alloca to hold the contents of a the core register

> sections.  These sections are typically fairly small, but there is no

> realy guarantee, so I think it would be more reasonable to just use

> dynamic allocation here.

>


This seems to be in a common file, why did it only fail on m68k?


> gdb/ChangeLog:

>

>         * corelow.c (core_target::get_core_register_section): Use

>           std::vector instead of alloca.

> ---

>  gdb/corelow.c | 9 ++++-----

>  1 file changed, 4 insertions(+), 5 deletions(-)

>

> diff --git a/gdb/corelow.c b/gdb/corelow.c

> index c53bf1df8fd8..e7efa652d62a 100644

> --- a/gdb/corelow.c

> +++ b/gdb/corelow.c

> @@ -594,7 +594,6 @@ core_target::get_core_register_section (struct

> regcache *regcache,

>  {

>    struct bfd_section *section;

>    bfd_size_type size;

> -  char *contents;

>    bool variable_size_section = (regset != NULL

>                                 && regset->flags & REGSET_VARIABLE_SIZE);

>

> @@ -622,8 +621,8 @@ core_target::get_core_register_section (struct

> regcache *regcache,

>                section_name.c_str ());

>      }

>

> -  contents = (char *) alloca (size);

> -  if (! bfd_get_section_contents (core_bfd, section, contents,

> +  std::vector<char> contents (size);

> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>


Since you're touching this, isn't usual style not to have a space after the
! ?

                                  (file_ptr) 0, size))
>      {

>        warning (_("Couldn't read %s registers from `%s' section in core

> file."),

> @@ -633,12 +632,12 @@ core_target::get_core_register_section (struct

> regcache *regcache,

>

>    if (regset != NULL)

>      {

> -      regset->supply_regset (regset, regcache, -1, contents, size);

> +      regset->supply_regset (regset, regcache, -1, contents.data (),

> size);

>        return;

>      }

>

>    gdb_assert (m_core_vec != nullptr);

> -  m_core_vec->core_read_registers (regcache, contents, size, which,

> +  m_core_vec->core_read_registers (regcache, contents.data (), size,

> which,

>                                    (CORE_ADDR) bfd_section_vma (section));

>  }

>

> --

> 2.24.1

>

>
Simon Marchi Jan. 13, 2020, 3:30 a.m. | #2
On 2020-01-12 6:20 p.m., Christian Biesinger via gdb-patches wrote:
> On Sun, Jan 12, 2020, 15:17 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> 

>> As I was trying to compile gdb for an m68k host, I got this error:

>>

>>   CXX    corelow.o

>> In file included from /binutils-gdb/gdb/gdbsupport/common-defs.h:120,

>>                  from /binutils-gdb/gdb/defs.h:28,

>>                  from /binutils-gdb/gdb/corelow.c:20:

>> /binutils-gdb/gdb/corelow.c: In member function 'void

>> core_target::get_core_register_section(regcache*, const regset*, const

>> char*, int, int, const char*, bool)':

>> /binutils-gdb/gdb/../include/libiberty.h:727:36: error: 'alloca' bound is

>> unknown [-Werror=alloca-larger-than=]

>>   727 | # define alloca(x) __builtin_alloca(x)

>>       |                    ~~~~~~~~~~~~~~~~^~~

>> /binutils-gdb/gdb/corelow.c:625:23: note: in expansion of macro 'alloca'

>>   625 |   contents = (char *) alloca (size);

>>       |                       ^~~~~~

>>

>> We are using alloca to hold the contents of a the core register

>> sections.  These sections are typically fairly small, but there is no

>> realy guarantee, so I think it would be more reasonable to just use

>> dynamic allocation here.

>>

> 

> This seems to be in a common file, why did it only fail on m68k?


Hmm, I don't really know.  I presumed that it's just that this warning was enabled in gcc
by default on m68k, and not on other architectures, because it's especially important
for architectures with small amounts of memory like m68k.  I tried to look in the gcc source
and documentation for this, but didn't find anything related to this (I don't know the gcc
internals thoug, so I probably didn't look at the right place).

When I add -Walloca-larger-than=16 to my x86-64 build, I get the same error.  However, note
that I only get it when building with -O2 as well.

>> gdb/ChangeLog:

>>

>>         * corelow.c (core_target::get_core_register_section): Use

>>           std::vector instead of alloca.

>> ---

>>  gdb/corelow.c | 9 ++++-----

>>  1 file changed, 4 insertions(+), 5 deletions(-)

>>

>> diff --git a/gdb/corelow.c b/gdb/corelow.c

>> index c53bf1df8fd8..e7efa652d62a 100644

>> --- a/gdb/corelow.c

>> +++ b/gdb/corelow.c

>> @@ -594,7 +594,6 @@ core_target::get_core_register_section (struct

>> regcache *regcache,

>>  {

>>    struct bfd_section *section;

>>    bfd_size_type size;

>> -  char *contents;

>>    bool variable_size_section = (regset != NULL

>>                                 && regset->flags & REGSET_VARIABLE_SIZE);

>>

>> @@ -622,8 +621,8 @@ core_target::get_core_register_section (struct

>> regcache *regcache,

>>                section_name.c_str ());

>>      }

>>

>> -  contents = (char *) alloca (size);

>> -  if (! bfd_get_section_contents (core_bfd, section, contents,

>> +  std::vector<char> contents (size);

>> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>>

> 

> Since you're touching this, isn't usual style not to have a space after the

> ! ?


I see it it some spots here and there, but no I don't think it's part of the style
we follow.  I've removed the space in my local version of the patch, thanks for
noticing.

Simon
Andreas Schwab Jan. 13, 2020, 10 a.m. | #3
On Jan 12 2020, Simon Marchi wrote:

> Hmm, I don't really know.  I presumed that it's just that this warning was enabled in gcc

> by default on m68k,


No target alters the warning's default.  That must be your local
customization.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Simon Marchi Jan. 13, 2020, 4:54 p.m. | #4
On 2020-01-13 5:00 a.m., Andreas Schwab wrote:
> On Jan 12 2020, Simon Marchi wrote:

> 

>> Hmm, I don't really know.  I presumed that it's just that this warning was enabled in gcc

>> by default on m68k,

> 

> No target alters the warning's default.  That must be your local

> customization.


Ok, then I have no clue why I see this only on m68k.  For reference, when I use
this command line (where I removed all -W flags except -Werror), I still see
the warning:

  m68k-linux-gnu-g++ -x c++  -g -O2   -I. -I/binutils-gdb/gdb -I/binutils-gdb/gdb/config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I/binutils-gdb/gdb/../include/opcode -I/binutils-gdb/gdb/../readline/readline/.. -I/binutils-gdb/gdb/../zlib -I../bfd -I/binutils-gdb/gdb/../bfd -I/binutils-gdb/gdb/../include -I../libdecnumber -I/binutils-gdb/gdb/../libdecnumber  -I/binutils-gdb/gdb/../gnulib/import -I../gnulib/import      -I/binutils-gdb/gdb/.. -pthread -Werror -c -o corelow.o -MT corelow.o -MMD -MP -MF ./.deps/corelow.Tpo /binutils-gdb/gdb/corelow.c

Simon
Tom Tromey Jan. 13, 2020, 5:30 p.m. | #5
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


Simon> We are using alloca to hold the contents of a the core register
Simon> sections.  These sections are typically fairly small, but there is no
Simon> realy guarantee, so I think it would be more reasonable to just use
Simon> dynamic allocation here.

I would be fine with simply avoiding alloca in most, or all, cases.

Simon> gdb/ChangeLog:

Simon> 	* corelow.c (core_target::get_core_register_section): Use
Simon> 	  std::vector instead of alloca.

Looks reasonable to me.

Tom
Simon Marchi Jan. 13, 2020, 7:34 p.m. | #6
On 2020-01-13 12:30 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

> 

> Simon> We are using alloca to hold the contents of a the core register

> Simon> sections.  These sections are typically fairly small, but there is no

> Simon> realy guarantee, so I think it would be more reasonable to just use

> Simon> dynamic allocation here.

> 

> I would be fine with simply avoiding alloca in most, or all, cases.


Agreed.

> Simon> gdb/ChangeLog:

> 

> Simon> 	* corelow.c (core_target::get_core_register_section): Use

> Simon> 	  std::vector instead of alloca.

> 

> Looks reasonable to me.


Ok, thanks all, I've pushed this patch.

Simon
Pedro Alves Jan. 13, 2020, 7:40 p.m. | #7
On 1/12/20 8:17 PM, Simon Marchi wrote:
> -  contents = (char *) alloca (size);

> -  if (! bfd_get_section_contents (core_bfd, section, contents,

> +  std::vector<char> contents (size);

> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>  				  (file_ptr) 0, size))

>      {


gdb::byte_vector

Thanks,
Pedro Alves
Simon Marchi Jan. 13, 2020, 8:02 p.m. | #8
On 2020-01-13 2:40 p.m., Pedro Alves wrote:
> On 1/12/20 8:17 PM, Simon Marchi wrote:

>> -  contents = (char *) alloca (size);

>> -  if (! bfd_get_section_contents (core_bfd, section, contents,

>> +  std::vector<char> contents (size);

>> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>>  				  (file_ptr) 0, size))

>>      {

> 

> gdb::byte_vector


I used std::vector<char> because the original code also used char, and
m_core_vec->core_read_registers also accepts a char * (although I
could/should have used gdb::char_vector anyway).  However, gdb_byte
probably makes more sense here.  I would push the patch below as a fixup
if it looks good to you.

Simon


From 6758886d9f1f0ddce518ffec5269cc440d2fe4f5 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>

Date: Mon, 13 Jan 2020 14:57:38 -0500
Subject: [PATCH] gdb: use gdb::byte_vector instead of std::vector<char> in
 core_target::get_core_register_section

Since the data held by the `contents` variable is arbitrary binary data,
it should have gdb_byte elements, not char elements.  Also, using
gdb::byte_vector is preferable, since it doesn't unnecessarily
zero-initialize the values.

Instead of adding a cast in the call to m_core_vec->core_read_registers,
I have changed core_read_registers' argument to be a gdb_byte* instead
of a char*.

gdb/ChangeLog:

	* gdbcore.h (struct core_fns) <core_read_registers>: Change
	core_reg_sect type to gdb_byte *.
	* arm-nbsd-nat.c (fetch_elfcore_registers): Likewise.
	* cris-tdep.c (fetch_core_registers): Likewise.
	* corelow.c (core_target::get_core_register_section): Change
	type of `contents` to gdb::byte_vector.
---
 gdb/arm-nbsd-nat.c | 2 +-
 gdb/corelow.c      | 2 +-
 gdb/cris-tdep.c    | 2 +-
 gdb/gdbcore.h      | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/arm-nbsd-nat.c b/gdb/arm-nbsd-nat.c
index 1d058a99ac1a..00f919194b95 100644
--- a/gdb/arm-nbsd-nat.c
+++ b/gdb/arm-nbsd-nat.c
@@ -397,7 +397,7 @@ arm_nbsd_nat_target::store_registers (struct regcache *regcache, int regno)

 static void
 fetch_elfcore_registers (struct regcache *regcache,
-			 char *core_reg_sect, unsigned core_reg_size,
+			 gdb_byte *core_reg_sect, unsigned core_reg_size,
 			 int which, CORE_ADDR ignore)
 {
   struct reg gregset;
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 0418ec2506af..5cd058d5993d 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -621,7 +621,7 @@ core_target::get_core_register_section (struct regcache *regcache,
 	       section_name.c_str ());
     }

-  std::vector<char> contents (size);
+  gdb::byte_vector contents (size);
   if (!bfd_get_section_contents (core_bfd, section, contents.data (),
 				 (file_ptr) 0, size))
     {
diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index 22b4db917ca0..6885d237f3a3 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -3793,7 +3793,7 @@ cris_supply_gregset (struct regcache *regcache, cris_elf_gregset_t *gregsetp)

 static void
 fetch_core_registers (struct regcache *regcache,
-		      char *core_reg_sect, unsigned core_reg_size,
+		      gdb_byte *core_reg_sect, unsigned core_reg_size,
                       int which, CORE_ADDR reg_addr)
 {
   cris_elf_gregset_t gregset;
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 5b216e9c1c97..66e81dfe950f 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -213,7 +213,7 @@ struct core_fns
        address X is at location core_reg_sect+x+reg_addr.  */

     void (*core_read_registers) (struct regcache *regcache,
-				 char *core_reg_sect,
+				 gdb_byte *core_reg_sect,
 				 unsigned core_reg_size,
 				 int which, CORE_ADDR reg_addr);

-- 
2.24.1
Pedro Alves Jan. 13, 2020, 10:04 p.m. | #9
On 1/13/20 8:02 PM, Simon Marchi wrote:
> On 2020-01-13 2:40 p.m., Pedro Alves wrote:

>> On 1/12/20 8:17 PM, Simon Marchi wrote:

>>> -  contents = (char *) alloca (size);

>>> -  if (! bfd_get_section_contents (core_bfd, section, contents,

>>> +  std::vector<char> contents (size);

>>> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>>>  				  (file_ptr) 0, size))

>>>      {

>>

>> gdb::byte_vector

> 

> I used std::vector<char> because the original code also used char, and

> m_core_vec->core_read_registers also accepts a char * (although I

> could/should have used gdb::char_vector anyway).  However, gdb_byte

> probably makes more sense here.  I would push the patch below as a fixup

> if it looks good to you.


It does LGTM, thanks!

Pedro Alves
Simon Marchi Jan. 13, 2020, 11:13 p.m. | #10
On 2020-01-13 5:04 p.m., Pedro Alves wrote:
> On 1/13/20 8:02 PM, Simon Marchi wrote:

>> On 2020-01-13 2:40 p.m., Pedro Alves wrote:

>>> On 1/12/20 8:17 PM, Simon Marchi wrote:

>>>> -  contents = (char *) alloca (size);

>>>> -  if (! bfd_get_section_contents (core_bfd, section, contents,

>>>> +  std::vector<char> contents (size);

>>>> +  if (! bfd_get_section_contents (core_bfd, section, contents.data (),

>>>>  				  (file_ptr) 0, size))

>>>>      {

>>>

>>> gdb::byte_vector

>>

>> I used std::vector<char> because the original code also used char, and

>> m_core_vec->core_read_registers also accepts a char * (although I

>> could/should have used gdb::char_vector anyway).  However, gdb_byte

>> probably makes more sense here.  I would push the patch below as a fixup

>> if it looks good to you.

> 

> It does LGTM, thanks!

> 

> Pedro Alves

> 



Thanks, it is now pushed.

Simon

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index c53bf1df8fd8..e7efa652d62a 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -594,7 +594,6 @@  core_target::get_core_register_section (struct regcache *regcache,
 {
   struct bfd_section *section;
   bfd_size_type size;
-  char *contents;
   bool variable_size_section = (regset != NULL
 				&& regset->flags & REGSET_VARIABLE_SIZE);
 
@@ -622,8 +621,8 @@  core_target::get_core_register_section (struct regcache *regcache,
 	       section_name.c_str ());
     }
 
-  contents = (char *) alloca (size);
-  if (! bfd_get_section_contents (core_bfd, section, contents,
+  std::vector<char> contents (size);
+  if (! bfd_get_section_contents (core_bfd, section, contents.data (),
 				  (file_ptr) 0, size))
     {
       warning (_("Couldn't read %s registers from `%s' section in core file."),
@@ -633,12 +632,12 @@  core_target::get_core_register_section (struct regcache *regcache,
 
   if (regset != NULL)
     {
-      regset->supply_regset (regset, regcache, -1, contents, size);
+      regset->supply_regset (regset, regcache, -1, contents.data (), size);
       return;
     }
 
   gdb_assert (m_core_vec != nullptr);
-  m_core_vec->core_read_registers (regcache, contents, size, which,
+  m_core_vec->core_read_registers (regcache, contents.data (), size, which,
 				   (CORE_ADDR) bfd_section_vma (section));
 }