From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121430 invoked by alias); 20 Jan 2017 00:04:03 -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 121202 invoked by uid 89); 20 Jan 2017 00:03:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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; Fri, 20 Jan 2017 00:03:51 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 8388015447; Fri, 20 Jan 2017 00:03:51 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0K03nDi031241; Thu, 19 Jan 2017 19:03:49 -0500 Subject: Re: [PATCH 4/6] Disassembly unit test: disassemble one instruction To: Yao Qi , gdb-patches@sourceware.org References: <1484051178-16013-1-git-send-email-yao.qi@linaro.org> <1484560977-8693-1-git-send-email-yao.qi@linaro.org> <1484560977-8693-5-git-send-email-yao.qi@linaro.org> From: Pedro Alves Message-ID: <620db0ef-5e3d-3b93-5596-33d24a78f6a9@redhat.com> Date: Fri, 20 Jan 2017 00:04: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: <1484560977-8693-5-git-send-email-yao.qi@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00399.txt.bz2 On 01/16/2017 10:02 AM, Yao Qi wrote: > +static void > +gdb_disassembler_print_one_insn_test (struct gdbarch *gdbarch) > +{ > + size_t len = 0; > + const gdb_byte *insn = NULL; > + > + insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, (int *) &len); That "(int *) &len" is invalid code. It's an aliasing violation. And even if that weren't a problem, consider what happens when sizeof size_t != sizeof int, on big endian and little endian. Use a temporary variable of the right type, like e.g.: int bplen; insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &bplen); len = bplen; > + break; > + default: > + { > + /* Test disassemble breakpoint instruction. */ > + CORE_ADDR pc = 0; > + int kind = gdbarch_breakpoint_kind_from_pc (gdbarch, &pc); > + > + insn = gdbarch_sw_breakpoint_from_kind (gdbarch, kind, > + (int *) &len); Ditto. > + len = sizeof (xstormy16_insn); > + break; > + case bfd_arch_arc: > + { > + /* PR 21003 */ > + if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601) > + return; > + } Odd that this case got braces when it doesn't declare any variable, and when other cases don't. Also, is the fallthrough intended? If so, add a comment otherwise we may get a warning with GCC 7. > + case bfd_arch_nios2: > + case bfd_arch_score: > + /* nios2 and score need to know the current instruction to select > + breakpoint instruction. Give the breakpoint instruction kind > + explicitly. */ > + insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, (int *) &len); > + break; > + default: > + > + break; > + } > + } > + SELF_CHECK (len > 0); > + > + /* Test gdb_disassembler for a given gdbarch by reading data from a > + pre-allocated buffer. If you want to see the disassembled > + instruction printed to gdb_stdout, set DISASSEMBLER_TEST_VERBOSE > + to true. */ > + > + class gdb_disassembler_test : public gdb_disassembler > + { > + public: > + > + const bool DISASSEMBLER_TEST_VERBOSE = false; static. We give macros long unique names in order to avoid naming conflicts, but if this is no longer a macro, the name could be shortened, to e.g., just: static const bool verbose = false; > + > + explicit gdb_disassembler_test (struct gdbarch *gdbarch, > + const gdb_byte *insn, > + size_t len) > + : gdb_disassembler (gdbarch, > + (DISASSEMBLER_TEST_VERBOSE > + ? gdb_stdout : null_stream ()), > + gdb_disassembler_test::read_memory), > + m_insn (insn), m_len (len) > + { > + } > + > + int > + print_insn (CORE_ADDR memaddr) > + { > + if (DISASSEMBLER_TEST_VERBOSE) > + { > + fprintf_unfiltered (stream (), "%s ", > + gdbarch_bfd_arch_info (arch ())->arch_name); > + } > + > + int len = gdb_disassembler::print_insn (memaddr); > + > + if (DISASSEMBLER_TEST_VERBOSE) > + fprintf_unfiltered (stream (), "\n"); > + > + return len; > + } > + > + private: > + /* A buffer contain one instruction. */ > + const gdb_byte *m_insn; > + > + /* Length of the buffer. */ > + size_t m_len; > + > + static int read_memory (bfd_vma memaddr, gdb_byte *myaddr, > + unsigned int len, struct disassemble_info *info) > + { > + gdb_disassembler_test *self > + = static_cast(info->application_data); > + > + /* The disassembler in opcodes may read more data than one > + instruction. */ I suggest: /* The opcodes disassembler may read more data than one instruction. Supply infinite consecutive copies of the same instruction. > + for (unsigned int i = 0; i < len; i++) size_t. > + myaddr[i] = self->m_insn[(memaddr + i) % self->m_len]; Clever. :-) > + > + return 0; > + } > + }; > + > + gdb_disassembler_test di (gdbarch, insn, len); > + > + SELF_CHECK (di.print_insn (0) == len); > +} > + > +} // namespace selftests > +#endif /* GDB_SELF_TEST */ > + > +/* Suppress warning from -Wmissing-prototypes. */ > +extern initialize_file_ftype _initialize_disasm_test; > + > +void > +_initialize_disasm_test (void) The standard is to name the _initialize_foo function after the file/module name: _initialize_disasm_selftests > + > +static void > +tests_with_arch (void) We longer need the "void" in C++. > +{ > + int failed = 0; > + > + for (const auto &f : gdbarch_tests) > + { > + const char **arches = gdbarch_printable_names (); > + int i; > + > + for (i = 0; arches[i] != NULL; i++) Can be "for (int i ..." now. > +/* Suppress warning from -Wmissing-prototypes. */ > +extern initialize_file_ftype _initialize_selftests_with_arch; > + > +void > +_initialize_selftests_with_arch (void) Likewise (naming / void). > +#ifndef SELFTEST_ARCH_H > +#define SELFTEST_ARCH_H > + > +typedef void self_test_function_with_gdbarch (struct gdbarch *); > + > +extern void register_self_test (self_test_function_with_gdbarch *function); IMO, overloading the "register_self_test" function is confusing. This function and the register_self_test() function in selftest.c are semantically different, not two ways to do the same thing (like e.g. const char * vs std::string). If nothing else, it makes it a bit harder to grep for / find arch self tests (only) in the future. I'd prefer calling this something else that indicates more clearly what the selftest being registered is about. That's why I had suggested before the distinct: register_arch_self_test Perhaps better would be: register_self_test_foreach_arch And then self_test_function_with_gdbarch -> self_test_foreach_arch_function. WDYT? Thanks, Pedro Alves