From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113112 invoked by alias); 14 Apr 2016 12:40:27 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 113102 invoked by uid 89); 14 Apr 2016 12:40:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=4747, relationship X-HELO: mail-wm0-f48.google.com Received: from mail-wm0-f48.google.com (HELO mail-wm0-f48.google.com) (74.125.82.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 14 Apr 2016 12:40:16 +0000 Received: by mail-wm0-f48.google.com with SMTP id v188so218305724wme.1 for ; Thu, 14 Apr 2016 05:40:16 -0700 (PDT) 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:in-reply-to:user-agent; bh=HrK7zTmJobP7fsXj9yA9nIXRm5/bOIJDnaIvaqOpeWA=; b=GOtLpKov4wWCoGrELTBJ7Q6KKbeyR8zsO0ECgnFS1QC1ZwUoTRjwIJPLXlGcWY4q3q XTgeJaEaZbcUlSgXckDoFrHYhZdNkn/2uRAL7glPaLh34aaAlJCLHW7HY9iJjDvNKVzc A5tOwX1rRzXjOPYcf39LEpC3yxr3vFHbil+9ZQWuyjd5KWeTCWGiQbYPukDUnXmeZfau lre35jNz0MHF+l1JuJ0A5tv6PKTLjkU6o47zQ+3NdF1y4QMmf2T4bLPbjaAmm8eorMOo 3DhEUU+Jlup/J8IQY8s4U0jZvbeDOK53+sH1mZ68mPz5rLP3CYkFKB+LzaEbBojFYY71 Ws0Q== X-Gm-Message-State: AOPr4FWnvH/qXPPbH/KvbCCQQBOl9lO95uHt9jfaIp1cUFKZNOkxynkxUjNz0Q+2omXY0g== X-Received: by 10.194.95.167 with SMTP id dl7mr15906419wjb.163.1460637613615; Thu, 14 Apr 2016 05:40:13 -0700 (PDT) Received: from localhost (host81-148-252-106.range81-148.btcentralplus.com. [81.148.252.106]) by smtp.gmail.com with ESMTPSA id w186sm6367745wmd.20.2016.04.14.05.40.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Apr 2016 05:40:12 -0700 (PDT) Date: Thu, 14 Apr 2016 12:40:00 -0000 From: Andrew Burgess To: Nick Clifton Cc: binutils@sourceware.org, Claudiu.Zissulescu@synopsys.com, Cupertino.Miranda@synopsys.com, noamca@mellanox.com Subject: Re: [PATCHv2 1/3] opcodes/arc: Move instruction length logic to new function Message-ID: <20160414124011.GH5531@embecosm.com> References: <570E4A76.5020504@redhat.com> <20160413144058.GB5531@embecosm.com> <570E661E.80908@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <570E661E.80908@redhat.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg00211.txt.bz2 * Nick Clifton [2016-04-13 16:30:38 +0100]: > Hi Andrew, > > >> Would this function ever return a negative value ? I assume not, so > >> it would make sense for its return type to be "unsigned int". > > > > I'm not sure is the answer. I agree that arc_insn_length will never > > return a negative, however, the return value from arc_insn_length is > > used to prime a variable that is then the return value for > > print_insn_arc, which is also defined to return 'int', and is part of > > the disassembler API, and does return a negative value if there's an > > error. > > > > It was this relationship that originally lead me to make > > arc_insn_length return an 'int'. > > > > Given that it will only ever return small positive integers there > > should be no problem making it return an unsigned value then casting > > to int in print_insn_arc - would this be preferred? > > Yes. Or (better IMHO) just explicitly set the return value for > print_insn_arc based upon testing the return value from arc_insn_length. > Ie something like: > > len = arc_insn_length (...); > if (len == 0) > return -1; > > (I am assuming here that a returned length of zero should never happen > with a valid instruction, and can therefore be used as an error return > value). Just so I know I've done what you imagined, here's the patch that I think addresses your point. Is this OK? It's mostly the same except for type changes associated with insnLen. Thanks, Andrew --- Move the logic that calculates the instruction length out to a new function. Restructure the code to make it simpler. opcodes/ChangeLog: * arc-dis.c (arc_insn_length): New function. (print_insn_arc): Use arc_insn_length, change insnLen to unsigned. (find_format): Change insnLen parameter to unsigned. --- opcodes/ChangeLog | 6 ++++++ opcodes/arc-dis.c | 57 ++++++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c index eb8bd67..7757ef6 100644 --- a/opcodes/arc-dis.c +++ b/opcodes/arc-dis.c @@ -105,7 +105,7 @@ special_flag_p (const char *opname, /* Find proper format for the given opcode. */ static const struct arc_opcode * find_format (const struct arc_opcode *arc_table, - unsigned *insn, int insnLen, + unsigned *insn, unsigned int insnLen, unsigned isa_mask) { unsigned int i = 0; @@ -309,6 +309,37 @@ get_auxreg (const struct arc_opcode *opcode, } return NULL; } + +/* Calculate the instruction length for an instruction starting with MSB + and LSB, the most and least significant byte. The ISA_MASK is used to + filter the instructions considered to only those that are part of the + current architecture. + + The instruction lengths are calculated from the ARC_OPCODE table, and + cached for later use. */ + +static unsigned int +arc_insn_length (bfd_byte msb, struct disassemble_info *info) +{ + bfd_byte major_opcode = msb >> 3; + + switch (info->mach) + { + case bfd_mach_arc_nps400: + case bfd_mach_arc_arc700: + case bfd_mach_arc_arc600: + return (major_opcode > 0xb) ? 2 : 4; + break; + + case bfd_mach_arc_arcv2: + return (major_opcode > 0x7) ? 2 : 4; + break; + + default: + abort (); + } +} + /* Disassemble ARC instructions. */ static int @@ -318,7 +349,7 @@ print_insn_arc (bfd_vma memaddr, bfd_byte buffer[4]; unsigned int lowbyte, highbyte; int status; - int insnLen = 0; + unsigned int insnLen; unsigned insn[2] = { 0, 0 }; unsigned isa_mask; const unsigned char *opidx; @@ -422,20 +453,19 @@ print_insn_arc (bfd_vma memaddr, return size; } - if ((((buffer[lowbyte] & 0xf8) > 0x38) - && ((buffer[lowbyte] & 0xf8) != 0x48)) - || ((info->mach == bfd_mach_arc_arcv2) - && ((buffer[lowbyte] & 0xF8) == 0x48)) /* FIXME! ugly. */ - ) + insnLen = arc_insn_length (buffer[lowbyte], info); + switch (insnLen) { - /* This is a short instruction. */ - insnLen = 2; + case 2: insn[0] = (buffer[lowbyte] << 8) | buffer[highbyte]; - } - else - { - insnLen = 4; + break; + default: + /* An unknown instruction is treated as being length 4. This is + possibly not the best solution, but matches the behaviour that was + in place before the table based instruction length look-up was + introduced. */ + case 4: /* This is a long instruction: Read the remaning 2 bytes. */ status = (*info->read_memory_func) (memaddr + 2, &buffer[2], 2, info); if (status != 0) @@ -444,6 +474,7 @@ print_insn_arc (bfd_vma memaddr, return -1; } insn[0] = ARRANGE_ENDIAN (info, buffer); + break; } /* Set some defaults for the insn info. */ -- 2.5.1