Message ID | 20200112201729.489317-1-simon.marchi@polymtl.ca |
---|---|
State | New, archived |
Headers |
Received: (qmail 9262 invoked by alias); 12 Jan 2020 20:17:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 9254 invoked by uid 89); 12 Jan 2020 20:17:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_SOFTFAIL autolearn=ham version=3.3.1 spammy=fairly, sk:variabl, HContent-Transfer-Encoding:8bit X-HELO: barracuda.ebox.ca Received: from barracuda.ebox.ca (HELO barracuda.ebox.ca) (96.127.255.19) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 12 Jan 2020 20:17:32 +0000 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id yOW4KJkXH2VDYtee (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 12 Jan 2020 15:17:30 -0500 (EST) Received: from simark.lan (unknown [192.222.164.54]) by smtp.ebox.ca (Postfix) with ESMTP id 1E08C441B21; Sun, 12 Jan 2020 15:17:30 -0500 (EST) From: Simon Marchi <simon.marchi@polymtl.ca> To: gdb-patches@sourceware.org Cc: Simon Marchi <simon.marchi@polymtl.ca> Subject: [PATCH] gdb: use std::vector instead of alloca in core_target::get_core_register_section Date: Sun, 12 Jan 2020 15:17:29 -0500 Message-Id: <20200112201729.489317-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
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
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 > >
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
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.
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
>>>>> "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
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
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 --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)); }