Message ID | 20200811030852.3396929-1-yhs@fb.com |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf] libbpf: do not use __builtin_offsetof for offsetof | expand |
On Mon, Aug 10, 2020 at 8:09 PM Yonghong Song <yhs@fb.com> wrote: > > Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro > in bpf_helpers.h") added a macro offsetof() to get the offset of a > structure member: > #define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER) > > In certain use cases, size_t type may not be available so > Commit da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof > for offsetof") changed to use __builtin_offsetof which removed > the dependency on type size_t, which I suggested. > > But using __builtin_offsetof will prevent CO-RE relocation > generation in case that, e.g., TYPE is annotated with "preserve_access_info" > where a relocation is desirable in case the member offset is changed > in a different kernel version. So this patch reverted back to > the original macro but using "unsigned long" instead of "site_t". > > Cc: Ian Rogers <irogers@google.com> > Fixes: da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof for offsetof") > Signed-off-by: Yonghong Song <yhs@fb.com> > --- LGTM. Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/lib/bpf/bpf_helpers.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index bc14db706b88..e9a4ecddb7a5 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -40,7 +40,7 @@ > * Helper macro to manipulate data structures > */ > #ifndef offsetof > -#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER) > +#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER) > #endif > #ifndef container_of > #define container_of(ptr, type, member) \ > -- > 2.24.1 >
On Mon, Aug 10, 2020 at 9:33 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Aug 10, 2020 at 8:09 PM Yonghong Song <yhs@fb.com> wrote: > > > > Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro > > in bpf_helpers.h") added a macro offsetof() to get the offset of a > > structure member: > > #define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER) > > > > In certain use cases, size_t type may not be available so > > Commit da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof > > for offsetof") changed to use __builtin_offsetof which removed > > the dependency on type size_t, which I suggested. > > > > But using __builtin_offsetof will prevent CO-RE relocation > > generation in case that, e.g., TYPE is annotated with "preserve_access_info" > > where a relocation is desirable in case the member offset is changed > > in a different kernel version. So this patch reverted back to > > the original macro but using "unsigned long" instead of "site_t". > > > > Cc: Ian Rogers <irogers@google.com> > > Fixes: da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof for offsetof") > > Signed-off-by: Yonghong Song <yhs@fb.com> > > --- > > LGTM. > > Acked-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Ian Rogers <irogers@google.com> Thanks, Ian > > tools/lib/bpf/bpf_helpers.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index bc14db706b88..e9a4ecddb7a5 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -40,7 +40,7 @@ > > * Helper macro to manipulate data structures > > */ > > #ifndef offsetof > > -#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER) > > +#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER) > > #endif > > #ifndef container_of > > #define container_of(ptr, type, member) \ > > -- > > 2.24.1 > >
On 8/11/20 5:08 AM, Yonghong Song wrote: > Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro > in bpf_helpers.h") added a macro offsetof() to get the offset of a > structure member: > #define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER) > > In certain use cases, size_t type may not be available so > Commit da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof > for offsetof") changed to use __builtin_offsetof which removed > the dependency on type size_t, which I suggested. > > But using __builtin_offsetof will prevent CO-RE relocation > generation in case that, e.g., TYPE is annotated with "preserve_access_info" > where a relocation is desirable in case the member offset is changed > in a different kernel version. So this patch reverted back to > the original macro but using "unsigned long" instead of "site_t". > > Cc: Ian Rogers <irogers@google.com> > Fixes: da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof for offsetof") > Signed-off-by: Yonghong Song <yhs@fb.com> Applied, thanks!
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index bc14db706b88..e9a4ecddb7a5 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -40,7 +40,7 @@ * Helper macro to manipulate data structures */ #ifndef offsetof -#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER) +#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER) #endif #ifndef container_of #define container_of(ptr, type, member) \
Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro in bpf_helpers.h") added a macro offsetof() to get the offset of a structure member: #define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER) In certain use cases, size_t type may not be available so Commit da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof for offsetof") changed to use __builtin_offsetof which removed the dependency on type size_t, which I suggested. But using __builtin_offsetof will prevent CO-RE relocation generation in case that, e.g., TYPE is annotated with "preserve_access_info" where a relocation is desirable in case the member offset is changed in a different kernel version. So this patch reverted back to the original macro but using "unsigned long" instead of "site_t". Cc: Ian Rogers <irogers@google.com> Fixes: da7a35062bcc ("libbpf bpf_helpers: Use __builtin_offsetof for offsetof") Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/bpf_helpers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)