From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98568 invoked by alias); 11 Feb 2016 11:52:39 -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 98551 invoked by uid 89); 11 Feb 2016 11:52:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-pf0-f195.google.com Received: from mail-pf0-f195.google.com (HELO mail-pf0-f195.google.com) (209.85.192.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 11 Feb 2016 11:52:37 +0000 Received: by mail-pf0-f195.google.com with SMTP id c10so2466853pfc.0 for ; Thu, 11 Feb 2016 03:52:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-type :content-transfer-encoding; bh=rpMYc8h/UoM/iCId40oHBVwE6t/Jeo+A4zD4MBznz48=; b=OvfSjXrq3S2Se5jM/gJ/UZHxkcwO66cZUCxnuvZgcFZ1IyRycK5FLHGdkKjaykLC7S 7MvuAXRk5dyJYA9Ei0gqkMWUUiu0XktDWeS9mYz5Ut2/6EXSTZe1pAmD91WBCow/w8VI CuzLXurJAste/jXkn6DCLUXWjXZRmRFoA3nCmmsuBQsBece4lEdpm6pjEX7wNZB7LixP mgLACDCZHWvCBkAA+8aQz/Qb2ohIrk4BrQqMyosOULuODQ+7oYmHtzc6XIo9lyZcH+bF y9hrFbWuAZMTW4Y8dNswFLcIxjqsz9Dt+JpwpEqI+9rtLN+B+mt8Y/GiK4ITnusaHqXK JOFg== X-Gm-Message-State: AG10YORP7lhwDsXFVqKj8WzfJ5p4ElBsESZfu5VcTK0i2VYqSt/63qiEWqg8atMU97bhDw== X-Received: by 10.98.75.196 with SMTP id d65mr15270267pfj.96.1455191555935; Thu, 11 Feb 2016 03:52:35 -0800 (PST) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id 3sm2154253pfn.59.2016.02.11.03.52.33 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 11 Feb 2016 03:52:35 -0800 (PST) From: Yao Qi To: Simon Marchi Cc: Subject: Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc References: <1455121027-27061-1-git-send-email-simon.marchi@ericsson.com> <1455121027-27061-3-git-send-email-simon.marchi@ericsson.com> Date: Thu, 11 Feb 2016 11:52:00 -0000 In-Reply-To: <1455121027-27061-3-git-send-email-simon.marchi@ericsson.com> (Simon Marchi's message of "Wed, 10 Feb 2016 11:17:06 -0500") Message-ID: <86h9hfpipt.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00341.txt.bz2 Simon Marchi writes: > Refactor arm_decode_dp_misc to make it more readable. The new layout > matches very closely the description in the ARM Architecture Reference > Manual. It uses the same order and same nomenclature. As I mentioned in the reply to the patch cover letter, the manual may adjust the layout in the future. For example, the manual lists instructions whose OP is 0 first, but it may change to list instructions whose OP is 1 first in the future. IMO, we don't have to 100% match the code to the manual. > > gdb/ChangeLog: > > * arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding. > --- > gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++-------------= ------ > 1 file changed, 49 insertions(+), 24 deletions(-) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 0a9c0f6..e17a1a4 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint= 32_t insn, > struct regcache *regs, > struct displaced_step_closure *dsc) > { > - if (bit (insn, 25)) > - switch (bits (insn, 20, 24)) > - { > - case 0x10: > - return arm_copy_unmodified (gdbarch, insn, "movw", dsc); > - > - case 0x14: > - return arm_copy_unmodified (gdbarch, insn, "movt", dsc); > + uint8_t op =3D bit (insn, 25); > + uint8_t op1 =3D bits (insn, 20, 24); > + uint8_t op2 =3D bits (insn, 4, 7); >=20=20 > - case 0x12: case 0x16: > - return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); > - > - default: > - return arm_copy_alu_imm (gdbarch, insn, regs, dsc); > - } > - else > + if (op =3D=3D 0) > { > - uint32_t op1 =3D bits (insn, 20, 24), op2 =3D bits (insn, 4, 7); > - > if ((op1 & 0x19) !=3D 0x10 && (op2 & 0x1) =3D=3D 0x0) > + /* Data-processing (register) */ > return arm_copy_alu_reg (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) !=3D 0x10 && (op2 & 0x9) =3D=3D 0x1) > + /* Data-processing (register-shifted register) */ > return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) =3D=3D 0x10 && (op2 & 0x8) =3D=3D 0x0) > + /* Miscellaneous instructions */ > return arm_decode_miscellaneous (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) =3D=3D 0x10 && (op2 & 0x9) =3D=3D 0x8) > + /* Halfword multiply and multiply accumulate */ > return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc); > else if ((op1 & 0x10) =3D=3D 0x00 && op2 =3D=3D 0x9) > + /* Multiply and multiply accumulate */ > return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc); > else if ((op1 & 0x10) =3D=3D 0x10 && op2 =3D=3D 0x9) > + /* Synchronization primitives */ > return arm_copy_unmodified (gdbarch, insn, "synch", dsc); These added comments are helpful. > - else if (op2 =3D=3D 0xb || (op2 & 0xd) =3D=3D 0xd) > - /* 2nd arg means "unprivileged". */ > - return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) =3D=3D 0x02, r= egs, > - dsc); > + else if ((op1 & 0x12) !=3D 0x2 && op2 =3D=3D 0xb) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x12) !=3D 0x2 && (op2 & 0xd) =3D=3D 0xd) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x13) =3D=3D 0x2 && (op2 & 0xd) =3D=3D 0xd) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x12) =3D=3D 0x2 && op2 =3D=3D 0xd) > + /* Extra load/store instructions, unprivileged */ > + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); > + else if ((op1 & 0x13) =3D=3D 0x3 && (op2 & 0xd) =3D=3D 0xd) > + /* Extra load/store instructions, unprivileged */ > + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); > + else > + return 1; However, I don't see how helpful or useful the changes above are. > + } > + else > + { > + switch (op1) > + { > + default: > + /* Data-processing (immediate) */ > + return arm_copy_alu_imm (gdbarch, insn, regs, dsc); > + > + case 0x10: > + /* 16-bit immediate load, MOV (immediate) */ > + return arm_copy_unmodified (gdbarch, insn, "movw", dsc); > + > + case 0x14: > + /* High halfword 16-bit immediate load, MOVT */ > + return arm_copy_unmodified (gdbarch, insn, "movt", dsc); > + > + case 0x12: > + case 0x16: > + /* MSR (immediate), and hints */ > + return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); > + } > } > - > - /* Should be unreachable. */ > - return 1; > } In short, I don't see how this patch improve the readability of the code, and I feel hard mapping the code to the manual. --=20 Yao (=E9=BD=90=E5=B0=A7)