From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126928 invoked by alias); 30 Nov 2016 18:29:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 126909 invoked by uid 89); 30 Nov 2016 18:29:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=447 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 18:29:15 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F0FFAC057FA6; Wed, 30 Nov 2016 18:29:13 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAUITCkP027114; Wed, 30 Nov 2016 13:29:13 -0500 Subject: Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer To: Yao Qi , gdb-patches@sourceware.org References: <1480428758-2481-1-git-send-email-yao.qi@linaro.org> From: Pedro Alves Message-ID: <249b2f2d-a678-07e8-bc75-df8128f3d8f5@redhat.com> Date: Wed, 30 Nov 2016 18:29:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1480428758-2481-1-git-send-email-yao.qi@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg01009.txt.bz2 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 > > * 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 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 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