From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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:35:00 -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-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00999.txt.bz2 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. -- Yao (齐尧) >From 0929e7f3819388c262e7ac8da157464361bb7787 Mon Sep 17 00:00:00 2001 From: Yao Qi Date: Mon, 28 Nov 2016 10:40:59 +0000 Subject: [PATCH] Add unit test to aarch64 prologue analyzer 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. 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. */