Message-ID: <CAH3MdRWrwMgjd7UT+7w=xCmEBD+mxv-YBqW+Q8zL8t5WvGTn8A@mail.gmail.com>
Date:   Fri, 15 Feb 2019 12:18:18 -0800
From:   Y Song <ys114321@...il.com>
To:     Joe Stringer <joe@...d.net.nz>
Cc:     bpf@...r.kernel.org, netdev <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH bpf-next 2/4] libbpf: Support 32-bit static data loads

On Thu, Feb 14, 2019 at 11:16 PM Joe Stringer <joe@...d.net.nz> wrote:
>
> On Thu, 14 Feb 2019 at 21:39, Y Song <ys114321@...il.com> wrote:
> >
> > On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@...d.net.nz> wrote:
> > >
> > > Support loads of static 32-bit data when BPF writers make use of
> > > convenience macros for accessing static global data variables. A later
> > > patch in this series will demonstrate its usage in a selftest.
> > >
> > > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > > complain if this technique is attempted with data of other sizes:
> > >
> > >     LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> > >     or check your static variable usage
> >
> > A little bit clarification from compiler side.
> > The above compiler error is to prevent people use static variables since current
> > kernel/libbpf does not handle this. The compiler only warns if .bss or
> > .data section
> > has more than one definitions. The first definition always has section offset 0
> > and the compiler did not warn.
>
> Ah, interesting. I observed that warning when I attempted to define
> global variables of multiple sizes, and I thought also with sizes
> other than 32-bit. This clarifies things a bit, thanks.
>
> For the .bss my observation was that if you had a definition like:
>
> static int a = 0;
>
> Then this will be placed into .bss, hence why I looked into the
> approach from this patch for patch 3 as well.
>
> > The restriction is a little strange. To only work with 32-bit data is
> > not a right
> > statement. The following are some examples.
> >
> > The following static variable definitions will succeed:
> > static int a; /* one in .bss */
> > static long b = 2;  /* one in .data */
> >
> > The following definitions will fail as both in .bss.
> > static int a;
> > static int b;
> >
> > The following definitions will fail as both in .data:
> > static char a = 2;
> > static int b = 3;
>
> Are there type restrictions or something? I've been defining multiple

There is no type restrictions.
-bash-4.4$ cat g.c
struct t {
  int a;
  char b;
  long c;
};
static volatile struct t v;
int test() { return v.a + v.b; }
-bash-4.4$ clang -O2 -g -c -target bpf g.c
-bash-4.4$

> static uint32_t and using them per the approach in this patch series
> without hitting this compiler assertion.

-bash-4.4$ cat g1.c
static volatile int a;
static volatile int b;
int test() { return a + b; }
-bash-4.4$ clang -O2 -g -c -target bpf g1.c
fatal error: error in backend: Unsupported relocation: try to compile
with -O2 or above, or check your static variable
      usage
-bash-4.4$

>
> > Using global variables can prevent compiler errors.
> > maps are defined as globals and the compiler does not
> > check whether a particular global variable is defining a map or not.
> >
> > If you just use static variable like below
> > static int a = 2;
> > without potential assignment to a, the compiler will replace variable
> > a with 2 at compile time.
> > An alternative is to define like below
> > static volatile int a = 2;
> > You can get a "load" for variable "a" in the bpf load even if there is
> > no assignment to a.
>
> I'll take a closer look at this too.
>
> > Maybe now is the time to remove the compiler assertions as
> > libbpf/kernel starts to
> > handle static variables?
>
> I don't understand why those assertions exists in this form. It
> already allows code which will not load with libbpf (ie generate any
> .data/.bss), does it help prevent unexpected situations for
> developers?

The error is introduced by the following llvm commit:
commit 39184e407cd937f2f20d3f61eec205925bae7b13
Author: Yonghong Song <yhs@...com>
Date:   Wed Aug 22 21:21:03 2018 +0000

    bpf: fix an assertion in BPFAsmBackend applyFixup()

    Fix bug https://bugs.llvm.org/show_bug.cgi?id=38643

    In BPFAsmBackend applyFixup(), there is an assertion for FixedValue to be 0.
    This may not be true, esp. for optimiation level 0.
    For example, in the above bug, for the following two
    static variables:
      @bpf_map_lookup_elem = internal global i8* (i8*, i8*)*
          inttoptr (i64 1 to i8* (i8*, i8*)*), align 8
      @bpf_map_update_elem = internal global i32 (i8*, i8*, i8*, i64)*
          inttoptr (i64 2 to i32 (i8*, i8*, i8*, i64)*), align 8

    The static variable @bpf_map_update_elem will have a symbol
    offset of 8 and a FK_SecRel_8 with FixupValue 8 will cause
    the assertion if llvm is built with -DLLVM_ENABLE_ASSERTIONS=ON.

    The above relocations will not exist if the program is compiled
    with optimization level -O1 and above as the compiler optimizes
    those static variables away. In the below error message, -O2
    is suggested as this is the common practice.

    Note that FixedValue = 0 in applyFixup() does exist and is valid,
    e.g., for the global variable my_map in the above bug. The bpf
    loader will process them properly for map_id's before loading
    the program into the kernel.

    The static variables, which are not optimized away by compiler,
    may have FK_SecRel_8 relocation with non-zero FixedValue.

    The patch removed the offending assertion and will issue
    a hard error as below if the FixedValue in applyFixup()
    is not 0.
      $ llc -march=bpf -filetype=obj fixup.ll
      LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
          or check your static variable usage

Its main purpose is to fix a behavior difference with and without
-DLLVM_ENABLE_ASSERTIONS=ON. The patch generated an error
regardless whether the compiler time assertion is turned on or not.

It does not catch all the cases e.g., only one static variable is defined,
which needs fine tuning as there are legitimate cases (e.g., in some dwarf
sessions) where the Fixup is valid with FixedValue = 0.

If you try to use more than onee static variables, the compiler will
print an error and let you know your potential issues.

The question is since we are on the path to allow static variables
in the bpf programs for later patching, we probably should remove
this compiler fatal error?

Powered by blists - more mailing lists