[PATCH] D132144: [Clang][BPF] Support record argument with direct values

anakryiko added a comment. Are there any restrictions about number of struct arguments that can be passed in? Kernel changes were assuming at most 2, should we have a test that tests passing 3 structs that fit into 5 input registers and another test that passes 3 structs that do not fit in 5

[PATCH] D131012: No longer place const volatile global variables in a read only section

anakryiko added a comment. In D131012#3697000 , @aaron.ballman wrote: > In D131012#3696810 , @anakryiko > wrote: > >> In D131012#3696327 , >> @aaron.ballman wrote: >>

[PATCH] D131012: No longer place const volatile global variables in a read only section

anakryiko added a comment. In D131012#3696327 , @aaron.ballman wrote: > In D131012#3695110 , @anakryiko > wrote: > >> This will severly break BPF users, as we have a heavy reliance on `const >> volatile`

[PATCH] D131012: No longer place const volatile global variables in a read only section

anakryiko added a comment. This will severly break BPF users, as we have a heavy reliance on `const volatile` variables being allocated to `.rodata`, which are passed to BPF verifier in Linux kernel as read-only values. This is critical for BPF Complie Once - Run Everywhere approach of writing

[PATCH] D111588: [Clang][Attr] rename btf_tag to btf_decl_tag

anakryiko accepted this revision. anakryiko added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111588/new/ https://reviews.llvm.org/D111588

[PATCH] D106614: [Clang] add btf_tag attribute

anakryiko added a comment. I think it's reasonable behavior to just merge all bpf_tags across declaration(s) and a definition. Kind of like __weak behaves, it doesn't matter if it's on actual function or its extern declaration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D106614: [Clang] add btf_tag attribute

anakryiko added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2019 +targets. This attribute may be attached to a struct/union, struct/union field, +function or variables declaration. If -g is specified, the ARGUMENT info will +be preserved in IR and be

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

anakryiko accepted this revision. anakryiko added a comment. Nice, thanks! This will work for externs with explicit section name (.ksym) and with no section name (externs for static linking), right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91489: BPF: make __builtin_btf_type_id() return 64bit int

anakryiko added inline comments. Comment at: llvm/lib/Target/BPF/BTFDebug.cpp:1230 +Reloc == BPFCoreSharedInfo::BTF_TYPE_ID_REMOTE) OutMI.setOpcode(BPF::LD_imm64); else yonghong-song wrote: > ast wrote: > > libbpf would need to

[PATCH] D83242: [clang][BPF] support type exist/size and enum exist/value relocations

anakryiko added a comment. In D83242#2195456 , @yonghong-song wrote: > In D83242#2193989 , @anakryiko wrote: > >>> This is different than testing whether a type exists or not where we want >>> to precisely test

[PATCH] D83242: [clang][BPF] support type exist/size and enum exist/value relocations

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

[PATCH] D85174: BPF: simplify IR generation for __builtin_btf_type_id()

anakryiko accepted this revision. anakryiko added a comment. This revision is now accepted and ready to land. Tested locally. Previously failing tests are now passing. All recorded relocations look correct. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D83242: [clang][BPF] support type exist/size and enum exist/value relocations

anakryiko accepted this revision. anakryiko added a comment. 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? Repository: rG LLVM Github Monorepo

[PATCH] D83242: [clang][BPF] support type exist/size and enum exist/value relocations

anakryiko accepted this revision. anakryiko added a comment. This revision is now accepted and ready to land. looks great, I'll complete libbpf support for all these ASAP Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83242/new/

[PATCH] D83242: [clang][BPF] support expr with typedef/record type for FIELD_EXISTENCE reloc

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

[PATCH] D83242: [clang][BPF] support expr with typedef/record type for FIELD_EXISTENCE reloc

anakryiko added a comment. This is awesome. I'll apply patches locally and will play with them tonight to see if everything works in libbpf as well. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83242/new/ https://reviews.llvm.org/D83242

[PATCH] D83242: [RFC][BPF] support expr with typedef type for FIELD_EXISTENCE reloc

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

[PATCH] D81479: [BPF] introduce __builtin_load_u32_to_ptr() intrinsic

anakryiko added a comment. IntrinsicsBPF.td uses `__builtin_bpf_load_u32_to_ptr`, while everywhere it is just `__builtin_load_u32_to_ptr`. I think having bpf prefix there is good, given this is bpf-specific. But either way, probably should be consistent everywhere? Repository: rG LLVM

[PATCH] D81131: [DebugInfo] Fix assertion for extern void type

anakryiko accepted this revision. anakryiko added a comment. This revision is now accepted and ready to land. For my purposes, having extern var BTF only for `extern const void` is just fine. Not crashing Clang for `extern void` is great as well :) Repository: rG LLVM Github Monorepo

[PATCH] D74668: [Clang][BPF] implement __builtin_btf_type_id() builtin function

anakryiko added a comment. In D74668#2020554 , @yonghong-song wrote: > In D74668#2019558 , @anakryiko wrote: > > > what's the use case for flag==0 (no relocation)? why using built-in at all > > in such case? Also

[PATCH] D74668: [Clang][BPF] implement __builtin_btf_type_id() builtin function

anakryiko added a comment. what's the use case for flag==0 (no relocation)? why using built-in at all in such case? Also flag==1 means relocate to local BTF ID or remote (kernel) BTF ID? Do you plan to add flag=2 as well to cover both cases? Or am I misunderstanding the meaning of this flag?

[PATCH] D74668: [Clang][BPF] implement __builtin_btf_type_id() builtin function

anakryiko added a comment. Let's extend __builtin_btf_type_id() to accept second argument specifying whether it's local BTF ID (from program's BTF) or target BTF ID (from kernel/module BTF)? We can probably make it an enum just like with preserve_access_index() built-in, for easy future

[PATCH] D73900: [BPF] use base lvalue type for preserve_{struct,union}_access_index metadata

anakryiko accepted this revision. anakryiko added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73900/new/ https://reviews.llvm.org/D73900

[PATCH] D67980: [WIP][CLANG][BPF] do compile-once run-everywhere relocation for bitfields

anakryiko added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9321-9322 + if (IsError) +return IsBitField ? EmitLValue(Arg).getBitFieldPointer() + : EmitLValue(Arg).getPointer(); + move this under `if (!getDebugInfo)`