[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

aaron.ballman added a comment. In D69759#1746192 , @yonghong-song wrote: > 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 r

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song marked 7 inline comments as done. yonghong-song added a comment. 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

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

aaron.ballman added a comment. Why did this get committed when there were still outstanding review questions that were not answered? In D69759#1739935 , @yonghong-song wrote: > Regarding to this one "This is missing a bunch of C++ tests that show what

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

This revision was automatically updated to reflect the committed changes. Closed by commit rG4e2ce228ae79: [BPF] Add preserve_access_index attribute for record definition (authored by yonghong-song). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6975

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. @aaron.ballman just ping, could you let me know if you have any further comments? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759 __

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. @aaron.ballman BTW, I indeed tested C-style inheritance. An attribute for the forward declaration successfully inherited by a later actual declaration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https:/

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. I've tested this patch on several kernel selftests/bpf/ and it works like magic. Very nice improvement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759 __

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. 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 suppor

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song marked 2 inline comments as done. yonghong-song added a comment. 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

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song updated this revision to Diff 228588. yonghong-song added a comment. use implicit attr to annotate records or fields not explicitly marked by users. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song updated this revision to Diff 228583. yonghong-song added a comment. The Decl "D" could be a nullptr in ProcessDeclAttributeDelayed, which will cause segfault. Handle this properly. Also addressed most of Aaron's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. Thanks, @aaron.ballman, I will fix the issue and address all you comments. I will re-open this commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759 _

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

aaron.ballman added a comment. This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10068 "__builtin_preserve_field_info argument %0 not a constant"

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

This revision was automatically updated to reflect the committed changes. Closed by commit rG4a5aa1a7bf8b: [BPF] Add preserve_access_index attribute for record definition (authored by yonghong-song). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6975

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. Great. Looks like inner propagation fix was straighforward. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song updated this revision to Diff 228538. yonghong-song edited the summary of this revision. yonghong-song added a comment. handle forward declaration correctly, i.e., not losing its attribute/action. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. Let me take a look whether this is a *relatively easy way* to support sticky forward declaration (w.r.t. applying to inner structures). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.or

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. 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

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. 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 a

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. 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 onl

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. 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 impl

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. > Is the attribute sticky with forward delcarations? forward declaration is not a record type, so an error will be emited if you have attribute on a forward declaration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. Is the attribute sticky with forward delcarations? struct s __reloc; struct s { int foo; } *s; what is s->foo ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. 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?

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

yonghong-song updated this revision to Diff 228474. 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. yonghong-son