From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id mWImJZMjF2PTbjQAWB0awg (envelope-from ) for ; Tue, 06 Sep 2022 06:40:19 -0400 Received: by simark.ca (Postfix, from userid 112) id 8EC541E4A7; Tue, 6 Sep 2022 06:40:19 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=ENA+KWiJ; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C6CFF1E13B for ; Tue, 6 Sep 2022 06:40:18 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 056BE38515F4 for ; Tue, 6 Sep 2022 10:40:18 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 056BE38515F4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1662460818; bh=px8u+G4Bt1TaRhS4aPfm6+IMCt5UA7T74mhYPJdeSoc=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ENA+KWiJeCo2+AGzApJKqC6Hg8JBYn0vlvqcEzQU27etgWcee5Da80GNARZS/StAz /r18s1QPKtc4E1iIJjk/2RoJG/B1aQ+YMmPvwVnXoKlcSg+mgmSUovI1R3nsoUsKxb NJboGH3LTE/jIVdIKNEX0aaWqeMCjTgAGWfRu7Ew= Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id C729B38582AD for ; Tue, 6 Sep 2022 10:39:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C729B38582AD Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 0F428337A2; Tue, 6 Sep 2022 10:39:56 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id E407F13A7A; Tue, 6 Sep 2022 10:39:55 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id KaIwNnsjF2M/DwAAMHmgww (envelope-from ); Tue, 06 Sep 2022 10:39:55 +0000 Message-ID: <2f9230ad-b4f7-9f4b-c60c-7c6adfa9cb01@suse.de> Date: Tue, 6 Sep 2022 12:39:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PING][PATCH,v2] [Arm] Fix endianness handling for arm record self tests Content-Language: en-US To: Luis Machado , gdb-patches@sourceware.org References: <20220808101203.168954-1-luis.machado@arm.com> <20220823203210.1561735-1-luis.machado@arm.com> <14ce7cf2-82be-4b93-b875-d7cc7a323f2d@arm.com> In-Reply-To: <14ce7cf2-82be-4b93-b875-d7cc7a323f2d@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Tom de Vries via Gdb-patches Reply-To: Tom de Vries Cc: mark@klomp.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 9/1/22 11:29, Luis Machado wrote: > On 8/23/22 21:32, Luis Machado via Gdb-patches wrote: >> v2: >> >> - Add 32-bit Arm instruction selftest >> - Refactored abstract memory reader into abstract instruction reader >> - Adjusted code to use templated type and to use host endianness as >>    opposed to target endianness. >> >> The arm record tests handle 16-bit and 32-bit thumb instructions, but the >> code is laid out in a way that handles the 32-bit thumb instructions as >> two 16-bit parts. >> >> This is fine, but it is prone to host-endianness issues given how the two >> 16-bit parts are stored and how they are accessed later on. Arm is >> little-endian by default, so running this test with a GDB built with >> --enable-targets=all and on a big endian host will run into the >> following: >> >> Running selftest arm-record. >> Process record and replay target doesn't support syscall number -2036195 >> Process record does not support instruction 0x7f70ee1d at address 0x0. >> Self test failed: self-test failed at >> ../../binutils-gdb/gdb/arm-tdep.c:14482 >> >> It turns out the abstract memory reader class is more generic than it >> needs to >> be, and we can simplify the code a bit by assuming we have a simple >> instruction >> reader that only reads up to 4 bytes, which is the length of a 32-bit >> instruction. >> >> Instead of returning a bool, we return instead the instruction that >> has been >> read. This way we avoid having to deal with the endianness conversion, >> and use >> the host endianness instead. The Arm selftests can be executed on non-Arm >> hosts. >> LGTM. Thanks, - Tom >> While at it, Tom suggested adding a 32-bit Arm instruction selftest to >> increase >> the coverage of the selftests. >> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29432 >> >> Co-authored-by: Tom de Vries >> --- >>   gdb/arm-tdep.c | 150 ++++++++++++++++++++++--------------------------- >>   1 file changed, 68 insertions(+), 82 deletions(-) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index 0091d14708f..ead9bbf46c5 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -14252,59 +14252,39 @@ thumb2_record_decode_insn_handler >> (arm_insn_decode_record *thumb2_insn_r) >>   } >>   namespace { >> -/* Abstract memory reader.  */ >> +/* Abstract instruction reader.  */ >> -class abstract_memory_reader >> +class abstract_instruction_reader >>   { >>   public: >> -  /* Read LEN bytes of target memory at address MEMADDR, placing the >> -     results in GDB's memory at BUF.  Return true on success.  */ >> +  /* Read one instruction of size LEN from address MEMADDR and using >> +     BYTE_ORDER endianness.  */ >> -  virtual bool read (CORE_ADDR memaddr, gdb_byte *buf, const size_t >> len) = 0; >> +  virtual ULONGEST read (CORE_ADDR memaddr, const size_t len, >> +             enum bfd_endian byte_order) = 0; >>   }; >>   /* Instruction reader from real target.  */ >> -class instruction_reader : public abstract_memory_reader >> +class instruction_reader : public abstract_instruction_reader >>   { >>    public: >> -  bool read (CORE_ADDR memaddr, gdb_byte *buf, const size_t len) >> override >> +  ULONGEST read (CORE_ADDR memaddr, const size_t len, >> +         enum bfd_endian byte_order) override >>     { >> -    if (target_read_memory (memaddr, buf, len)) >> -      return false; >> -    else >> -      return true; >> +    return read_code_unsigned_integer (memaddr, len, byte_order); >>     } >>   }; >>   } // namespace >> -/* Extracts arm/thumb/thumb2 insn depending on the size, and returns >> 0 on success >> -and positive val on failure.  */ >> - >> -static int >> -extract_arm_insn (abstract_memory_reader& reader, >> -          arm_insn_decode_record *insn_record, uint32_t insn_size) >> -{ >> -  gdb_byte buf[insn_size]; >> - >> -  memset (&buf[0], 0, insn_size); >> - >> -  if (!reader.read (insn_record->this_addr, buf, insn_size)) >> -    return 1; >> -  insn_record->arm_insn = (uint32_t) extract_unsigned_integer (&buf[0], >> -               insn_size, >> -               gdbarch_byte_order_for_code (insn_record->gdbarch)); >> -  return 0; >> -} >> - >>   typedef int (*sti_arm_hdl_fp_t) (arm_insn_decode_record*); >>   /* Decode arm/thumb insn depending on condition cods and opcodes; and >>      dispatch it.  */ >>   static int >> -decode_insn (abstract_memory_reader &reader, >> +decode_insn (abstract_instruction_reader &reader, >>            arm_insn_decode_record *arm_record, >>            record_type_t record_type, uint32_t insn_size) >>   { >> @@ -14339,20 +14319,12 @@ decode_insn (abstract_memory_reader &reader, >>     uint32_t ret = 0;    /* return value: negative:failure >> 0:success.  */ >>     uint32_t insn_id = 0; >> +  enum bfd_endian code_endian >> +    = gdbarch_byte_order_for_code (arm_record->gdbarch); >> +  arm_record->arm_insn >> +    = reader.read (arm_record->this_addr, insn_size, code_endian); >> -  if (extract_arm_insn (reader, arm_record, insn_size)) >> -    { >> -      if (record_debug) >> -    { >> -      gdb_printf (gdb_stdlog, >> -              _("Process record: error reading memory at " >> -            "addr %s len = %d.\n"), >> -              paddress (arm_record->gdbarch, >> -                arm_record->this_addr), insn_size); >> -    } >> -      return -1; >> -    } >> -  else if (ARM_RECORD == record_type) >> +  if (ARM_RECORD == record_type) >>       { >>         arm_record->cond = bits (arm_record->arm_insn, 28, 31); >>         insn_id = bits (arm_record->arm_insn, 25, 27); >> @@ -14412,36 +14384,35 @@ decode_insn (abstract_memory_reader &reader, >>   #if GDB_SELF_TEST >>   namespace selftests { >> -/* Provide both 16-bit and 32-bit thumb instructions.  */ >> +/* Instruction reader class for selftests. >> + >> +   For 16-bit Thumb instructions, an array of uint16_t should be used. >> -class instruction_reader_thumb : public abstract_memory_reader >> +   For 32-bit Thumb instructions and regular 32-bit Arm instructions, >> an array >> +   of uint32_t should be used.  */ >> + >> +template >> +class instruction_reader_selftest : public abstract_instruction_reader >>   { >>   public: >>     template >> -  instruction_reader_thumb (enum bfd_endian endian, >> -                const uint16_t (&insns)[SIZE]) >> -    : m_endian (endian), m_insns (insns), m_insns_size (SIZE) >> +  instruction_reader_selftest (const T (&insns)[SIZE]) >> +    : m_insns (insns), m_insns_size (SIZE) >>     {} >> -  bool read (CORE_ADDR memaddr, gdb_byte *buf, const size_t len) >> override >> +  ULONGEST read (CORE_ADDR memaddr, const size_t length, >> +         enum bfd_endian byte_order) override >>     { >> -    SELF_CHECK (len == 4 || len == 2); >> -    SELF_CHECK (memaddr % 2 == 0); >> -    SELF_CHECK ((memaddr / 2) < m_insns_size); >> +    SELF_CHECK (length == sizeof (T)); >> +    SELF_CHECK (memaddr % sizeof (T) == 0); >> +    SELF_CHECK ((memaddr / sizeof (T)) < m_insns_size); >> -    store_unsigned_integer (buf, 2, m_endian, m_insns[memaddr / 2]); >> -    if (len == 4) >> -      { >> -    store_unsigned_integer (&buf[2], 2, m_endian, >> -                m_insns[memaddr / 2 + 1]); >> -      } >> -    return true; >> +    return m_insns[memaddr / sizeof (T)]; >>     } >>   private: >> -  enum bfd_endian m_endian; >> -  const uint16_t *m_insns; >> -  size_t m_insns_size; >> +  const T *m_insns; >> +  const size_t m_insns_size; >>   }; >>   static void >> @@ -14461,6 +14432,8 @@ arm_record_test (void) >>       memset (&arm_record, 0, sizeof (arm_insn_decode_record)); >>       arm_record.gdbarch = gdbarch; >> +    /* Use the endian-free representation of the instructions here. >> The test >> +       will handle endianness conversions.  */ >>       static const uint16_t insns[] = { >>         /* db b2    uxtb    r3, r3 */ >>         0xb2db, >> @@ -14468,8 +14441,7 @@ arm_record_test (void) >>         0x58cd, >>       }; >> -    enum bfd_endian endian = gdbarch_byte_order_for_code >> (arm_record.gdbarch); >> -    instruction_reader_thumb reader (endian, insns); >> +    instruction_reader_selftest reader (insns); >>       int ret = decode_insn (reader, &arm_record, THUMB_RECORD, >>                  THUMB_INSN_SIZE_BYTES); >> @@ -14495,13 +14467,14 @@ arm_record_test (void) >>       memset (&arm_record, 0, sizeof (arm_insn_decode_record)); >>       arm_record.gdbarch = gdbarch; >> -    static const uint16_t insns[] = { >> -      /* 1d ee 70 7f     mrc    15, 0, r7, cr13, cr0, {3} */ >> -      0xee1d, 0x7f70, >> +    /* Use the endian-free representation of the instruction here. >> The test >> +       will handle endianness conversions.  */ >> +    static const uint32_t insns[] = { >> +      /* mrc    15, 0, r7, cr13, cr0, {3} */ >> +      0x7f70ee1d, >>       }; >> -    enum bfd_endian endian = gdbarch_byte_order_for_code >> (arm_record.gdbarch); >> -    instruction_reader_thumb reader (endian, insns); >> +    instruction_reader_selftest reader (insns); >>       int ret = decode_insn (reader, &arm_record, THUMB2_RECORD, >>                  THUMB2_INSN_SIZE_BYTES); >> @@ -14510,6 +14483,27 @@ arm_record_test (void) >>       SELF_CHECK (arm_record.reg_rec_count == 1); >>       SELF_CHECK (arm_record.arm_regs[0] == 7); >>     } >> + >> +  /* 32-bit instructions.  */ >> +  { >> +    arm_insn_decode_record arm_record; >> + >> +    memset (&arm_record, 0, sizeof (arm_insn_decode_record)); >> +    arm_record.gdbarch = gdbarch; >> + >> +    /* Use the endian-free representation of the instruction here. >> The test >> +       will handle endianness conversions.  */ >> +    static const uint32_t insns[] = { >> +      /* mov     r5, r0 */ >> +      0xe1a05000, >> +    }; >> + >> +    instruction_reader_selftest reader (insns); >> +    int ret = decode_insn (reader, &arm_record, ARM_RECORD, >> +               ARM_INSN_SIZE_BYTES); >> + >> +    SELF_CHECK (ret == 0); >> +  } >>   } >>   /* Instruction reader from manually cooked instruction sequences.  */ >> @@ -14609,18 +14603,10 @@ arm_process_record (struct gdbarch *gdbarch, >> struct regcache *regcache, >>       } >>     instruction_reader reader; >> -  if (extract_arm_insn (reader, &arm_record, 2)) >> -    { >> -      if (record_debug) >> -    { >> -      gdb_printf (gdb_stdlog, >> -              _("Process record: error reading memory at " >> -            "addr %s len = %d.\n"), >> -              paddress (arm_record.gdbarch, >> -                arm_record.this_addr), 2); >> -    } >> -      return -1; >> -    } >> +  enum bfd_endian code_endian >> +    = gdbarch_byte_order_for_code (arm_record.gdbarch); >> +  arm_record.arm_insn >> +    = reader.read (arm_record.this_addr, 2, code_endian); >>     /* Check the insn, whether it is thumb or arm one.  */ >