From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: David Faust <david.faust@oracle.com>
Cc: binutils@sourceware.org, cupertino.miranda@oracle.com,
indu.bhagat@oracle.com
Subject: Re: [PATCH] bpf: fix calculation when deciding to relax branch
Date: Thu, 25 Apr 2024 21:46:21 +0200 [thread overview]
Message-ID: <87le515ipe.fsf@oracle.com> (raw)
In-Reply-To: <20240425192223.8892-1-david.faust@oracle.com> (David Faust's message of "Thu, 25 Apr 2024 12:22:23 -0700")
Hi David.
OK.
Thanks for the patch!
> In certain cases we were calculating the jump displacement incorrectly
> when deciding whether to relax a branch. This meant for some branches,
> such as a very long backwards conditional branch, relaxation was not
> done when it should have been. The result was to error later, because
> the actual jump displacement was too large to fit in the original
> instruction.
>
> This patch fixes up the displacement calculation so that those branches
> are correctly relaxed and no longer result in an error. In addition, it
> changes md_convert_frag to install fixups for the JAL instructions in
> the resulting relaxations rather than encoding the displacement value
> directly.
>
> Tested on x86_64-linux-gnu host for bpf-unknown-none target.
> Sanity checked building Linux kernel BPF selftests.
>
> gas/
> * config/tc-bpf.c (relaxed_branch_length): Correct displacement
> calculation when relaxing.
> (md_convert_frag): Likewise. Install fixups for JAL
> instructions resulting from relaxation.
> * testsuite/gas/bpf/jump-relax-ja-be.d: Correct and expand test.
> * testsuite/gas/bpf/jump-relax-ja.d: Likewise.
> * testsuite/gas/bpf/jump-relax-ja.s: Likewise.
> * testsuite/gas/bpf/jump-relax-jump-be.d: Likewise.
> * testsuite/gas/bpf/jump-relax-jump.d: Likewise.
> * testsuite/gas/bpf/jump-relax-jump.s: Likewise.
> ---
> gas/config/tc-bpf.c | 38 +++++++++++++++++++---
> gas/testsuite/gas/bpf/jump-relax-ja-be.d | 16 +++++----
> gas/testsuite/gas/bpf/jump-relax-ja.d | 16 +++++----
> gas/testsuite/gas/bpf/jump-relax-ja.s | 13 +++++---
> gas/testsuite/gas/bpf/jump-relax-jump-be.d | 22 ++++++++-----
> gas/testsuite/gas/bpf/jump-relax-jump.d | 22 ++++++++-----
> gas/testsuite/gas/bpf/jump-relax-jump.s | 12 ++++---
> 7 files changed, 96 insertions(+), 43 deletions(-)
>
> diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
> index dfa44ea194c..377ac6a7a4c 100644
> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -434,6 +434,7 @@ relaxed_branch_length (fragS *fragp, asection *sec, int update)
> && sec == S_GET_SEGMENT (fragp->fr_symbol))
> {
> offsetT val = S_GET_VALUE (fragp->fr_symbol) + fragp->fr_offset;
> + val -= fragp->fr_address + fragp->fr_fix;
>
> /* Convert to 64-bit words, minus one. */
> val = (val - 8) / 8;
> @@ -578,6 +579,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNUSED,
> && sec == S_GET_SEGMENT (fragp->fr_symbol))
> {
> offsetT val = S_GET_VALUE (fragp->fr_symbol) + fragp->fr_offset;
> + val -= fragp->fr_address + fragp->fr_fix;
> /* Convert to 64-bit blocks minus one. */
> disp_to_target = (val - 8) / 8;
> disp_is_known = 1;
> @@ -626,15 +628,27 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNUSED,
> {
> /* 16-bit disp is known and not in range. Turn the JA
> into a JAL with a 32-bit displacement. */
> - char bytes[8];
> + char bytes[8] = {0};
>
> bytes[0] = ((BPF_CLASS_JMP32|BPF_CODE_JA|BPF_SRC_K) >> 56) & 0xff;
> bytes[1] = (word >> 48) & 0xff;
> bytes[2] = 0; /* disp16 high */
> bytes[3] = 0; /* disp16 lo */
> - encode_int32 ((int32_t) disp_to_target, bytes + 4);
> -
> write_insn_bytes (buf, bytes);
> +
> + /* Install fixup for the JAL. */
> + reloc_howto_type *reloc_howto
> + = bfd_reloc_type_lookup (stdoutput, BFD_RELOC_BPF_DISP32);
> + if (!reloc_howto)
> + abort();
> +
> + fixp = fix_new_exp (fragp, buf - (bfd_byte *) fragp->fr_literal,
> + bfd_get_reloc_size (reloc_howto),
> + &exp,
> + reloc_howto->pc_relative,
> + BFD_RELOC_BPF_DISP32);
> + fixp->fx_file = fragp->fr_file;
> + fixp->fx_line = fragp->fr_line;
> }
> }
> else
> @@ -731,8 +745,24 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNUSED,
> bytes[1] = 0;
> bytes[2] = 0;
> bytes[3] = 0;
> - encode_int32 ((int32_t) disp_to_target, bytes + 4);
> + encode_int32 ((int32_t) 0, bytes + 4);
> write_insn_bytes (buf, bytes);
> +
> + /* Install fixup for the JAL. */
> + reloc_howto_type *reloc_howto
> + = bfd_reloc_type_lookup (stdoutput, BFD_RELOC_BPF_DISP32);
> + if (!reloc_howto)
> + abort();
> +
> + fixp = fix_new_exp (fragp,
> + buf - (bfd_byte *) fragp->fr_literal,
> + 4 /* size */,
> + &exp,
> + true /* pc-relative */,
> + BFD_RELOC_BPF_DISP32);
> + fixp->fx_file = fragp->fr_file;
> + fixp->fx_line = fragp->fr_line;
> +
> buf += 8;
> }
> }
> diff --git a/gas/testsuite/gas/bpf/jump-relax-ja-be.d b/gas/testsuite/gas/bpf/jump-relax-ja-be.d
> index 5f07847e90c..e5ad30da807 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-ja-be.d
> +++ b/gas/testsuite/gas/bpf/jump-relax-ja-be.d
> @@ -8,10 +8,14 @@
> Disassembly of section .text:
>
> 0+ <.*>:
> - 0: 05 00 80 00 00 00 00 00 ja -32768
> - 8: 05 00 7f ff 00 00 00 00 ja 32767
> - 10: 05 00 ff fd 00 00 00 00 ja -3
> - 18: 05 00 00 00 00 00 00 00 ja 0
> + 0: 05 00 80 00 00 00 00 00 ja -32768
> + 8: 05 00 7f ff 00 00 00 00 ja 32767
> + 10: 05 00 ff fd 00 00 00 00 ja -3
> + 18: 05 00 00 00 00 00 00 00 ja 0
> 18: R_BPF_GNU_64_16 undefined
> - 20: 06 00 00 00 00 00 80 01 jal 32769
> - 28: 06 00 00 00 00 00 80 01 jal 32769
> + 20: 06 00 00 00 00 00 80 00 jal 32768
> + 28: 05 00 7f ff 00 00 00 00 ja 32767
> + ...
> +
> +0+40028 <tail>:
> + 40028: 06 00 00 00 ff ff 7f fa jal -32774
> diff --git a/gas/testsuite/gas/bpf/jump-relax-ja.d b/gas/testsuite/gas/bpf/jump-relax-ja.d
> index ed3aa6bad4e..1ac33f5d2f5 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-ja.d
> +++ b/gas/testsuite/gas/bpf/jump-relax-ja.d
> @@ -8,10 +8,14 @@
> Disassembly of section .text:
>
> 0+ <.*>:
> - 0: 05 00 00 80 00 00 00 00 ja -32768
> - 8: 05 00 ff 7f 00 00 00 00 ja 32767
> - 10: 05 00 fd ff 00 00 00 00 ja -3
> - 18: 05 00 00 00 00 00 00 00 ja 0
> + 0: 05 00 00 80 00 00 00 00 ja -32768
> + 8: 05 00 ff 7f 00 00 00 00 ja 32767
> + 10: 05 00 fd ff 00 00 00 00 ja -3
> + 18: 05 00 00 00 00 00 00 00 ja 0
> 18: R_BPF_GNU_64_16 undefined
> - 20: 06 00 00 00 01 80 00 00 jal 32769
> - 28: 06 00 00 00 01 80 00 00 jal 32769
> + 20: 06 00 00 00 00 80 00 00 jal 32768
> + 28: 05 00 ff 7f 00 00 00 00 ja 32767
> + ...
> +
> +0+40028 <tail>:
> + 40028: 06 00 00 00 fa 7f ff ff jal -32774
> diff --git a/gas/testsuite/gas/bpf/jump-relax-ja.s b/gas/testsuite/gas/bpf/jump-relax-ja.s
> index f164176bbd5..61f2e61cb3c 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-ja.s
> +++ b/gas/testsuite/gas/bpf/jump-relax-ja.s
> @@ -4,13 +4,18 @@
> 1: ja -32768
> ja 32767
> /* The following instruction refers to a defined symbol that
> - is on reach, so it should not be relaxed. */
> + is in reach, so it should not be relaxed. */
> ja 1b
> /* The following instruction has an undefined symbol as a
> target. It is not to be relaxed. */
> ja undefined + 10
> - /* The following instructions refer to a defined symbol that
> - is not on reach. They shall be relaxed to a JAL. */
> + /* The following instruction refers to a defined symbol that
> + is not in reach, so it shall be relaxed to JAL. */
> ja tail
> - tail = .text + 262160
> + /* Now the symbol is in reach, and the following instruction
> + shall not be relaxed. */
> ja tail
> + .space 262136
> + tail = .
> + /* The jump back is too large and shall be relaxed. */
> + ja 1b
> diff --git a/gas/testsuite/gas/bpf/jump-relax-jump-be.d b/gas/testsuite/gas/bpf/jump-relax-jump-be.d
> index 0cacdb38fc9..5cf3bce2a2b 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-jump-be.d
> +++ b/gas/testsuite/gas/bpf/jump-relax-jump-be.d
> @@ -8,12 +8,16 @@
> Disassembly of section .text:
>
> 0+ <.*>:
> - 0: 1d 12 80 00 00 00 00 00 jeq %r1,%r2,-32768
> - 8: ad 12 7f ff 00 00 00 00 jlt %r1,%r2,32767
> - 10: bd 12 ff fd 00 00 00 00 jle %r1,%r2,-3
> - 18: 1d 12 00 01 00 00 00 00 jeq %r1,%r2,1
> - 20: 05 00 00 01 00 00 00 00 ja 1
> - 28: 06 00 00 00 00 00 80 01 jal 32769
> - 30: 2d 12 00 01 00 00 00 00 jgt %r1,%r2,1
> - 38: 05 00 00 01 00 00 00 00 ja 1
> - 40: 06 00 00 00 00 00 80 01 jal 32769
> + 0: 1d 12 80 00 00 00 00 00 jeq %r1,%r2,-32768
> + 8: ad 12 7f ff 00 00 00 00 jlt %r1,%r2,32767
> + 10: bd 12 ff fd 00 00 00 00 jle %r1,%r2,-3
> + 18: 1d 12 00 01 00 00 00 00 jeq %r1,%r2,1
> + 20: 05 00 00 01 00 00 00 00 ja 1
> + 28: 06 00 00 00 00 00 80 00 jal 32768
> + 30: 2d 12 7f ff 00 00 00 00 jgt %r1,%r2,32767
> + ...
> +
> +0+40030 <tail>:
> + 40030: 55 10 00 01 00 00 00 00 jne %r1,0,1
> + 40038: 05 00 00 01 00 00 00 00 ja 1
> + 40040: 06 00 00 00 ff ff 7f f7 jal -32777
> diff --git a/gas/testsuite/gas/bpf/jump-relax-jump.d b/gas/testsuite/gas/bpf/jump-relax-jump.d
> index dd31ba5f4e4..4300bf3f59f 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-jump.d
> +++ b/gas/testsuite/gas/bpf/jump-relax-jump.d
> @@ -8,12 +8,16 @@
> Disassembly of section .text:
>
> 0+ <.*>:
> - 0: 1d 21 00 80 00 00 00 00 jeq %r1,%r2,-32768
> - 8: ad 21 ff 7f 00 00 00 00 jlt %r1,%r2,32767
> - 10: bd 21 fd ff 00 00 00 00 jle %r1,%r2,-3
> - 18: 1d 21 01 00 00 00 00 00 jeq %r1,%r2,1
> - 20: 05 00 01 00 00 00 00 00 ja 1
> - 28: 06 00 00 00 01 80 00 00 jal 32769
> - 30: 2d 21 01 00 00 00 00 00 jgt %r1,%r2,1
> - 38: 05 00 01 00 00 00 00 00 ja 1
> - 40: 06 00 00 00 01 80 00 00 jal 32769
> + 0: 1d 21 00 80 00 00 00 00 jeq %r1,%r2,-32768
> + 8: ad 21 ff 7f 00 00 00 00 jlt %r1,%r2,32767
> + 10: bd 21 fd ff 00 00 00 00 jle %r1,%r2,-3
> + 18: 1d 21 01 00 00 00 00 00 jeq %r1,%r2,1
> + 20: 05 00 01 00 00 00 00 00 ja 1
> + 28: 06 00 00 00 00 80 00 00 jal 32768
> + 30: 2d 21 ff 7f 00 00 00 00 jgt %r1,%r2,32767
> + ...
> +
> +0+40030 <tail>:
> + 40030: 55 01 01 00 00 00 00 00 jne %r1,0,1
> + 40038: 05 00 01 00 00 00 00 00 ja 1
> + 40040: 06 00 00 00 f7 7f ff ff jal -32777
> diff --git a/gas/testsuite/gas/bpf/jump-relax-jump.s b/gas/testsuite/gas/bpf/jump-relax-jump.s
> index 5ea61109bca..6b23c56514a 100644
> --- a/gas/testsuite/gas/bpf/jump-relax-jump.s
> +++ b/gas/testsuite/gas/bpf/jump-relax-jump.s
> @@ -2,11 +2,13 @@
> fix in the jump 16-bit signed displacement operand. */
> 1: jeq %r1, %r2, -32768
> jlt %r1, %r2, 32767
> - /* The following instruction refers to a defined symbol that
> - is on reach, so it should not be relaxed. */
> + /* The following instructions refer to defined symbols that
> + are in reach, so they should not be relaxed. */
> jle %r1, %r2, 1b
> - /* The following instructions refer to a defined symbol that
> - is not on reach. They shall be relaxed. */
> jeq %r1, %r2, tail
> - tail = .text + 262160
> jgt %r1, %r2, tail
> + /* The following instructions refers to defined symbols that
> + are not in reach, so they shall be relaxied. */
> + .space 262136
> + tail = .
> + jne %r1, 0, 1b
next prev parent reply other threads:[~2024-04-25 19:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 19:22 David Faust
2024-04-25 19:46 ` Jose E. Marchesi [this message]
2024-04-25 19:50 ` David Faust
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=87le515ipe.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=binutils@sourceware.org \
--cc=cupertino.miranda@oracle.com \
--cc=david.faust@oracle.com \
--cc=indu.bhagat@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).