From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35971 invoked by alias); 19 Dec 2016 21:50:34 -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 35878 invoked by uid 89); 19 Dec 2016 21:50:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=Abstract, core_addr, CORE_ADDR X-HELO: mail-wj0-f196.google.com Received: from mail-wj0-f196.google.com (HELO mail-wj0-f196.google.com) (209.85.210.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Dec 2016 21:50:31 +0000 Received: by mail-wj0-f196.google.com with SMTP id kp2so25324807wjc.0 for ; Mon, 19 Dec 2016 13:50:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=F8yrBGzmCGL1GX/baA4IdYjv+m/wE0DRrD2K52JM90w=; b=ohcP8M5fNfvRDZI+lRx0sfrqkhy3LLqKOGEJNZLCWwICZNdsqk6FZQg0EtFem5V4/j ShQ3/h5uptuHhsUgRyfC3gwVEfobH6OXpj8pyrmOIUevQnygfTYkmUvd7SuDMi5ry3rS huo/Hv5nJaaFlhiPkbPB35NEoMepH3PqpmdhgULw6ZBOj2SUjX5/kjLVNnG6G8rXGjxP SmubgiEAaTiAijoYS+CH+2oqi6X0U0Gp1PHlSRQ4OJPbJkRekslm9MZ8X73JpLQZHutC 4/XPiCr2KCuXf0/leTF9fkGwczWWy/qH1vFGLGz7Dbyzan5SKOHbYTGWlI9TuqLiexve J8xw== X-Gm-Message-State: AIkVDXL/He0cpv3jwRZR+FeZxa2MvxxW8CJ9M/CGE2UBAp/X1laWLQC8kSfneyWwUElEjw== X-Received: by 10.194.26.133 with SMTP id l5mr15782798wjg.4.1482184229575; Mon, 19 Dec 2016 13:50:29 -0800 (PST) Received: from localhost ([2a02:c7d:8e80:c00:8f1:2555:134d:ef1a]) by smtp.gmail.com with ESMTPSA id w8sm18966940wmw.4.2016.12.19.13.50.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 19 Dec 2016 13:50:29 -0800 (PST) Date: Mon, 19 Dec 2016 21:50:00 -0000 From: Yao Qi To: Bernhard Heckel Cc: gdb-patches@sourceware.org Subject: Re: [PATCH V2 2/2] Prologue: Add selftests to x64/x32 architecture. Message-ID: <20161219215024.xttdy4m3ipaqh6co@localhost> References: <1481896716-1233-1-git-send-email-bernhard.heckel@intel.com> <1481896716-1233-3-git-send-email-bernhard.heckel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1481896716-1233-3-git-send-email-bernhard.heckel@intel.com> User-Agent: NeoMutt/20161104 (1.7.1) X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00345.txt.bz2 On 16-12-16 14:58:36, Bernhard Heckel wrote: > +/* Abstract code reader. */ > + > +class abstract_code_reader > +{ > +protected: > + bool target_memory = 0; We don't need this field, see comments below, > +public: > + /* Read in one byte. */ /* Read LEN bytes from MEMADDR to BUFFER. */ > + virtual void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len) = 0; > + > + bool is_target_memory (void) { return target_memory;}; > +}; > + > +/* Code reader from real target. */ > + > +class code_reader : public abstract_code_reader > +{ > +public: > + code_reader (void) > + { > + target_memory = 1; > + } > + > + void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len) > + { > + read_code (memaddr, buffer, len); > + } > +}; > + > /* Do a limited analysis of the prologue at PC and update CACHE > accordingly. Bail out early if CURRENT_PC is reached. Return the > address where the analysis stopped. > @@ -2282,7 +2312,8 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc, > static CORE_ADDR > amd64_analyze_prologue (struct gdbarch *gdbarch, > CORE_ADDR pc, CORE_ADDR current_pc, > - struct amd64_frame_cache *cache) > + struct amd64_frame_cache *cache, > + abstract_code_reader& reader) > { > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > /* There are two variations of movq %rsp, %rbp. */ > @@ -2304,12 +2335,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch, > if (current_pc <= pc) > return current_pc; > > - if (gdbarch_ptr_bit (gdbarch) == 32) > - pc = amd64_x32_analyze_stack_align (pc, current_pc, cache); > - else > - pc = amd64_analyze_stack_align (pc, current_pc, cache); > - > - op = read_code_unsigned_integer (pc, 1, byte_order); > + /* For selftests, we don't analyze stack alignment. */ Why don't you pass reader to amd64{x32,}_x32_analyze_stack_align and analyze the stack alignment as well?... > + if (reader.is_target_memory ()) > + { > + if (gdbarch_ptr_bit (gdbarch) == 32) > + pc = amd64_x32_analyze_stack_align (pc, current_pc, cache); > + else > + pc = amd64_analyze_stack_align (pc, current_pc, cache); > + } In this way, we don't need such change. > +#if GDB_SELF_TEST > + > +namespace selftests { > + > +/* Code reader from manually created instruction sequences. */ > + > +class code_reader_test : public abstract_code_reader > +{ > +private: > + const std::vector memory; std::vector is a little heavy, and there are a lot of copies in the tests below. Why don't you follow class instruction_reader_test in aarch64-tdep.c? You can have a field gdb_byte *m_data in class. > + > +public: > + explicit code_reader_test (const std::vector &memory) > + : memory (memory) > + { > + /* Nothing to do. */ > + } > + > + void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len) > + { > + SELF_CHECK ((memaddr + len) <= memory.size ()); > + memcpy (buffer, memory.data () + memaddr, len); > + } > +}; > + > +struct prologue_test_t { > + amd64_frame_cache exp_cache; Why don't you test other fields in amd64_frame_cache? > + CORE_ADDR exp_pc; > + std::vector memory; > +}; > + > +/* Returns x64 test pattern and expected results. */ > + > +static std::vector get_prologue_tests_x64 (void) > +{ > + std::vector prologue_tests; > + struct prologue_test_t prologue_test; > + > + /* Negative test. */ > + prologue_test.exp_pc = 0; > + prologue_test.memory = {0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, > + 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */ > + amd64_init_frame_cache (&prologue_test.exp_cache); > + prologue_test.exp_cache.frameless_p = 1; > + prologue_tests.push_back (prologue_test); > + > + prologue_test.exp_pc = 4; > + prologue_test.memory = {0x55, /* push %rbp */ > + 0x48, 0x89, 0xe5, /* mov %rsp, %rbp */ > + 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */ I don't think compiler can generate prologue with some many 'nop'. I hope the test case is the real code sequences generated by compiler. prologue_test.memory should have all prologue instructions plus one non-prologue instruction. Test can check the analysis stops at the last instruction (non-prologue one). > + amd64_init_frame_cache (&prologue_test.exp_cache); > + prologue_test.exp_cache.frameless_p = 0; > + prologue_tests.push_back (prologue_test); > + You can use .emplace_back to avoid copying. > + > +/* Returns test pattern and expected results. */ > + > +static std::vector get_prologue_tests (const std::string arch) > +{ > + SELF_CHECK (arch == "i386:x86-64" > + || arch == "i386:x64-32"); > + > + if (arch == "i386:x86-64") > + return get_prologue_tests_x64 (); > + else > + return get_prologue_tests_x32 (); > +} > + > +static void > +amd64_analyze_prologue_test (void) > +{ > + const std::string arch_string[] = {"i386:x86-64", "i386:x64-32"}; > + Alternatively, you can have two test functions, amd64_analyze_prologue_test, and amd64_x32_analyze_prologue_test. -- Yao