diff mbox

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, archived
Headers

Commit Message

Simon Marchi Jan. 12, 2020, 8:17 p.m. UTC 
  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(-)

Comments

Terekhov, Mikhail via Gdb-patches Jan. 12, 2020, 11:20 p.m. UTC | #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. UTC | #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. UTC | #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.
Simon Marchi Jan. 13, 2020, 4:54 p.m. UTC | #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. UTC | #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. UTC | #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. UTC | #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
diff mbox

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));
 }