From patchwork Wed Nov 30 16:34:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 18072 Received: (qmail 18429 invoked by alias); 30 Nov 2016 16:35:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 17352 invoked by uid 89); 30 Nov 2016 16:35:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=personal X-HELO: mail-wj0-f195.google.com Received: from mail-wj0-f195.google.com (HELO mail-wj0-f195.google.com) (209.85.210.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 16:35:02 +0000 Received: by mail-wj0-f195.google.com with SMTP id he10so8781774wjc.2 for ; Wed, 30 Nov 2016 08:35:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=Nad9eYxfsgSVlaYVSzVI3dQgxFxjiiQUhthcJe4T72M=; b=e2vEnlj5JVbxrKmla/57KreKI3jJKoGSBymRl9Adf5MO9YDlIkaesZrTaaW0Ce8aQ2 RDtGiHSEPagrgPNLZkM1K6m75YiAIG1AWyDvzk1NVnBcxTGep4P+WlPZtKg0BNU9654t 0mc9OSqKBaKtcfezEAJXkiySiRvErfVR8y/8NXzckcfApRegCdzmEHPhCVIHGChpKP8R uJy2loqwsJNovgrF+7TFBfTbDH7wvVD/sSpe6uC7hp15u+0qt2373AoyE6GhQzbzJazw e5egzi3cdU5AGV2wBsB6jMioqxxR4cc53sx3gjv7CSEdg33mfj3Yb/fUWdbTNN/eH745 kkvQ== X-Gm-Message-State: AKaTC00pyCjYG8V7PzewbXMjJRAz0+csQCamqPkh5bk9fPIOuHl7Ami8imdwgmKj2lfqbw== X-Received: by 10.194.146.131 with SMTP id tc3mr29767609wjb.129.1480523700538; Wed, 30 Nov 2016 08:35:00 -0800 (PST) Received: from E107787-LIN (power8-aix.osuosl.org. [140.211.9.96]) by smtp.gmail.com with ESMTPSA id w18sm8758693wme.9.2016.11.30.08.34.57 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 30 Nov 2016 08:35:00 -0800 (PST) Date: Wed, 30 Nov 2016 16:34:49 +0000 From: Yao Qi To: Antoine Tremblay Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer Message-ID: <20161130163449.GI22209@E107787-LIN> References: <1480428758-2481-1-git-send-email-yao.qi@linaro.org> <20161130111459.GG22209@E107787-LIN> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote: > >> Also I wonder if we need to specify the default constructor explicitly ? > >> Is there a rationale for it? > >> > >> It's never used too, unless you apply my previous comment. > > > > The instruction_reader_test default constructor is never used. How > > about using "= delete"? > > > > instruction_reader_test () = delete; > > instruction_reader_test (std::initializer_list init) > > : insns{init} {} > > Yes that would be more appropriate if we're going to specify that. > > I just wrote a patch with a C++ class and did not include explicit > default constructors do you think we should make it a code convention to > explicitly specify their existence or non-existence (=default, =delete) ? If you don't want default constructor to be used, "=delete" is useful, IMO, which tells compiler not to generate the default constructor. The intention is quite clear that I don't want you to use the default constructor. Using "=default" is not that clear. I personally prefer to write code in an explicit way, so I prefer putting "=default" at the end. > > I could not find mention of that in GCC's C++ conventions... IMO, using "=default" is a personal programming habit, so it is reasonable not to mention it in C++ code conventions. diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index 6b95d7c..3838393 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; + + 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 + { + public: + instruction_reader_test () = delete; + 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; + }; + +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 */ + }; + + 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. */