Re: [PATCH bpf-next v2] docs/bpf: add llvm_reloc.rst to explain llvm bpf relocations

Re: [PATCH bpf-next v2] docs/bpf: add llvm_reloc.rst to explain llvm bpf relocations

[Fāng-ruì Sòng]

On Tue, Jun 8, 2021 at 11:32 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jun 08, 2021 at 09:33:28AM -0700, Fāng-ruì Sòng wrote:
> > On Tue, Jun 8, 2021 at 8:49 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Jun 7, 2021 at 10:51 PM Fāng-ruì Sòng <maskray@google.com> wrote:
> > > >
> > > > You can rename R_BPF_64_64 to something more meaningful, e.g. R_BPF_64_LDIMM64.
> > > > Then I am fine that such a relocation type applies inconsecutive bytes.
> > > >
> > > > See below. Just change every occurrence of the old name in llvm-project.
> > >
> > > No. We cannot rename them, because certain gnu tools resolve relos by name
> > > and not by number.
> >
> > How do the GNU tools resolve relocations by name instead of by
> > relocation type number?
> > I don't think this should and can be supported.
> >
> > Most tools should do:
> > if (type == R_BPF_64_64) do_something();
> >
> > You are free to change them to
> > if (type == R_BPF_64_LDIMM64) do_something();
> > as long as R_BPF_64_LDIMM64 is defined as the number.
>
> If you're going to succeed convincing elfutils maintainers to change
> their whole design then we can realistically talk about renaming.
> As a homework try cloning elfutils.git then change the name in backends/x86_64_reloc.def
> or bpf_reloc.def while keeping the numbers and observe how the standard tools stop working.
>
> Also R_BPF_64_64 may not be the best name, but R_BPF_64_LDIMM64 is
> not a good name either.

I used R_BPF_64_LDIMM64 as an example. Surely you could name it more
appropriately.

> Most architectures avoid using instruction mnemonic
> in relo names. The relo name should describe what it does instead of insn
> it applies to. TLS, GOT, PLT, ABS are good suffixes to use. LDIMM64 - not really.
> Instead of R_BPF_64_32 we could have used R_BPF_64_PC32, but not R_BPF_64_CALL32.
> Anyway it's too late to change.

R_X86_64_PC32/R_X86_64_PLT32 are different.
Please see https://sourceware.org/pipermail/binutils/2020-April/000424.html
for why a dedicated branch relocation
is preferred for a branch instruction.



elfutils folks,

BPF is adding new relocation types R_BPF_64_ABS64/R_BPF_64_ABS32 which
will can cause ongoing confusion with the existing
R_BPF_64_32/R_BPF_64_64.

Can you comment on why elfutils cannot rename R_BPF_64_32/R_BPF_64_64
while keep R_BPF_64_32/R_BPF_64_64 as deprecated aliases for the new
names?

Re: [PATCH bpf-next v2] docs/bpf: add llvm_reloc.rst to explain llvm bpf relocations

[Alexei Starovoitov]

On Tue, Jun 8, 2021 at 4:10 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On Tue, Jun 8, 2021 at 11:32 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jun 08, 2021 at 09:33:28AM -0700, Fāng-ruì Sòng wrote:
> > > On Tue, Jun 8, 2021 at 8:49 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 7, 2021 at 10:51 PM Fāng-ruì Sòng <maskray@google.com> wrote:
> > > > >
> > > > > You can rename R_BPF_64_64 to something more meaningful, e.g. R_BPF_64_LDIMM64.
> > > > > Then I am fine that such a relocation type applies inconsecutive bytes.
> > > > >
> > > > > See below. Just change every occurrence of the old name in llvm-project.
> > > >
> > > > No. We cannot rename them, because certain gnu tools resolve relos by name
> > > > and not by number.
> > >
> > > How do the GNU tools resolve relocations by name instead of by
> > > relocation type number?
> > > I don't think this should and can be supported.
> > >
> > > Most tools should do:
> > > if (type == R_BPF_64_64) do_something();
> > >
> > > You are free to change them to
> > > if (type == R_BPF_64_LDIMM64) do_something();
> > > as long as R_BPF_64_LDIMM64 is defined as the number.
> >
> > If you're going to succeed convincing elfutils maintainers to change
> > their whole design then we can realistically talk about renaming.
> > As a homework try cloning elfutils.git then change the name in backends/x86_64_reloc.def
> > or bpf_reloc.def while keeping the numbers and observe how the standard tools stop working.
> >
> > Also R_BPF_64_64 may not be the best name, but R_BPF_64_LDIMM64 is
> > not a good name either.
>
> I used R_BPF_64_LDIMM64 as an example. Surely you could name it more
> appropriately.
>
> > Most architectures avoid using instruction mnemonic
> > in relo names. The relo name should describe what it does instead of insn
> > it applies to. TLS, GOT, PLT, ABS are good suffixes to use. LDIMM64 - not really.
> > Instead of R_BPF_64_32 we could have used R_BPF_64_PC32, but not R_BPF_64_CALL32.
> > Anyway it's too late to change.
>
> R_X86_64_PC32/R_X86_64_PLT32 are different.
> Please see https://sourceware.org/pipermail/binutils/2020-April/000424.html
> for why a dedicated branch relocation
> is preferred for a branch instruction.
>
>
> elfutils folks,
>
> BPF is adding new relocation types R_BPF_64_ABS64/R_BPF_64_ABS32 which
> will can cause ongoing confusion with the existing
> R_BPF_64_32/R_BPF_64_64.

Not true. There is no confusion.
Everything is clearly documented:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/Documentation/bpf/llvm_reloc.rst

> Can you comment on why elfutils cannot rename R_BPF_64_32/R_BPF_64_64
> while keep R_BPF_64_32/R_BPF_64_64 as deprecated aliases for the new
> names?

To make it clear... we're not proposing to rename or deprecate them.
That's Fang-Rui's suggestion that doesn't make sense to us
due to overhead involved and backward compatibility issues it brings.