On Tue, Aug 27, 2024 at 10:37 AM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote: > > On Tue, Aug 27, 2024 at 12:09:07AM -0400, Will Hawkins wrote: > > Thank you for the feedback on my first attempt. I believe that this > > attempt is better. Given the feedback, I > > > > 0. Removed the incorrect use of linkage_name field; > > 1. Tried to maintain the semantics of DWARF as much as possible; > > 2. Reworked how BTF information is loaded into a new function based on > > my understanding of the semantics of BTF_FUNC_STATIC, BTF_FUNC_GLOBAL, > > and BTF_FUNC_EXTERN; and > > 3. Modified (slightly) the way functions are printed in pfunc to show > > linkage where appropriate (based, in particular, on > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45153#c6. > > > > I saw that Arnaldo and Namhyung have made similar changes to the > > printing code so I hope that I am not stepping on their work (sorry if I > > am!). > > > > Again, I appreciate the feedback and hope that I was able to address it > > with this v2. > > So, I broke it down into two patches, the first for the BTF loader and > then a second patch for pfunct to use that info, but: > > ⬢[acme@toolbox pahole]$ readelf -wi cases/btf_linkages/linkage.o > Contents of the .debug_info section: > > Compilation Unit @ offset 0: > Length: 0x4e (32-bit) > Version: 5 > Unit Type: DW_UT_compile (1) > Abbrev Offset: 0 > Pointer Size: 8 > <0><c>: Abbrev Number: 1 (DW_TAG_compile_unit) > <d> DW_AT_producer : (indirect string, offset: 0x5): GNU C17 14.1.1 20240701 (Red Hat 14.1.1-7) -mtune=generic -march=x86-64 -gbtf -g -O0 > <11> DW_AT_language : 29 (C11) > <12> DW_AT_name : (indirect line string, offset: 0): linkage.c > <16> DW_AT_comp_dir : (indirect line string, offset: 0xa): /home/acme/git/pahole/cases/btf_linkages > <1a> DW_AT_low_pc : 0 > <22> DW_AT_high_pc : 0xb > <2a> DW_AT_stmt_list : 0 > <1><2e>: Abbrev Number: 2 (DW_TAG_subprogram) > <2f> DW_AT_name : x > <31> DW_AT_decl_file : 1 > <32> DW_AT_decl_line : 6 > <33> DW_AT_decl_column : 12 > <34> DW_AT_type : <0x4a> > <38> DW_AT_low_pc : 0 > <40> DW_AT_high_pc : 0xb > <48> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa) > <4a> DW_AT_call_all_calls: 1 > <1><4a>: Abbrev Number: 3 (DW_TAG_base_type) > <4b> DW_AT_byte_size : 4 > <4c> DW_AT_encoding : 5 (signed) > <4d> DW_AT_name : int > <1><51>: Abbrev Number: 0 > > ⬢[acme@toolbox pahole]$ > > In DWARF we don't have that DW_AT_external, so the simplified second > patch: > > ⬢[acme@toolbox pahole]$ git diff > diff --git a/pfunct.c b/pfunct.c > index bc3026b384284cb4..9cf61713354d178e 100644 > --- a/pfunct.c > +++ b/pfunct.c > @@ -377,6 +377,9 @@ static void function__show(struct function *func, struct cu *cu) > if (fstats && fstats->printed) > return; > > + if (!func->external) > + fprintf(stdout, "static "); > + > if (expand_types) > function__emit_type_definitions(func, cu, stdout); > tag__fprintf(tag, cu, &conf, stdout); > ⬢[acme@toolbox pahole]$ > > Would print as static, but it isn't... > > ⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c > // https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@xxxxxx/T/#u > // Will Hawkins > // $ gcc -gbtf -g -O0 -c linkage.c > // $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o > > static int x() { > return 5; > } > ⬢[acme@toolbox pahole]$ > > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o > int x(void); > ⬢[acme@toolbox pahole]$ > Sorry for the trouble! I *thought* that I tested cases like these and got results that seemed reasonable. Clearly what you got does not seem to work. So, I will go through and test using a few different cases, make sure that I get the right answer, then confirm with you in a follow-up patch. (See below) > > So, if instead I do it at: > > ⬢[acme@toolbox pahole]$ git diff > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > index ffbe6fbd3f5c405a..9a8bb5f15c88125a 100644 > --- a/dwarves_fprintf.c > +++ b/dwarves_fprintf.c > @@ -1406,6 +1406,9 @@ static size_t function__fprintf(const struct tag *tag, const struct cu *cu, > size_t printed = 0; > bool inlined = !conf->strip_inline && function__declared_inline(func); > > + if (!func->external) > + printed += fprintf(fp, "%s ", "static"); > + > if (tag->attribute) > printed += fprintf(fp, "%s ", tag->attribute); > > ⬢[acme@toolbox pahole]$ > > Then we seem to get what we want: > > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o > static int x(void); > ⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o > static int x(void) > { > return 0; > } > > ⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o > static int x(void); > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o > static int x(void); > ⬢[acme@toolbox pahole]$ > > And if we add a non-static function, it also works for both DWARF and > BTF when generated by gcc: > > ⬢[acme@toolbox btf_linkages]$ gcc -gbtf -g -O0 -c linkage.c > ⬢[acme@toolbox btf_linkages]$ cd - > /home/acme/git/pahole > ⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c > // https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@xxxxxx/T/#u > // Will Hawkins > // $ gcc -gbtf -g -O0 -c linkage.c > // $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o > > static int x() { > return 5; > } > > int y() { > return 4; > } > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o > static int x(void); > int y(void); > ⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o > static int x(void); > int y(void); > ⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o > int y(void) > { > return 0; > } > > static int x(void) > { > return 0; > } > > ⬢[acme@toolbox pahole]$ pfunct --compile -F dwarf cases/btf_linkages/linkage.o > int y(void) > { > } > > static int x(void) > { > } > > ⬢[acme@toolbox pahole]$ > > But, for a vmlinux I have laying around it seems we're not encoding that > with pahole: > > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | grep ^static | head > static void clean_cache_range(void * addr, size_t size); > static void arch_wb_cache_pmem(void * addr, size_t size); > static long int __copy_user_flushcache(void * dst, const void * src, unsigned int size); > static void __memcpy_flushcache(void * _dst, const void * _src, size_t size); > static long unsigned int copy_from_user_nmi(void * to, const void * from, long unsigned int n); > static void call_depth_return_thunk(void); > static void entry_untrain_ret(void); > static void retbleed_untrain_ret(void); > static void srso_untrain_ret(void); > static void srso_alias_safe_ret(void); > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | grep ^static | wc -l > 59015 > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | wc -l > 59015 > ⬢[acme@toolbox pahole]$ > > ⬢[acme@toolbox pahole]$ pahole --btf_encode -j vmlinux > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | wc -l > 59650 > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | grep ^static | wc -l > 59650 > ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux | grep -v ^static > ⬢[acme@toolbox pahole]$ > > So before we change function__fprintf() we need to investigate this > other part: pahole seemingly not encoding: > > + func->declaration = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN; > + func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_GLOBAL; > > The thing is we want to release 1.28, so lets leave this for after that, > ok. Sorry -- it took me a few reads through your thorough email to understand. I appreciate your willingness to engage with me on this patch! If I understand correctly, you are saying that pahole may not be encoding the BTF_FUNC* properly. So, we will need to debug that at the same time as we add this feature. If that is the case, then I will start to work on that! If possible, would you be willing to send around that "bad" vmlinux? (You can send directly to me -- I know that they are far from small). That would help me debug and recreate what you are seeing. Thank you again for engaging with me on this effort! I really, really appreciate your openness! As I said, I am a huge fan of your work (I really enjoyed your talk at DevConf in 2021)! Will > > - Arnaldo
