Message-ID: <CAH3MdRXHdysYLccCsawKqcmzScW=7B-ArPaMq7F8qjm+wsCWQg@mail.gmail.com>
Date:   Wed, 27 Feb 2019 14:42:28 -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

FYI.The latest llvm trunk will not emit errors for static variables. The patch
also gives detailed information how to relate a particular static
variable to a particular
relocation.

https://reviews.llvm.org/rL354954

Thanks,
Yonghong

On Fri, Feb 15, 2019 at 12:18 PM Y Song <ys114321@...il.com> wrote:
>
> 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