[cfe-dev] support __attribute__((weak)) for non-primitive types

[Aaron Ballman via cfe-dev]

On Wed, Aug 25, 2021 at 1:59 PM Y Song <ys114321 at gmail.com> wrote:
>
> On Wed, Aug 25, 2021 at 5:34 AM Aaron Ballman <aaron at aaronballman.com> wrote:
> >
> > On Tue, Aug 24, 2021 at 1:18 AM Y Song <ys114321 at gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > This is a BPF use case. We would like to check whether
> > > __attribute__((weak)) support for non-primitive types would be
> > > possible in clang or not. For example, if we have two types like
> > >    struct t { int a; } __attribute__((weak));
> > >    struct t { int a; };
> > >
> > >    typedef unsigned __u32;
> > >    typedef unsigned __u32 __attribute__((weak));
> > > the compilation should not fail. It should just ignore weak definitions.
> > >
> > > In BPF, to support CO-RE (compile once and run everywhere), we
> > > generate a *generic* vmlinux.h
> > > (https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html)
> > > which is included in the bpf program. The bpf loader (libbpf) will
> > > adjust field offsets properly based on host record layout so the same
> > > program can run on different kernels which may have different record
> > > layouts.
> > >
> > > But vmlinux.h does not support macros (and simple static inline helper
> > > functions in kernel header files). Currently, users need to define
> > > these macros and static inline functions explicitly in their bpf
> > > program. What we are thinking is to permit users to include kernel
> > > header files containing these macros/static inline functions. But this
> > > may introduce duplicated types as these types have been defined in
> > > vmlinux.h.
> > >
> > > For example, we may have:
> > >     socket.h:
> > >         struct socket { ... };
> > >         #define TCP    6
> > >     vmlinux.h:
> > >         struct socket { ... };
> > >     bpf.c:
> > >         #include <socket.h>
> > >         #include <vmlinux.h>
> > >         ... TCP ...
> > >
> > > In this case, struct "socket" is defined twice. If we can have something like
> > >     bpf.c:
> > >       #pragma clang attribute push (__attribute__((weak)), apply_to = record)
> > >       #include <socket.h>
> > >       #pragma clang attribute pop
> > >       #include <vmlinux.h>
> > >         ... TCP ...
> > >
> > >
> > > Then bpf program can get header file macro definitions without copying
> > > explicitly.
> > >
> > > We need support for "apply_to = typedef" in the above pragma, so we
> > > can deal with typedef types as well.
> > >
> > > Another issue is related to what if two types are not compatible. For example,
> > >     struct t { int a; } __attribute__((weak));
> > >     struct t { int a; int b; };
> > > I think we could have a flag to control whether we allow incompatible
> > > weak type or not.
> >
> > What's the use case for allowing incompatible weak types? Having
>
> In our use case, we are talking about incompatible types between
>    . vmlinux.h, generated with a particular kernel config which tries to
>      contains all fields so it can be used for different kernel builds
>    . kernel header files on a particular kernel build which may have
>      less fields than vmlinux.
>
> The below is a concrete example.
> In linux kernel internal header files, many struct definition depends on
> build time configuration. For example, struct "task_struct" in
> linux:include/linux/sched.h,
> https://github.com/torvalds/linux/blob/master/include/linux/sched.h#L661-L1418
> For example, whether field "numa_scan_seq" exists in the struct or not
> depends on
> config option CONFIG_NUMA_BALANCING.
>
> It is totally possible vmlinux.h "task_struct" structure has field
> "numa_scan_seq"
> while the host include/linux/sched.h "task_struct" structure does not have
> "numa_scan_seq" field.

Ah, thank you for the explanation!

> > multiple distinct types seems like it will make for confusion. e.g.,
> >
> > struct foo { int a; };
> > struct __attribute__((whatever_we_name_it)) foo { float f; };
> >
> > int func(struct foo *f) {
> >   return f->f + f->a;
> > }
> >
> > Which lookup causes the error (or do they succeed somehow)?
>
> In our use case, we intend to ensure struct field access *always*
> coming from final structure.

Okay, so "last definition seen wins"?

I think it's a bit odd that the API designer can write a structure
definition and assume that it's the one the user will use, and then a
later definition can come along and silently override it. Say I have
the following:

// AwesomeAPI.h
struct Cool {
  int a;
  float f;
};

void DoAwesomeStuff(struct Cool *C);

// AwesomeAPI.c
#include "AwesomeAPI.h"
#include "string.h"

void DoAwesomeStuff(struct Cool *C) {
  C->a = 12;
  C->f = 1.2;
}

// LessAwesomeAPI.c
#include "AwesomeAPI.h"

struct __attribute__((whatever_we_name_this)) Cool {
  int a;
};

int main(void) {
  struct Cool C;
  DoAwesomeStuff(&C);
}

Assume these TUs are compiled separately and then linked together
later. Is there a buffer overflow in DoAwesomeStuff() because the Cool
object from LessAwesomeAPI.c is only sizeof(int) while the Cool object
expected within AwesomeAPI.c is sizeof(int) + sizeof(float)?

> So, if the final resolved type is in
> vmlinux.h, the field must be in vmlinux.h structure, otherwise,
> issue an error. The same if the final resolved type is in host
> kernel header file.
>
> Therefore, in the above case, f->f is an error since final
> resolved type is "struct foo { int a; }" and field "f" does not
> exist in struct foo.
>
>
>
> >
> > > It would be good if I can get some suggestions/directions on whether
> > > such a feature is possible or not for clang frontend.
> >
> > What happens if the only definition seen in a TU is marked as being weak?
>
> That weak will be promoted to the resolved type.
> >
> > In terms of incompatible weak types, are these incompatible?
> >
> > struct base {
> >   int a;
> > };
> >
> > struct foo : base {
> >   int b;
> > };
> >
> > struct __attribute__((whatever_we_name_it)) foo {
> >   int a, b;
> > }
>
> Our use case is for C only.

That's good to know. In C, are these incompatible?

struct foo {
  int a;
  float f;
};

struct __attribute__((whatever_we_name_this)) foo {
  struct {
    int a;
    float f;
  };
};

(We could ask the same question using anonymous unions, etc.)

> > I'm not comfortable with the idea of having two different type
> > definitions in the same TU unless it's clear and consistent which one
> > "wins". If the type definitions are the same between the types, then I
> > don't see a problem because that limits the chances for surprises.
> > When the type definitions differ, it becomes harder to reason about
> > the way lookup works, how the type is laid out at runtime, what the
> > ABI implications are, etc. Is there a consistent rule for which type
> > is the one to be honored (e.g., only one definition is allowed to
> > elide the attribute and that's the canonical definition of the type
> > which always "wins")?
>
> All these are very good questions. To handle incompatible types,
> yes, we need clear rules. Otherwise, it will cause confusion.
> In our use case, the rule looks like below:
>   - For a particular named type, if only one is defined with
>     "weaktype" attribute, that type becomes resolved type.
>   - If only one is defined without "weaktype" attribute,
>     that type becomes resolved type.
>   - If you have one "weaktype" and one non-weaktype types,
>     the non-weaktype becomes the resolved type.

Oh, this is different from my interpretation of "last one wins" above.
If so, you can reverse which definition the attribute is applied to
and have the same question about buffer overflows.

>   - more than one weaktype type or more than one non-weaktype
>     type will cause a compilation error.

Ah, so there will be exactly 0, 1, or 2 definitions?

>   - any field access will be based on resolve type.
>
> Does this sound reasonable? Or I could still miss some cases?

I apologize if I'm being dense, but I'm still trying to wrap my head
around how this works in practice. I'm worried about linking more than
one TU together where this resolution is different and getting ODR
violations as a result, getting weird situations where sizeof() the
type differs between TUs and causes issues, etc. I suppose one way to
dig into that is: if there was such a thing as a "type sanitizer"
(akin to UBSan but for types), what sort of functionality would you
expect it needs to help programmers detect bugs at runtime?

~Aaron