This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Wed, 30 Nov 2016 18:29:12 +0000
- Subject: Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer
- Authentication-results: sourceware.org; auth=none
- References: <1480428758-2481-1-git-send-email-yao.qi@linaro.org>
On 11/29/2016 02:12 PM, Yao Qi wrote:
> We don't have an effective way to test prologue analyzer which is
> highly dependent on instruction patterns in prologue generated by
> compiler. GDB prologue analyzer may not handle the new sequences
> generated by new compiler, or may still handle some sequences that
> generated by very old compilers which are no longer used. The
> former is a functionality issue, while the latter is a maintenance
> issue.
>
> The input and output of prologue analyzer is quite clear, so it
> fits for unit test. The input is series of instructions, and the
> output are 1) where prologue end, 2) where registers are saved.
> In aarch64, they are represented in 'struct aarch64_prologue_cache'.
>
> This patch refactors aarch64_analyze_prologue so it can read
> instructions from either real target or test harness. In unit
> test aarch64_analyze_prologue_test, aarch64_analyze_prologue gets
> instructions we prepared in the test, as the input of prologue
> analyzer. Then, we checked various fields in
> 'struct aarch64_prologue_cache'.
>
> gdb:
>
> 2016-11-28 Yao Qi <yao.qi@linaro.org>
>
> * aarch64-tdep.c: Include "selftest.h".
> (abstract_instruction_reader): New class.
> (instruction_reader): New class.
> (aarch64_analyze_prologue): Add new parameter reader. Call
> reader.read instead of read_memory_unsigned_integer.
> [GDB_SELF_TEST] (instruction_reader_test): New class.
> (aarch64_analyze_prologue_test): New function.
> (_initialize_aarch64_tdep) [GDB_SELF_TEST]: Register
> selftests::aarch64_analyze_prologue_test.
> * trad-frame.c (trad_frame_cache_zalloc):
> (trad_frame_alloc_saved_regs): Add a new function.
> * trad-frame.h (trad_frame_alloc_saved_regs): Declare.
> ---
> gdb/aarch64-tdep.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> gdb/trad-frame.c | 21 ++++++----
> gdb/trad-frame.h | 1 +
> 3 files changed, 129 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 6b95d7c..b10002a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -44,6 +44,7 @@
> #include "infcall.h"
> #include "ax.h"
> #include "ax-gdb.h"
> +#include "selftest.h"
>
> #include "aarch64-tdep.h"
>
> @@ -195,6 +196,29 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
> fprintf_filtered (file, _("AArch64 debugging is %s.\n"), value);
> }
>
> +/* Abstract instruction reader. */
> +
> +class abstract_instruction_reader
> +{
> +public:
> + /* Read in one instruction. */
> + virtual ULONGEST read (CORE_ADDR memaddr, int len,
> + enum bfd_endian byte_order) = 0;
> +};
> +
> +/* Instruction reader from real target. */
> +
> +class instruction_reader : public abstract_instruction_reader
> +{
> + public:
> + instruction_reader () = default;
(As mentioned in the previous email, this just looks like
unnecessary redundancy to me; suggest just removing it. The compiler
generates it for you.)
> +
> + ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
> + {
> + return read_memory_unsigned_integer (memaddr, len, byte_order);
> + }
> +};
> +
> /* Analyze a prologue, looking for a recognizable stack frame
> and frame pointer. Scan until we encounter a store that could
> clobber the stack frame unexpectedly, or an unknown instruction. */
> @@ -202,7 +226,8 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
> static CORE_ADDR
> aarch64_analyze_prologue (struct gdbarch *gdbarch,
> CORE_ADDR start, CORE_ADDR limit,
> - struct aarch64_prologue_cache *cache)
> + struct aarch64_prologue_cache *cache,
> + abstract_instruction_reader& reader)
> {
> enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> int i;
> @@ -221,7 +246,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
> uint32_t insn;
> aarch64_inst inst;
>
> - insn = read_memory_unsigned_integer (start, 4, byte_order_for_code);
> + insn = reader.read (start, 4, byte_order_for_code);
>
> if (aarch64_decode_insn (insn, &inst, 1) != 0)
> break;
> @@ -436,6 +461,89 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
> return start;
> }
>
> +static CORE_ADDR
> +aarch64_analyze_prologue (struct gdbarch *gdbarch,
> + CORE_ADDR start, CORE_ADDR limit,
> + struct aarch64_prologue_cache *cache)
> +{
> + instruction_reader reader { };
It's more idiomatic to just do:
instruction_reader reader;
when you want default construction.
> +
> + return aarch64_analyze_prologue (gdbarch, start, limit, cache,
> + reader);
> +}
> +
> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> + /* Instruction reader from manually cooked instruction sequences. */
> + class instruction_reader_test : public abstract_instruction_reader
> + {
> + public:
> + instruction_reader_test() = default ;
> + instruction_reader_test (std::initializer_list<uint32_t> init)
> + : insns{init} {}
I think we should put a space before "{" -> "insns {init}". We put
it before "(", and before "{" in all other contexts.
> +
> + ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
> + {
> + SELF_CHECK (len == 4);
> + SELF_CHECK (memaddr % 4 == 0);
> + SELF_CHECK (memaddr / 4 < insns.size());
> +
> + return insns[memaddr / 4];
> + }
> +
> + private:
> + std::vector<uint32_t> insns;
Private data members should be prefixed with "m_".
I'll note that it always itches me a bit when we do
unnecessary copying. :-) In this case, you always
start from an array of instructions known at compile-time,
and copy it into the vector at run time. You could
instead create the instructions array as a separate const
array, and pass than to the reader's constructor
as parameter, which would store a pointer to the array,
instead of a deep copy.
> + };
> +
> +static void
> +aarch64_analyze_prologue_test (void)
> +{
> + struct gdbarch_info info;
> +
> + gdbarch_info_init (&info);
> + info.bfd_arch_info = bfd_scan_arch ("aarch64");
> +
> + struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> + SELF_CHECK (gdbarch != NULL);
> +
> + struct aarch64_prologue_cache cache;
> + cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> +
> + instruction_reader_test test {
> + 0xa9af7bfd, /* stp x29, x30, [sp,#-272]! */
> + 0x910003fd, /* mov x29, sp */
> + 0x97ffffe6, /* bl 0x400580 */
> + };
Indentation looks odd here. "0x..." should be two columns
to the right of instruction_reader_test, and "};" aligned
at the same level as "instruction_reader_test".
Thanks,
Pedro Alves