Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

Pedro Alves
2017-02-01T15:48:46Z

On 02/01/2017 12:41 PM, Yao Qi wrote:
> On 17-01-27 12:11:09, Pedro Alves wrote:
>>
>> Makes me wonder whether this is the right approach. :-/
>> (No, I don't have a formed opinion for what that would be.)
>>
>
> Hi Pedro,
> What do you mean by "this" here? We can't take MAX_REGISTER_SIZE for
> ever, as we may have bigger and bigger single register. We do need a
> gdbarch specific max register size. Given there is no standard VLA in
> C++, using alloca is the best way we can do so far.
>

I meant "alloca". And having to worry about loops. Along with other
oddities that come with alloca, like alloca(-1), the most common type
of bad huge allocation (I think), crashing gdb vs
xmalloc/new -> malloc_failure causing a recoverable internal
error, for example.

Some options I was pondering:

#1 - Use VLAs anyway.

I.e., assume we can only build with C++ compilers that support VLAs as extension.
GCC does, clang does, Intel's does I believe. ARM's probably does too?
I don't know whether there's actually any C++ compiler that does not.
Maybe MSVC, though I don't know of anyone trying to build GDB with that.
Though arguably, we shouldn't close the door on "standard" compilers like that.

#2 - Switch to heap allocation

Or std::vector or something like that that hides it. It's not clear whether
that would have a noticeable performance impact.

#3 - Use alloca, move inner loop bodies to functions/lambdas

I.e., avoid having to put the allocas before the loops by moving the body
of loops to separate functions, or to lambdas (effectively
the same), like:

void
function ()
{
for (int regnum = 0; regnum < allregisters; regnum++)
{
[&] {
char *buf = (char *) alloca (register_size (gdbarch, regnum));

/// do something with buf.
} ();
}
}

though that may end up looking a bit ... odd if you're not too
familiar with alloca and lambdas. It also has the problem that
a "return" inside that scope doesn't do what one would expect
if the lambda wasn't there (more on that below).

#4 - Like above, but wrap in macros.

I.e., make #3 a bit more explicit and to the point, by wrapping
the lambda syntax with a couple macros. Like:

#define ALLOCA_SCOPE_START [&] ()
#define ALLOCA_SCOPE_END ();

void
function ()
{
for (int regnum = 0; regnum < allregisters; regnum++)
{
ALLOCA_SCOPE_START
{
char *buf = (char *) alloca (register_size (gdbarch, regnum));

// do something
}
ALLOCA_SCOPE_END
}
}

#5 - Like above, but fix the "return" issue too.

It's doable, but it'll require a bit more "magic". See attached patch.
I picked a few random examples of cases where alloca was being moved
to outside a loops in Alan's patch, and converted them (against master).

I've never seen anyone do something like this. I just thought of it
now, tried it, and it works (tried g++ 4.8 / 5.3 / 7)...

The main advantages of something like ALLOCA_SCOPE_START/ALLOCA_SCOPE_END
would be:

- ALLOCA_SCOPE_START/ALLOCA_SCOPE_END stand out, which in case of alloca
is probably a good thing.
- Allows keeping the alloca buffer allocation closer to where it is used.

Disadvantages:

- Might not be up to everyone's taste?

From ce55d77461a5fa429cea8837b09679b301fa9b40 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 1 Feb 2017 15:41:14 +0000
Subject: [PATCH] Introduce and use ALLOCA_SCOPE_START / ALLOCA_SCOPE_END

Allows using alloca in loops.
---
gdb/aarch64-tdep.c | 25 ++++++++-----
gdb/common/alloca-scope.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/common/common-defs.h | 3 ++
gdb/frame.c | 21 ++++++++---
gdb/ia64-tdep.c | 15 ++++++--
5 files changed, 136 insertions(+), 17 deletions(-)
create mode 100644 gdb/common/alloca-scope.h

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 801c03d..accfadb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -62,6 +62,7 @@

#include "opcode/aarch64.h"
#include <algorithm>
+#include "alloca-scope.h"

#define submask(x) ((1L << ((x) + 1)) - 1)
#define bit(obj,st) (((obj) >> (st)) & 1)
@@ -1982,18 +1983,24 @@ aarch64_store_return_value (struct type *type, struct regcache *regs,
for (i = 0; i < elements; i++)
{
int regno = AARCH64_V0_REGNUM + i;
- bfd_byte tmpbuf[MAX_REGISTER_SIZE];

- if (aarch64_debug)
+ ALLOCA_SCOPE_START
{
- debug_printf ("write HFA or HVA return value element %d to %s\n",
- i + 1,
- gdbarch_register_name (gdbarch, regno));
- }
+ bfd_byte *tmpbuf
+ = (bfd_byte *) alloca (register_size (gdbarch, regno));

- memcpy (tmpbuf, valbuf, len);
- regcache_cooked_write (regs, regno, tmpbuf);
- valbuf += len;
+ if (aarch64_debug)
+ {
+ debug_printf ("write HFA or HVA return value element %d to %s\n",
+ i + 1,
+ gdbarch_register_name (gdbarch, regno));
+ }
+
+ memcpy (tmpbuf, valbuf, len);
+ regcache_cooked_write (regs, regno, tmpbuf);
+ valbuf += len;
+ }
+ ALLOCA_SCOPE_END
}
}
else if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type)
diff --git a/gdb/common/alloca-scope.h b/gdb/common/alloca-scope.h
new file mode 100644
index 0000000..311257c
--- /dev/null
+++ b/gdb/common/alloca-scope.h
@@ -0,0 +1,89 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+
+/* Stack allocated with "alloca" is released on function exit, not
+ scope exit. This makes it dangerous to use alloca in loops. To
+ work around that, create an alloca scope using the
+ ALLOCA_SCOPE_START and ALLOCA_SCOPE_END macros like this:
+
+ for (...)
+ {
+ ALLOCA_SCOPE_START
+ {
+ gdb_byte *buf = (gdb_byte *) alloca (some size);
+
+ // use BUF
+ }
+ ALLOCA_SCOPE_END
+ }
+
+ ALLOCA_SCOPE_START / ALLOCA_SCOPE_END define and call a lambda
+ behind the scenes. Any memory allocated with alloca is thus
+ released when the lambda returns, since the lambda is just an
+ (inline) function.
+
+ Note that it's not possible to "return" directly from within an
+ alloca scope, since that would return from the lambda, instead of
+ from the lambda's caller. The macros are defined in such a way
+ that return expressions won't even compile, to avoid such mistakes.
+
+ The scope defined by ALLOCA_SCOPE_START/END has access to all the
+ local variables of the containing scope (because the lambda
+ captures all by reference).
+
+ This wrapping has no run time overhead, since the lambda never
+ escapes out of the local, calling scope.
+ __attribute__((always_inline)) also helps a bit, by instructing the
+ compiler that it should inline everything about the lambda,
+ regardless of optimization level.
+*/
+
+#ifndef COMMON_ALLOCA_SCOPE_H
+#define COMMON_ALLOCA_SCOPE_H
+
+#include <alloca.h>
+
+/* A couple types used to prevent trying to "return" from within an
+ ALLOCA_SCOPE lambda. */
+
+struct alloca_scopes_cannot_return_empty_base {};
+
+/* Inheriting from an empty class prevents "return {};" inside the
+ lambda, by making alloca_scopes_cannot_return a non-aggregate. */
+struct alloca_scopes_cannot_return : alloca_scopes_cannot_return_empty_base
+{
+private:
+ alloca_scopes_cannot_return () = default;
+
+public:
+ static inline alloca_scopes_cannot_return make ()
+ ATTRIBUTE_ALWAYS_INLINE
+ { return alloca_scopes_cannot_return({}); }
+};
+
+/* Wraps a block in a lambda, so that stack memory "allocated" with
+ alloca is released on scope end. */
+#define ALLOCA_SCOPE_START \
+ [&] () ATTRIBUTE_ALWAYS_INLINE -> alloca_scopes_cannot_return \
+ {
+
+#define ALLOCA_SCOPE_END \
+ return alloca_scopes_cannot_return::make(); \
+ } ();
+
+#endif /* COMMON_ALLOCA_SCOPE_H */
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index af37111..ff54a45 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -69,6 +69,9 @@
#undef ATTRIBUTE_PRINTF
#define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF

+/* Could go to ansidecl.h. */
+#define ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
+
#include "libiberty.h"
#include "pathmax.h"
#include "gdb/signals.h"
diff --git a/gdb/frame.c b/gdb/frame.c
index d98003d..3fae7b4 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -42,6 +42,7 @@
#include "tracepoint.h"
#include "hashtab.h"
#include "valprint.h"
+#include "alloca-scope.h"

/* The sentinel frame terminates the innermost end of the frame chain.
If unwound, it returns the information needed to construct an
@@ -1410,16 +1411,26 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,
}
else
{
- gdb_byte buf[MAX_REGISTER_SIZE];
enum lval_type lval;
CORE_ADDR addr;
int realnum;
+ bool valid;

- frame_register (frame, regnum, optimizedp, unavailablep,
- &lval, &addr, &realnum, buf);
- if (*optimizedp || *unavailablep)
+ ALLOCA_SCOPE_START
+ {
+ gdb_byte *buf
+ = (gdb_byte *) alloca (register_size (gdbarch, regnum));
+
+ frame_register (frame, regnum, optimizedp, unavailablep,
+ &lval, &addr, &realnum, buf);
+ valid = !*optimizedp && !*unavailablep;
+ if (valid)
+ memcpy (myaddr, buf + offset, curr_len);
+ }
+ ALLOCA_SCOPE_END
+
+ if (!valid)
return 0;
- memcpy (myaddr, buf + offset, curr_len);
}

myaddr += curr_len;
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 4c53bc6..80e6764 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -38,6 +38,7 @@
#include "osabi.h"
#include "ia64-tdep.h"
#include "cp-abi.h"
+#include "alloca-scope.h"

#ifdef HAVE_LIBUNWIND_IA64_H
#include "elf/ia64.h" /* for PT_IA_64_UNWIND value */
@@ -1516,7 +1517,6 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
else if (qp == 0 && rN == 2
&& ((rM == fp_reg && fp_reg != 0) || rM == 12))
{
- gdb_byte buf[MAX_REGISTER_SIZE];
CORE_ADDR saved_sp = 0;
/* adds r2, spilloffset, rFramePointer
or
@@ -1534,8 +1534,17 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
{
struct gdbarch *gdbarch = get_frame_arch (this_frame);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
- get_frame_register (this_frame, sp_regnum, buf);
- saved_sp = extract_unsigned_integer (buf, 8, byte_order);
+
+ ALLOCA_SCOPE_START
+ {
+ gdb_byte *buf
+ = (gdb_byte *) alloca (register_size (gdbarch,
+ sp_regnum));
+
+ get_frame_register (this_frame, sp_regnum, buf);
+ saved_sp = extract_unsigned_integer (buf, 8, byte_order);
+ }
+ ALLOCA_SCOPE_END
}
spill_addr = saved_sp
+ (rM == 12 ? 0 : mem_stack_frame_size)
--
2.5.5