From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51436 invoked by alias); 30 Nov 2016 19:30:32 -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 47196 invoked by uid 89); 30 Nov 2016 19:30:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=gnus, gathered X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 19:30:23 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cCAaE-0002ny-6l from Luis_Gustavo@mentor.com ; Wed, 30 Nov 2016 11:30:22 -0800 Received: from [172.30.5.15] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 30 Nov 2016 11:30:19 -0800 Subject: Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer References: <1480428758-2481-1-git-send-email-yao.qi@linaro.org> To: Yao Qi , From: Luis Machado Reply-To: Luis Machado Message-ID: <55ea5ba0-6ea7-686d-1841-48875ed8587c@codesourcery.com> Date: Wed, 30 Nov 2016 19:30: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"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg01013.txt.bz2 On 11/29/2016 08:12 AM, 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 > +{ There is a new line before the class declaration here, but not on some of the other declarations. > +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; > + > + 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 { }; > + > + 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 > + { No newline between class declaration and comment. > + public: > + instruction_reader_test() = default ; > + instruction_reader_test (std::initializer_list init) > + : insns{init} {} > + > + 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; > + }; Should the curly braces both be on their own lines as the rest of the uses? I see mixed formatting in the uses of namespace we currently have. We should pick one and go with it. I particularly dislike the curly brace on the same line being used to GNU's coding standards. > + > +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 */ > + }; Same here. > + > + CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, > + &cache, test); > + SELF_CHECK (end == 4 * 2); > + > + SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM); > + SELF_CHECK (cache.framesize == 272); > + > + for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++) > + { > + if (i == AARCH64_FP_REGNUM) > + SELF_CHECK (cache.saved_regs[i].addr == -272); > + else if (i == AARCH64_LR_REGNUM) > + SELF_CHECK (cache.saved_regs[i].addr == -264); > + else > + SELF_CHECK (cache.saved_regs[i].addr == -1); > + } > + > + for (int i = 0; i < AARCH64_D_REGISTER_COUNT; i++) > + { > + int regnum = gdbarch_num_regs (gdbarch); > + > + SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1); > + } > +} > +} > +#endif /* GDB_SELF_TEST */ > + > /* Implement the "skip_prologue" gdbarch method. */ > > static CORE_ADDR > @@ -2864,6 +2972,10 @@ When on, AArch64 specific debugging is enabled."), > NULL, > show_aarch64_debug, > &setdebuglist, &showdebuglist); > + > +#if GDB_SELF_TEST > + register_self_test (selftests::aarch64_analyze_prologue_test); > +#endif > } > > /* AArch64 process record-replay related structures, defines etc. */ > diff --git a/gdb/trad-frame.c b/gdb/trad-frame.c > index ebf19df..4430dd5 100644 > --- a/gdb/trad-frame.c > +++ b/gdb/trad-frame.c > @@ -43,16 +43,10 @@ trad_frame_cache_zalloc (struct frame_info *this_frame) > return this_trad_cache; > } > > -/* A traditional frame is unwound by analysing the function prologue > - and using the information gathered to track registers. For > - non-optimized frames, the technique is reliable (just need to check > - for all potential instruction sequences). */ > - > struct trad_frame_saved_reg * > -trad_frame_alloc_saved_regs (struct frame_info *this_frame) > +trad_frame_alloc_saved_regs (struct gdbarch *gdbarch) > { > int regnum; > - struct gdbarch *gdbarch = get_frame_arch (this_frame); > int numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch); > struct trad_frame_saved_reg *this_saved_regs > = FRAME_OBSTACK_CALLOC (numregs, struct trad_frame_saved_reg); > @@ -65,6 +59,19 @@ trad_frame_alloc_saved_regs (struct frame_info *this_frame) > return this_saved_regs; > } > > +/* A traditional frame is unwound by analysing the function prologue > + and using the information gathered to track registers. For > + non-optimized frames, the technique is reliable (just need to check > + for all potential instruction sequences). */ > + > +struct trad_frame_saved_reg * > +trad_frame_alloc_saved_regs (struct frame_info *this_frame) > +{ > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > + > + return trad_frame_alloc_saved_regs (gdbarch); > +} > + > enum { TF_REG_VALUE = -1, TF_REG_UNKNOWN = -2 }; > > int > diff --git a/gdb/trad-frame.h b/gdb/trad-frame.h > index b8aed16..d1c24b0 100644 > --- a/gdb/trad-frame.h > +++ b/gdb/trad-frame.h > @@ -104,6 +104,7 @@ int trad_frame_realreg_p (struct trad_frame_saved_reg this_saved_regs[], > > /* Return a freshly allocated (and initialized) trad_frame array. */ > struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct frame_info *); > +struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct gdbarch *); > > /* Given the trad_frame info, return the location of the specified > register. */ > Otherwise looks good to me.