[cfe-dev] support __attribute__((weak)) for non-primitive types
Y Song via cfe-dev
cfe-dev at lists.llvm.org
Mon Aug 30 08:43:00 PDT 2021
On Mon, Aug 30, 2021 at 4:24 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Sun, Aug 29, 2021 at 2:17 AM Y Song <ys114321 at gmail.com> wrote:
> >
> > On Thu, Aug 26, 2021 at 4:24 AM Aaron Ballman <aaron at aaronballman.com> wrote:
> > >
> > > 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!
> >
> > After a little more thought, I think we will have vmlinux.h ahead of
> > socket.h to make compiler easy to resolve weak types.
> >
> > bpf.c:
> > #include <vmlinux.h>
> > #pragma clang attribute push (__attribute__((weak)), apply_to = any)
> > #include <socket.h>
> > #pragma clang attribute pop
> > ... TCP ...
> >
> > This way, any type in socket.h, if it is also in vmlinux.h, will be quickly
> > discarded.
>
> So the non-weak types are expected to be seen first, so you'll pick
> the non-weak type every time unless the type only exists in weak form?
> That seems reasonable.
>
> > > > > 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)?
> >
> > Thanks for the detailed explanation in the above. This indeed could make
> > it really hard to use for applications.
> >
> > For the weaktype attribute, I am not sure whether there is a use case or
> > not outside the bpf community. How about we design the attribute to
> > be bpf specific. We can name the attribute to be "bpf_weaktype".
>
> That could help make the expected usage domain more clear, but we'll
> still need to consider accidentally incorrect usage to help the bpf
> community. In the above example, I think it's fine to say "if this
> happens, it's UB" and we should probably document it as such.
>
> > We can change rules to be ""last definition seen wins"".
> > - if we only see bpf_weaktype types, last seen definition wins.
> > - as soon as we see one non bpf_weaktype type, that one wins.
> > Any later weaktype type will be ignored and and any later non-weaktype
> > type will cause a redefine compilation error.
>
> I think that makes sense, though I think it could also make sense to
> say "If we only see bpf_weaktype types, the first definition wins and
> any later weaktype definition for the same type will be diagnosed as
> an error. Effectively, you'll see 0, 1, or 2 definitions, but never
> more than two.
What you suggested in above makes sense for us. In bpf use case,
we never see more than 2 definitions. It will be either
(1). non weaktype, or
(2). non weaktype followed by a weaktype, or
(3). weaktype
I think any other cases w.r.t. types can be considered as an error.
>
> > The following is a concrete example:
> > struct t { int a; } __bpf_weaktype;
> > struct d1 { struct t f; }; // use struct t {int a;}
> > struct t { int b; int c; } __bpf_weaktype;
> > struct d2 { struct t f; }; // use struct t {int b; int c; }
>
> The above bit could catch users by surprise. One potential way to
> implement this functionality would be to use a two-pass system where
> pass one finds all the type definitions and resolves them to one type
> and pass two does the actual compilation using that resolved type. In
> that case, a reasonable user might expect d2 to use the `int a;`
> definition. However, if we made additional weak definitions be an
> error, they'd never get into this situation. (And if we have to
> support multiple weaktypes, then we could perhaps warn on the
> situation where a weak definition was used somewhere and then another
> weak definition of the same type is encountered, if that'd make sense
> to do.)
Thanks for the explanation. Let us not allow multiple weak types.
This is not a bpf use case. This should simplify implementation a lot.
>
> > struct t {char c; };
> > struct d3 { strut t f; }; // use struct t { char c; }
> > struct t {short s; } __bpf_weaktype;
> > struct d4 { struct t f; }; // use struct t { char c; }
> > struct t {long long l; }; // compilation error: redefinition
> >
> > >
> > > > 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.)
> >
> > Based on what I described in the above, the first "struct foo" is a
> > non bpf_weaktype and
> > the second is a bpf_weaktype, so the second will be ignored regardless
> > of whether
> > they are compatible or not.
>
> Makes sense, thanks!
>
> > > > > 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
> >
> > Indeed sizeof() may get weird result even in my above example
> > as at different program point, sizeof() of the type might have
> > different values. This may propagate to other types as well.
> >
> > > 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?
> >
> > This is the reason that I would like to limit this attribute to be bpf only.
> > All bpf programs need to pass the kernel verifier before being able to
> > run in the kernel. They may get a program rejected by the kernel or
> > get an incorrect answer but not crash the kernel.
>
> Oh, that's good to know!
>
> > In bpf use case, I expect users mostly use the code pattern
> > #include <vmlinux.h>
> > #pragma clang attribute push (__attribute__((bpf_weaktype)),
> > apply_to = any)
> > #include <socket.h>
> > #pragma clang attribute pop
> > ... TCP ...
> > ... use types from vmlinux.h ...
> >
> > so weird sizeof() issue shouldn't happen.
>
> My gut instinct is that there will be weird issues from this because
> of odd interactions between types (and possibly due to inline
> functions in header files that use the types), but I think there's
> enough here to be worth experimenting with.
Sounds great! We will start to do some prototyping and will have
a RFC diff for discussion once it is ready. Thanks!
>
> Thanks!
>
> ~Aaron
More information about the cfe-dev
mailing list