This is an archive of the discontinued LLVM Phabricator instance.

[BPF] Add preserve_access_index attribute for record definition
ClosedPublic

Authored by yonghong-song on Nov 2 2019, 8:41 PM.

Details

Summary

This patch introduced a new bpf specific attribute which can
be added to struct or union definition. For example,

struct s { ... } __attribute__((preserve_access_index));
union u { ... } __attribute__((preserve_access_index));

The goal is to simplify user codes for cases
where preserve access index happens for certain struct/union,
so user does not need to use clang __builtin_preserve_access_index
for every members.

The attribute has no effect if -g is not specified.

When the attribute is specified and -g is specified, any member
access defined by that structure or union, including array subscript
access and inner records, will be preserved through

__builtin_preserve_{array,struct,union}_access_index()

IR intrinsics, which will enable relocation generation
in bpf backend.

The following is an example to illustrate the usage:

-bash-4.4$ cat t.c
#define __reloc__ __attribute__((preserve_access_index))
struct s1 {
  int c;
} __reloc__;

struct s2 {
  union {
    struct s1 b[3];
  };
} __reloc__;

struct s3 {
  struct s2 a;
} __reloc__;

int test(struct s3 *arg) {
  return arg->a.b[2].c;
}
-bash-4.4$ clang -target bpf -g -S -O2 t.c

A relocation with access string "0:0:0:0:2:0" will be generated
representing access offset of arg->a.b[2].c.

forward declaration with attribute is also handled properly such
that the attribute is copied and populated in real record definition.

Diff Detail

Event Timeline

yonghong-song created this revision.Nov 2 2019, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2019, 8:41 PM
yonghong-song added a comment.EditedNov 2 2019, 8:42 PM

Discussed with Martin last Friday that it would be easier for CO-RE programming if we can annotate the structure itself. This is the RFC patch. Please let me whether this sounds a reasonable approach.

yonghong-song retitled this revision from [RFC][BPF] Add preserve_access_index attribute to record definition to [BPF] Add preserve_access_index attribute for record definition.
yonghong-song edited the summary of this revision. (Show Details)

handling inner records and array subscript accesses. removing RFC tag.

ast added a comment.Nov 8 2019, 9:49 AM

Looks great. This is a big improvement in usability.
Is there a use case to apply that attribute to inner types automatically ?

struct s1 {
  int foo;
};
struct s2 {
   struct s1 *ptr;
} __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?

yonghong-song added a comment.EditedNov 8 2019, 9:57 AM

For the below:

struct s1 {
 int foo;
};
struct s2 {
 struct s1 *ptr;
} __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?
Only s2->ptr is relocated.

ast added a comment.Nov 8 2019, 10:06 AM

Is the attribute sticky with forward delcarations?

struct s __reloc;

struct s {
  int foo;
} *s;

what is s->foo ?

For question

Is there a use case to apply that attribute to inner types automatically ?

It is possible, but I feel make attribute per record is better. For example,

struct s1 {
 int foo;
}; 
struct s2 {
 struct s1 *ptr;
} __reloc__ *s2;

If we implicitly mark struct s1 as reloc, later user may have
another code like

struct s1 *p;
p->foo

he may be surprised by relocation.
If this is user intention to have relocation for struct s1,
I think explicit attribute is better.

Another question would be during IR generation can we magically generate relocation for ptr->foo in s2->ptr->foo? Without marking the record, this will require pass context sensitive information along code generation, which could be error prone as when to clear such context sensitive information. I would prefer explicit marking. If user uses vmlinux.h, all struct/unions in vmlinux.h could have this attribute. If user defines their own structures for later relocation, I expect this should not be a huge amount and some manual work by users should be okay.

yonghong-song added a comment.EditedNov 8 2019, 10:21 AM

Is the attribute sticky with forward delcarations?

My previous comment is incorrect. It does look forward declaration can have attribution and it is sticky.

#define __reloc__ __attribute__((preserve_access_index))
struct __reloc__ p;
struct p {
 int a;
};

int test(struct p *arg) {
  return arg->a;
}
-bash-4.4$ clang -target bpf -S -O2 -emit-llvm -g t.c
-bash-4.4$ cat t.ll
...
  %0 = tail call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.ps(%struct.p*  %arg, i32 0, i32 0), !dbg !18, !llvm.pr
...

But it does not work for inner structs any more.
-bash-4.4$ cat t.c
#define reloc attribute((preserve_access_index))
struct reloc p;
struct p {

int a;
struct {
 int b;
};

};

int test(struct p *arg) {

return arg->b;

}
-bash-4.4$

In only generates reloc for struct p's immediate members, but not inner structures.
The forward declaration gives empty record with attribute set, but this set does not
extend to inner record.

ast added a comment.Nov 8 2019, 10:53 AM

So what happens in the following case:

struct S1;
struct S2 {
  struct S1 *f;
} relo *s2;
// access s2->f
struct S1 {
  int i;
} relo;
// access s2->f->i

lack of relo on fwd declaration is not going to be sticky 'non-relo' when it's seen later, right?
So all 3 deref will be relocatable?

Yes, in the above, all three of them will be relocatable.

-bash-4.4$ cat t2.c
#define __reloc__ __attribute__((preserve_access_index))
 struct S1;
 struct S2 {
  struct S1 *f;
 } __reloc__;
 struct S1 {
    int i;
 } __reloc__;
 // access s2->f
 // access s2->f->i
 int test(struct S2 *arg) {
   return arg->f->i;
 }
-bash-4.4$ clang -target bpf -S -O2 -emit-llvm -g t2.c
...
  %0 = tail call %struct.S1** @llvm.preserve.struct.access.index.p0p0s_struct.S1s.p0s_struct.S2s(%struct.S2* %arg, i32 0, i32 0
), !dbg !22, !llvm.preserve.access.index !12
  %1 = load %struct.S1*, %struct.S1** %0, align 8, !dbg !22, !tbaa !23
  %2 = tail call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.S1s(%struct.S1* %1, i32 0, i32 0), !dbg !28, !llvm.pr
eserve.access.index !16

In clang, BPF routines will see the record definition if it has the attribute. So in the above, the actual definition of
struct S1 and S2 will be seen and marked with attributes properly.

Let me take a look whether this is a *relatively easy way* to support sticky forward declaration (w.r.t. applying to inner structures).

yonghong-song edited the summary of this revision. (Show Details)

handle forward declaration correctly, i.e., not losing its attribute/action.

ast accepted this revision.Nov 8 2019, 6:34 PM

Great. Looks like inner propagation fix was straighforward. Thanks!

This revision is now accepted and ready to land.Nov 8 2019, 6:34 PM
This revision was automatically updated to reflect the committed changes.

This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc.

clang/include/clang/Basic/DiagnosticSemaKinds.td
10068 ↗(On Diff #228576)

This is unnecessary.

clang/lib/Sema/SemaDeclAttr.cpp
5706

Any reason not to use a range-based for?

5712

Can use auto here

5713–5718

These should be implicit attributes, not explicit ones, right?

5725

Similar here.

5727

Can use auto here as well.

5730–5736

Implicit attributes?

5742–5747

This should be handled in Attr.td with an explicit subject.

5749

This is not needed.

Thanks, @aaron.ballman, I will fix the issue and address all you comments. I will re-open this commit.

yonghong-song reopened this revision.Nov 9 2019, 8:51 AM
This revision is now accepted and ready to land.Nov 9 2019, 8:51 AM

The Decl "D" could be a nullptr in ProcessDeclAttributeDelayed, which will cause segfault. Handle this properly.
Also addressed most of Aaron's comments.

yonghong-song marked 6 inline comments as done.Nov 9 2019, 9:56 AM

use implicit attr to annotate records or fields not explicitly marked by users.

yonghong-song marked 2 inline comments as done.Nov 9 2019, 10:50 AM

Hi, @aaron.ballman I think I addressed all your comments, could you take a look again? I tested all failed tests as exposed by the buildbot here (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/57856). They are all passing now. Thanks!

Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
(a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
template instantiation since we are at clang stage?

ast added a comment.Nov 9 2019, 11:22 AM

I've tested this patch on several kernel selftests/bpf/ and it works like magic. Very nice improvement.

@aaron.ballman BTW, I indeed tested C-style inheritance. An attribute for the forward declaration successfully inherited by a later actual declaration.

@aaron.ballman just ping, could you let me know if you have any further comments? Thanks!

This revision was automatically updated to reflect the committed changes.

Why did this get committed when there were still outstanding review questions that were not answered?

Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
(a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
template instantiation since we are at clang stage?

If the attribute only makes sense in C mode, it should not be accepted in C++ mode. However, it currently is accepted in C++ mode which is why I was asking for tests demonstrating how it should behave.

@aaron.ballman just ping, could you let me know if you have any further comments? Thanks!

I was out all last week at wg21 meetings and a short vacation, so that's why the delay in review. FWIW, we usually give a week before pinging a review.

clang/include/clang/Basic/Attr.td
1584

If this attribute is only supported in C mode, then this attribute should have: let LangOpts = [COnly]; in it.

clang/lib/CodeGen/CGExpr.cpp
3411–3422

Is there a reason to do this as opposed to IgnoreImpCasts()?

3423–3424
if (const auto *ME = dyn_cast<MemberExpr>(E))
  return ...
3427–3428

Similar here -- you can sink the variable into the if.

3433

const auto *PtrT = VarDef->getType()->getAs<PointerType>();

3440–3443

Is there a reason you only want to strip off typedefs, and not other forms of sugar? e.g., why not call getUnqualifiedDesugaredType()?

3446

const auto *

yonghong-song marked 7 inline comments as done.Nov 14 2019, 11:04 AM

Sorry about the committing. I thought all the review questions are addressed.
Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.
Thanks for the detailed review! I have addressed all of your review comments in patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.

clang/include/clang/Basic/Attr.td
1584

Yes, added.

clang/lib/CodeGen/CGExpr.cpp
3440–3443

Actually, ElaboratedT is also a sugar type, the above getUnqualifiedDesugaredType() will remove it too.

Sorry about the committing. I thought all the review questions are addressed.

No worries, this happens sometimes. :-)

Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.

Thanks!

Thanks for the detailed review! I have addressed all of your review comments in patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.

Fantastic, thank you!

clang/lib/CodeGen/CGExpr.cpp
3440–3443

Yes, but you just strip off that sugar anyway, so that's why I suggested this. It seems you got the gist of my suggestion in the other review though.