From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender.a4lg.com [153.120.152.154]) by sourceware.org (Postfix) with ESMTPS id 5CC1B3858D3C for ; Sun, 16 Oct 2022 13:32:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5CC1B3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=irq.a4lg.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=irq.a4lg.com Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id CEFFC300089; Sun, 16 Oct 2022 13:32:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1665927136; bh=AGqHTcbsVN9xuUWAr3OVjy20Zz3dky0LwsD0rbAeXnw=; h=Message-ID:Date:Mime-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=N8AF/DVukG5tciCPGQ74Y4TXKa+3lu32m1KThVaoyrEntctU/ZUkxNK8GSI7w6M6E CSCLVC9y9DuAE2X/VJ601kZi065nGtPER0pIH9SDv/lVGtbKJT108/TRR+31IaqRDT Q7DHUSS9ppCGrVUEIQdV7CCNlkN9Tww7XE6ZFIFw= Message-ID: Date: Sun, 16 Oct 2022 22:32:15 +0900 Mime-Version: 1.0 Subject: Re: [PATCH v3 1/2] RISC-V: Fallback for instructions longer than 64b Content-Language: en-US To: Nelson Chu , Jan Beulich Cc: Kito Cheng , Palmer Dabbelt , binutils@sourceware.org References: From: Tsukasa OI In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2022/10/14 10:32, Nelson Chu wrote: > In fact we don't really need this change, since so far the parameter > length of validate_riscv_insn will only be 2 and 4, > https://github.com/bminor/binutils-gdb/blob/master/gas/config/tc-riscv.c#L134 The argument "length" of validate_riscv_insn will be 2, 4 OR 0 but this is not the point. I have to agree that this change alone will not change the behavior. Still, I don't want to surprise future developers with "instruction length support" status (as Jan added). To note, commit message of PATCH 1/2 is an important part of this patchset. Could you reconsider this patchset again? Thanks, Tsukasa > > Nelson > > On Thu, Oct 6, 2022 at 5:56 PM Tsukasa OI wrote: >> >> We don't support instructions longer than 64-bits yet. Still, we can >> modify validate_riscv_insn function to prevent unexpected behavior by >> limiting the "length" of an instruction to 64-bit (or less). >> >> gas/ChangeLog: >> >> * config/tc-riscv.c (validate_riscv_insn): Fix function >> description comment based on current spec. Limit instruction >> length up to 64-bit for now. Make sure that required_bits does >> not corrupt even if unsigned long long is longer than 64-bit. >> --- >> gas/config/tc-riscv.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c >> index 22385d1baa0..41d6dfc6062 100644 >> --- a/gas/config/tc-riscv.c >> +++ b/gas/config/tc-riscv.c >> @@ -1109,7 +1109,8 @@ arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop) >> >> /* For consistency checking, verify that all bits are specified either >> by the match/mask part of the instruction definition, or by the >> - operand list. The `length` could be 0, 4 or 8, 0 for auto detection. */ >> + operand list. The `length` could be the actual instruction length or >> + 0 for auto-detection. */ >> >> static bool >> validate_riscv_insn (const struct riscv_opcode *opc, int length) >> @@ -1120,11 +1121,13 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length) >> insn_t required_bits; >> >> if (length == 0) >> - insn_width = 8 * riscv_insn_length (opc->match); >> - else >> - insn_width = 8 * length; >> + length = riscv_insn_length (opc->match); >> + /* We don't support instructions longer than 64-bits yet. */ >> + if (length > 8) >> + length = 8; >> + insn_width = 8 * length; >> >> - required_bits = ~0ULL >> (64 - insn_width); >> + required_bits = ((insn_t)~0ULL) >> (64 - insn_width); >> >> if ((used_bits & opc->match) != (opc->match & required_bits)) >> { >> -- >> 2.34.1 >> >