Re: [PATCH] gdb: use std::vector instead of alloca in core_target::get_core_register_section

[Simon Marchi <simon.marchi@polymtl.ca>]

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