From: Guillermo Martinez <guillermo.e.martinez@oracle.com>
To: Jose Marchesi <jose.marchesi@oracle.com>, Alan Modra <amodra@gmail.com>
Cc: "cgen@sourceware.org" <cgen@sourceware.org>,
"binutils@sourceware.org" <binutils@sourceware.org>
Subject: Re: [PATCH v2] cgen: Compute correct mask and values when offset in define-ifield is not 0.
Date: Thu, 25 Nov 2021 23:28:31 +0000 [thread overview]
Message-ID: <10060503.AQFEo9pWI6@sali> (raw)
In-Reply-To: <YaAV83hDtXjBYEye@squeak.grove.modra.org>
On Thursday, November 25, 2021 5:02:11 PM CST Alan Modra wrote:
> On Wed, Sep 15, 2021 at 06:04:44PM +0000, Guillermo Martinez wrote:
> > On Wednesday, September 15, 2021 11:49:12 AM CDT Jose E. Marchesi wrote:
> > > I just installed this version of the patch on your behalf.
> > > Thanks!
> > >
> > > > If an instruction field is defined in a long form, assigning
> > > > an offset different to 0 the mask and constant values are not
> > > > computed appropriately:
> [snip]
>
> It's been a while since I refreshed the binutils opcodes files that
> are cgen generated. On doing so it appears this patch is responsible
> for build errors when using mainline gcc. First example:
>
> /home/alan/src/binutils-gdb/opcodes/bpf-opc.c:57:11: error: conversion from ‘long unsigned int’ to ‘unsigned int’ changes value from ‘18446744073709486335’ to ‘4294902015’ [-Werror=overflow]
> 57 | 64, 64, 0xffffffffffff00ff, { { F (F_IMM32) }, { F (F_OFFSET16) }, { F (F_SRCLE) }, { F (F_OP_CODE) }, { F (F_DSTLE) }, { F (F_OP_SRC) }, { F (F_OP_CLASS) }, { 0 } }
> | ^~~~~~~~~~~~~~~~~~
>
> I tested an obvious workaround,
>
> diff --git a/include/opcode/cgen.h b/include/opcode/cgen.h
> index 8b7d2a4b547..0e9571d19b0 100644
> --- a/include/opcode/cgen.h
> +++ b/include/opcode/cgen.h
> @@ -914,7 +914,7 @@ typedef struct
> Each insn's value is stored with the insn.
> The first step in recognizing an insn for disassembly is
> (opcode & mask) == value. */
> - CGEN_INSN_INT mask;
> + uint64_t mask;
> #define CGEN_IFMT_MASK(ifmt) ((ifmt)->mask)
>
> /* Instruction fields.
> diff --git a/opcodes/cgen-dis.c b/opcodes/cgen-dis.c
> index 1a5d1ae8459..37ee5a23564 100644
> --- a/opcodes/cgen-dis.c
> +++ b/opcodes/cgen-dis.c
> @@ -39,7 +39,7 @@ static void add_insn_to_hash_chain (CGEN_INSN_LIST *,
> static int
> count_decodable_bits (const CGEN_INSN *insn)
> {
> - unsigned mask = CGEN_INSN_BASE_MASK (insn);
> + uint64_t mask = CGEN_INSN_BASE_MASK (insn);
> #if GCC_VERSION >= 3004
> return __builtin_popcount (mask);
> #else
>
> Quite possibly the above isn't a complete fix, I just threw it
> together to see what happens. Regressions:
correct, it's not a complete fix:
https://sourceware.org/pipermail/binutils/2021-October/118284.html
However applying patch v2 on upstream pitifully it won't work because there is
an error in BPF simulator (sim/bpf/decode-le.c (extract_sfmt_lddwle))
(not introduced by this patch) but now raised by adding -Werror=shift-count-overflow
in binutils-gdb builder (currently im debugging this :-))
> +FAIL: eBPF CALL instruction
> +FAIL: eBPF CALL instruction, big endian
> +FAIL: CALL with disp32 reloc
> +FAIL: CALL with disp32 reloc and addend
> +FAIL: CALL check unsigned underflow
>
> gas/testsuite/gas.log for the first one shows:
> regexp_diff match failure
> regexp "^ 20: 85 10 00 00 00 00 00 00 call 0$"
> line " 20: 85 10 00 00 00 00 00 00 *unknown*"
> regexp_diff match failure
> regexp "^ 28: 85 10 00 00 ff ff ff ff call -1$"
> line " 28: 85 10 00 00 ff ff ff ff *unknown*"
> regexp_diff match failure
> regexp "^ 30: 85 10 00 00 fe ff ff ff call -2$"
> line " 30: 85 10 00 00 fe ff ff ff *unknown*"
> regexp_diff match failure
> regexp "^ 38: 85 10 00 00 fd ff ff ff call -3$"
> line " 38: 85 10 00 00 fd ff ff ff *unknown*"
> FAIL: eBPF CALL instruction
>
> At this point I don't intend to look any further into the problem.
> The opcodes/ refresh can wait.
I'll work on this ASAP.
Thanks Alan!
Guillermo
next prev parent reply other threads:[~2021-11-25 23:28 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-20 2:32 [PATCH CGEN v1] " Guillermo E. Martinez
2021-09-02 13:22 ` Guillermo Martinez
2021-09-10 1:23 ` Frank Ch. Eigler
2021-09-10 2:29 ` Jose E. Marchesi
2021-09-10 2:36 ` Jose E. Marchesi
2021-09-14 21:34 ` Jose E. Marchesi
2021-09-15 16:32 ` [PATCH v2] " Guillermo E. Martinez
2021-09-15 16:49 ` Jose E. Marchesi
2021-09-15 18:04 ` Guillermo Martinez
2021-11-25 23:02 ` Alan Modra
2021-11-25 23:28 ` Guillermo Martinez [this message]
2021-11-26 0:46 ` Alan Modra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10060503.AQFEo9pWI6@sali \
--to=guillermo.e.martinez@oracle.com \
--cc=amodra@gmail.com \
--cc=binutils@sourceware.org \
--cc=cgen@sourceware.org \
--cc=jose.marchesi@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).