This is an archive of the discontinued LLVM Phabricator instance.

[clang][BPF] support type exist/size and enum exist/value relocations
ClosedPublic

Authored by yonghong-song on Jul 6 2020, 9:54 AM.

Details

Summary

This patch added the following additional compile-once
run-everywhere (CO-RE) relocations:

  • existence/size of typedef, struct/union or enum type
  • enum value and enum value existence

These additional relocations will make CO-RE bpf programs more
adaptive for potential kernel internal data structure changes.

For existence/size relocations, the following two code patterns
are supported:

  1. __builtin_preserve_type_info(*(<type> *)0, flag);
  2. <type> var; __builtin_preserve_field_info(var, flag);

flag = 0 for existence relocation and flag = 1 for size relocation.

For enum value existence and enum value relocations, the following code
pattern is supported:

uint64_t __builtin_preserve_enum_value(*(<enum_type> *)<enum_value>,
                                       flag);

flag = 0 means existence relocation and flag = 1 for enum value.
relocation. In the above <enum_type> can be an enum type or
a typedef to enum type. The <enum_value> needs to be an enumerator
value from the same enum type. The return type is uint64_t to
permit potential 64bit enumerator values.

Diff Detail

Event Timeline

yonghong-song created this revision.Jul 6 2020, 9:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2020, 9:54 AM

Awesome, that's exactly what we need for BPF helper availability checks!

Can you please also add test that this pattern works:

return __builtin_preserve_field_info((btf_bpf_read_branch_records)0, FIELD_EXISTENCE);

Also for non-func pointer typedefs, something like this would work as well, right?

return __builtin_preserve_field_info(*(T *)0, FIELD_EXISTENCE);
fjricci added a subscriber: fjricci.Jul 6 2020, 6:16 PM
jrfastab added a subscriber: jrfastab.EditedJul 7 2020, 6:34 AM

Agreed also useful for us. I can pull it in and test if that is helpful.

Sounds good. Will try to add support for union/struct type as well.

yonghong-song retitled this revision from [RFC][BPF] support expr with typedef type for FIELD_EXISTENCE reloc to [clang][BPF] support expr with typedef/record type for FIELD_EXISTENCE reloc.
yonghong-song edited the summary of this revision. (Show Details)

clang change only and added tests

ast added a comment.Jul 15 2020, 12:32 PM

lgtm.
curious what happens when type is defined within args, like:
__builtin_preserve_field_info(*(struct { int a; } *)0, 2);

This is awesome. I'll apply patches locally and will play with them tonight to see if everything works in libbpf as well. Thanks!

In D83242#2154145, @ast wrote:

lgtm.
curious what happens when type is defined within args, like:
__builtin_preserve_field_info(*(struct { int a; } *)0, 2);

-bash-4.4$ clang -target bpf -O2 -g -S t1.c
t1.c:3:40: error: __builtin_preserve_field_info argument 1 invalid

return __builtin_preserve_field_info(*(struct { int a; } *)0, 2);

it will flag out an error since an non-empty name is required
as we will need it to generate relocation.

if (!RT || RT->getDecl()->getDeclName().isEmpty())

With an name, it works properly.

-bash-4.4$ cat t1.c

unsigned unit1() {
  return __builtin_preserve_field_info(*(struct t { int a; } *)0, 2);
}
-bash-4.4$ clang -target bpf -O2 -g -S t1.c                                                        
-bash-4.4$

fix clang-format warnings.

After thinking about this a bit more, I think adding a new TYPE_EXISTENCE (instead of FIELD_EXISTENCE) relocation type is the better way to handle this. It would allow Clang to be stricter as to what is passed into FIELD_EXISTENCE (only fields, not types) vs TYPE_EXISTENCE(only types, no fields). It will make it cleaner on libbpf side as well, because the validation and relocation logic is different between handling fields and types. So overall, I think it will be beneficial on every level of the stack to separate them. I hope it's not too hard to make this change?

After thinking about this a bit more, I think adding a new TYPE_EXISTENCE (instead of FIELD_EXISTENCE) relocation type is the better way to handle this. It would allow Clang to be stricter as to what is passed into FIELD_EXISTENCE (only fields, not types) vs TYPE_EXISTENCE(only types, no fields). It will make it cleaner on libbpf side as well, because the validation and relocation logic is different between handling fields and types. So overall, I think it will be beneficial on every level of the stack to separate them. I hope it's not too hard to make this change?

Yes. Sounds better to separate them as they handle disjoint patterns. Will make the change.

yonghong-song retitled this revision from [clang][BPF] support expr with typedef/record type for FIELD_EXISTENCE reloc to [clang][BPF] support expr with typedef/record type for TYPE_EXISTENCE reloc.
yonghong-song edited the summary of this revision. (Show Details)

use a different reloc type TYPE_EXISTENCE (defined in llvm/lib/Target/BPF/BPFCORE.h) instead of old FIELD_EXISTENCE. This will simplify libbpf processing.

@anakryiko Please rebase on top of latest clang. There is a recent core change which touches BPF code which I am also modifying.

yonghong-song retitled this revision from [clang][BPF] support expr with typedef/record type for TYPE_EXISTENCE reloc to [clang][BPF] support type existence/size and enum value relocations.
yonghong-song edited the summary of this revision. (Show Details)

support relocations for type_sizeof and enum_value as well.

One potential missing relocation is to test whether a particular enum value (from an enum type) exists or not.

One way is to create a new relocation type ENUM_VALUE_EXISTENCE where "access_string" to encode the enum value. This is different from TYPE_EXISTENCE for enum type where the "access_string" is "0".

Alternatively, we could have TYPE_EXISTENCE to include enum value existence as well. "access_string" is say "0" for typedef/record/enum types and "0:<enum_value>" for enum value.

WDYT?

yonghong-song retitled this revision from [clang][BPF] support type existence/size and enum value relocations to [clang][BPF] support type exist/size and enum exist/value relocations.
yonghong-song edited the summary of this revision. (Show Details)

use a different __builtin function for new relocations to they are easily discoverable in user space and this also make implementation more structured.

anakryiko accepted this revision.Jul 30 2020, 12:17 PM

looks great, I'll complete libbpf support for all these ASAP

This revision is now accepted and ready to land.Jul 30 2020, 12:17 PM
yonghong-song edited the summary of this revision. (Show Details)

enum value relocation change: use ld_imm64 so we support 64bit enum value from day one. put enumerator name as the AccessString.

anakryiko accepted this revision.Aug 3 2020, 4:29 PM

LGTM. One question: why didn't we run into the need for SeqNumVal trick with field-based relocations? We seem to need it for all other types (including type ID-based), but not for field-based?

LGTM. One question: why didn't we run into the need for SeqNumVal trick with field-based relocations? We seem to need it for all other types (including type ID-based), but not for field-based?

good question. In general CSE can only happen if
the base address is the same, for a->b->c->d and a->b->c, portion of address calculation may be shared (CSE'ed).
If the base address is different (e.g., as different variable), CSE cannot happen since the base address is different.

The CSE with different types can happen if the base address is nullptr. This may happen when you try to test
whether a member exists or not. For example:

-bash-4.4$ cat t.c
struct t { int a; int b; };
typedef struct t t;
int test(struct t *arg1,
t *arg2) {

return __builtin_preserve_field_info(((struct t *)0)->b, 1) +
       __builtin_preserve_field_info(((__t *)0)->b, 1);

}
-bash-4.4$

Although clang generates two IR builtins tagged with "struct t" and "t" debuginfo type respectively, the IR type for the first argument base is the same as "struct t" as typedef is ignored at IR level. So in this case, two builtin_preserve_field_info will be CSE'ed to one. I think this is okay, we are trying to test whether a field is available, the correct result is still returned.

This is different than testing whether a type exists or not where we want to precisely test whether that type exists or not, so we want to preserve typedef type vs. struct t type.

This revision was landed with ongoing or failed builds.Aug 4 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.

This is different than testing whether a type exists or not where we want to precisely test whether that type exists or not, so we want to preserve typedef type vs. struct t type.

typedef t and struct t are also different for field relocations (those t is in different namespaces, actually), so it might be a good idea to disambiguate those for field relocations as well?

This is different than testing whether a type exists or not where we want to precisely test whether that type exists or not, so we want to preserve typedef type vs. struct t type.

typedef t and struct t are also different for field relocations (those t is in different namespaces, actually), so it might be a good idea to disambiguate those for field relocations as well?

Just to be clear. The type recorded in relocation will be always valid since we have type_id there. For field exists, due to we may use nullptr as the object, if CSE happens, it is possible the root type id may be the "struct t" instead of "typedef t" (or the other ordering) assuming the definition "typedef struct t t;". But from root type_id, libbpf will be able to correctly trace down to the field and do the relocation properly.

On the other hand, I agree that this is not ideal as we may use a different root type than what user specified in the source. But everything is hidden and invisible to user so I won't have user visible impact.

To fix this in clang is actually tricky, we do not want to add seq number to every preserve_field_info* IR builtin since this is not really necessary for non-existence relocations. Also preserve_field_info* although used by bpf only so far is llvm generic builtin, I would hesitate to change it with new seq_num fields. The best will be have another bpf specific builtin
for field existence, but given the current mechanism also works, I probably will keep it this way.

This is different than testing whether a type exists or not where we want to precisely test whether that type exists or not, so we want to preserve typedef type vs. struct t type.

typedef t and struct t are also different for field relocations (those t is in different namespaces, actually), so it might be a good idea to disambiguate those for field relocations as well?

Just to be clear. The type recorded in relocation will be always valid since we have type_id there. For field exists, due to we may use nullptr as the object, if CSE happens, it is possible the root type id may be the "struct t" instead of "typedef t" (or the other ordering) assuming the definition "typedef struct t t;". But from root type_id, libbpf will be able to correctly trace down to the field and do the relocation properly.

On the other hand, I agree that this is not ideal as we may use a different root type than what user specified in the source. But everything is hidden and invisible to user so I won't have user visible impact.

Hm.. This root type is used by libbpf to find candidate in kernel BTF. Name and kind of that type is what is used. When you have typedef t and struct t, their names match, but their kinds don't. So it could lead to inability to find a candidate.

To fix this in clang is actually tricky, we do not want to add seq number to every preserve_field_info* IR builtin since this is not really necessary for non-existence relocations. Also preserve_field_info* although used by bpf only so far is llvm generic builtin, I would hesitate to change it with new seq_num fields. The best will be have another bpf specific builtin
for field existence, but given the current mechanism also works, I probably will keep it this way.

A new built-in is definitely a big hassle. It hasn't been a problem so far and the possibility is really slim. If it's a problem to do this for existing built-in, let's leave it as is for now. Just something to keep in mind.

Hm.. This root type is used by libbpf to find candidate in kernel BTF. Name and kind of that type is what is used. When you have typedef t and struct t, their names match, but their kinds don't. So it could lead to inability to find a candidate.

The inproper CSE can only happen if the declaration in user program like struct t { ... }; typedef struct t t; and user try to find existence of both struct t and typedef t. We could have issue if kernel does not have struct t { ...} and only has typedef t. Theoretically this could happen, but as you mentioned the chance is really slim. So will not change current implementation right now.