From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf32.google.com (mail-qv1-xf32.google.com [IPv6:2607:f8b0:4864:20::f32]) by sourceware.org (Postfix) with ESMTPS id 57B98385782C for ; Wed, 14 Feb 2024 04:17:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 57B98385782C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=obs.cr Authentication-Results: sourceware.org; spf=none smtp.mailfrom=obs.cr ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 57B98385782C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::f32 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707884242; cv=none; b=uuF5ecRe6xIH1NZN7cW0Nyl/yIwDDuiY5JiecTyyYAiMuarumf/Svj9CK3vk1A/g+p4xdiO5ZLoFEAhij4bJLkd4SwGN65RRcVjSj2G5MLlWRNROIt8mbd+hB1tjzGIDwk+iiJmHHRUghwxoqISCdkCBOsI3RhZ6hHNhjDTpeH4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707884242; c=relaxed/simple; bh=IDKfu9me94QsOvJWuJqLie9YRDaRavMCI85NBzbroBo=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=H1uCKP7NDRIZz9JTF+eg2Yh/xI7mG7YxDy6ifSKRq/Zmnpmuiy3v6KpeUWblgUuqFp0xeMEwXjLBKYlBMTZzRCSNWPRTdvYxJ48ZAF5TtYzyt/3BzKdGoTMQxQrdy2iD2tbqUOOm6HWsn2YPtRHPzWK/5yg6ULuIoZPkH0hMLas= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-qv1-xf32.google.com with SMTP id 6a1803df08f44-6818aa08a33so3896526d6.0 for ; Tue, 13 Feb 2024 20:17:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=obs-cr.20230601.gappssmtp.com; s=20230601; t=1707884239; x=1708489039; darn=sourceware.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=VSkGuI6pPY9c/mXeDZe0NkhuUaJ4wcdVJ/RgdBPIBNg=; b=HN/ZpN6GRL40VlRTMEYMWp1lh/nk0P9zD/0E3PhCTlDOa8Uk99ZLnza1ZUU7wBgtQ6 tIJ4z59gkIG4yi1+G+v194+PtZMj0h+PQwAun9mN2vvcM2WxZrwHHC/TjxOxunOI2W5m 8omJ/YwizRaDGGJdlDXuvEB4cTWSq0S6ZOF36DFb1oncEl/r5dnDr6Cca3BNRx/gjcw0 0qxUhX4hUmTn3MK+Qs6ZhP9EoeZhuxCvWyurp4Jea8+K/D/kVQwvCNd2pLDrwb9H/hB/ Iny3qiAzbYhNJJKGyxo/sHs7mA0adZXKAEWZp4NZdSETdCbcYjPEB/EXgnT5Pr685dK7 cNpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707884239; x=1708489039; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=VSkGuI6pPY9c/mXeDZe0NkhuUaJ4wcdVJ/RgdBPIBNg=; b=vXm47aAhZcMU4eZxVRIjdlvPx0e+oxwHpo6BL42CxB6bDZ5KnozdbjUDhEyEH6hDId w9iXj4zoIjSrcn5KgYi0/VoqjH3rt80OD9RlBw+M0J6UKYxWzNbWVKaTsAxsrZyzEFHN wpPGU6qsLNm4kz6F9PqFvTpwS/Z9Zn+tqVN+E5OcYkjhmon0VyXuYdAsfEJhLIjDvHyh OWmZixTsL4M1qZ7palZ/ERnWMtbkjB6SDLHn3+juHYaqhCWgW5hcvA7x4fOAHzM3ujTV zWMjaKqMnRnF8FMTYD82UrnsB2N7vSdg0N1BDk/ZT3JmDgRyOR1hNcTaKNb0TzO+e4Lu RvVw== X-Gm-Message-State: AOJu0YwFDvmTiGuk9TN6dQA7kxmDZdH2pqGSljXYDPoBvMLqco4wVatg beoQNwtEfqeYoDC2XWqimrloWeb6XR4r84yGHCUIgUmMWG1DWwKHK5f9oZXroQfS2SyxOf98Uqf CnT3hh0EKPt7Q4ciQk3H2Yu2WTqYnh0SdyL20w0YBiQd3dLtPvSU= X-Google-Smtp-Source: AGHT+IHTpVIXL/u04Ekmx7VErz8FT1fCSkPLtfCJ6NlqzZeQN12w92tMrqs+R46m3/tT+vmiIkZOcKUeDChZa6RA+JY= X-Received: by 2002:a0c:ab12:0:b0:68c:b9e3:1c13 with SMTP id h18-20020a0cab12000000b0068cb9e31c13mr1164642qvb.9.1707884239481; Tue, 13 Feb 2024 20:17:19 -0800 (PST) MIME-Version: 1.0 References: <20240212174209.620310-1-hawkinsw@obs.cr> <20240212174209.620310-2-hawkinsw@obs.cr> <82116e8c-390b-4389-8d04-744839785f2c@redhat.com> In-Reply-To: <82116e8c-390b-4389-8d04-744839785f2c@redhat.com> From: Will Hawkins Date: Tue, 13 Feb 2024 23:17:08 -0500 Message-ID: Subject: Re: [PATCH v2 1/1] objdump, as: Add callx support for BPF CPU v1 To: Nick Clifton Cc: binutils@sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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 Tue, Feb 13, 2024 at 6:57=E2=80=AFAM Nick Clifton wro= te: > > Hi Will, > > > Add support for (dis)assembling the callx instruction back to CPU v1. > > I am not going to comment on the callx instruction itself, since I am > not a BPF expert and it seems like you are already working on that with > Jose and YYonghong. Instead I thought that I would comment on the patch > instead... Nick, First, as I said yesterday in my direct message to you, thank you for making binutils such a pleasant place to contribute to FOSS. You have no idea what that means to contributors like me! Second, thank you for this helpful critique. I really appreciated reading your feedback and will reply inline below (including with an offer for a patch unrelated to callx that may clean up some of the non-constant uses throughout the bpf-specific code). Third, there is good news: The heavy lifting of this patch is largely "overcome by events" -- clang/llvm developers are changing their encoding of the callx instruction to more closely match what gcc does. In fact, v3 of this patch will look much more like v1 than v2. And now, on with the action ... > > > > Signed-off-by: Will Hawkins > > Just to be clear: your sign off here does mean that you agree to the > Developer Certificate of Origin (v1.1) right ? [I am being paranoid...:-= ] > Yes, 100%! I appreciate you checking but I specifically included the sign off as my agreement. If there is a better way to positively signal that intention, please let me know! > > > > @@ -935,7 +937,13 @@ encode_insn (struct bpf_insn *insn, char *bytes, > > if (immediate_overflow (imm, 32)) > > as_bad (_("immediate out of range, shall fit in 32 bits")); > > else > > - encode_int32 (insn->imm32.X_add_number, bytes + 4); > > + encode_int32 (insn->imm32.X_add_number, bytes + 4); > > + } > > + > > + if (insn->has_rmm32) > > + { > > + int64_t imm =3D insn->rmm32; > > + encode_int32 (imm, bytes + 4); > > } > > > > if (insn->has_disp32 && insn->disp32.X_op =3D=3D O_constant) > > This frag struck me as strange since it appears that the first change is = to > replace a line with itself. Looking a bit closer I saw that you are remo= ving > some unnecessary spaces at the end of the line. Which is good and not a > problem. It just looked odd when I was reviewing the patch. I was hesitant to include this piece in the PR because I know that whitespace patches are always a touchy subject (see below for my offer and how it relates to this line of code ...) > > The second part of the frag also seems a little odd. Given that encode_i= nt32() > takes an int32_t as its first parameter, why do you use int64_t as the ty= pe > for "imm" ? In fact why have a local variable at all ? Wouldn't it be s= impler > to just have: > > if (insn->has_rmm32) > encode_int32 (insn->rmm32, bytes + 4); > Yes, absolutely. This local/64-bit difference was left over from a piece of the code that I replaced. I had imm as a way to perform a similar out-of-32-bit-range-check as the block above. I removed that after revamping the way that the instruction was parsed (I originally parsed the "register" as a signed number and had protections to check for overflow). I think that your implementation is unquestionably better. > > > > @@ -1610,6 +1618,21 @@ md_assemble (char *str ATTRIBUTE_UNUSED) > > insn.has_imm32 =3D 1; > > p +=3D 4; > > } > > + else if (strncmp (p, "%r32", 4) =3D=3D 0) > > This is not a criticism, but merely a comment. I dislike the presence > of "magic" numbers in code, numbers whose value may or may not be obvious > to the reader and which may appear more than once. Here is my offer (hinted at above): There are several parts of this particular file that appear out of the ordinary: 1. Inconsistent tab/space usage 2. The presence of magic values 3. Spaces at the end of lines And so on. If I were to take a pass at "cleaning up" the code in this file, would you be interested in having a PR? As I said, I really appreciate your openness to my contribution and I love working with FOSS. In other words, I would really enjoy contributing. That said, I don't want to step in and intrude on someone else's territory. Just let me know! > > So for example the number 4 in the code line above is "magic" to my mind, > and a potential source of problems. Imagine that at some point in the > future the name of the register was changed to "%foo32". Most coders > would realise that the 4 is the length of the "%r32" string and so change > it to 5, but would they also realise that the 4 is reused later on to > adjust the "p" pointer: > > > + p +=3D 4; > > Now you are following the code style that is already present in the tc-bp= f.c > file, so there is no fault there. But, in my opinion, the style is wrong= . > I would rather see something like this: > > #define RMM32_REG_NAME "%r32" > #define RMM32_REG_LEN strlen (RMM32_REG_NAME) > > [...] > else if (strncmp (p, RMM32_REG_NAEM, RMM32_REG_LEN) =3D=3D 0) > { > [...] > p +=3D RMM32_REG_LEN; > } > > That way if the name is ever changed, all the other adjustments > would happen automatically. Plus if those defines were in a BPF > specific header file then they could be reused elsewhere, eg in > the disassembler. > > Alternatively ... there is a function called startswith() which is used > in the assembler in lots of places, and you could use this to make the > if-statement simpler, ie: > > [...] > else if (startswith (p, "%r32") > { > [...] > > This does not resolve the use of the magic value 4 later on, but still > makes the code look cleaner. Just my opnion of course. > See above ... (smiley!) > > > > diff --git a/opcodes/bpf-dis.c b/opcodes/bpf-dis.c > > index d4020c259fb..50e714cb74b 100644 > > --- a/opcodes/bpf-dis.c > > +++ b/opcodes/bpf-dis.c > > @@ -251,6 +251,12 @@ print_insn_bpf (bfd_vma pc, disassemble_info *info= ) > > imm32); > > p +=3D 4; > > } > > + else if (strncmp (p, "%r32", 4) =3D=3D 0) > > + { > > + int32_t imm32 =3D bpf_extract_imm32 (word, endian); > > + print_register (info, p, imm32); > > + p +=3D 4; > > + } > > See the comments above. :-) > > > > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > > > @@ -254,6 +254,7 @@ struct bpf_opcode > > %d16 - 16-bit signed displacement (in 64-bit words minus one.) > > %o16 - 16-bit signed offset (in bytes.) > > %i32 - 32-bit signed immediate. > > + %r32 - 32-bit signed immediate indicating a register. > > %I32 - Like %i32. > > %i64 - 64-bit signed immediate. > > %w - expect zero or more white spaces and print a single space. > > Why is %r32 signed ? Does the BPF format support negative register numbe= rs ? > > This is a great question. I struggled to decide what type to make this format code represent. I ultimately went with 32-bit signed because the immediate value in a BPF instruction is "typically" considered to be signed (https://www.ietf.org/archive/id/draft-thaler-bpf-isa-00.html#sec= tion-3-6.2). Of course, you are correct in saying that register numbers are most likely (I am being facetious, of course!) not going to be negative but, because of the choice to encode a register in this odd place, I didn't think that I had much of a choice. The good news is that we won't have to make this lesser-of-two-evils choice. > > > > diff --git a/sim/bpf/bpf-sim.c b/sim/bpf/bpf-sim.c > > Just FYI - the simulator is part of the GDB project, not the Binutils > project, so you will need to submit the patch to bpf-sim.c to them, not > us. The email address to use is: gdb-patches@sourceware.org I had no idea! Thank you! I will make sure that any changes to the simulator in v3 are directed to them. Sorry for the additional overhead and thank you for the heads up! > > Cheers > Nick > > PS. Please don't think that my comments are meant to be criticisms of > your code. The code is good. I am merely trying to provide some > suggestions to make it even better. :-) I will say it again: I sincerely appreciate the time you took out of your day to review this code. As an active participant in the world of FOSS and, in particular, BPF, I hope that this contribution will be the first of many. Thanks again! Will > > > >