Re: [PATCH v3 03/12] perf annotate-data: Resolve DW_OP_addrx global variables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf annotate-data: DW_OP_addrx fallback in dwarf-aux.c
  breaks annotate-data.c build
- [Low] perf annotate-data: typo in Fixes trailer SHA d7d60803a790b716
  in commit message
--

commit c0595ea7c114bb34a2fadcf5632812fb611fb7b5
Author: Arnaldo Carvalho de Melo <acme@kernel.org>

> perf annotate-data: Resolve DW_OP_addrx global variables
>
> clang emits DW_AT_location with DW_OP_addrx for global variables in
> DWARF 5, gcc emits DW_OP_addr.  The address index was not resolved, so
> those variables were not found by address; the by-name fallback also
> missed the ones in .bss past the file backed end of the last segment.
>
> Resolve the index with dwarf_getlocation_attr() + dwarf_formaddr() and
> use the resulting address in the by-address lookup and in the global
> variable cache.  The new global_data workload keeps a zero initialized
> struct in .bss and its test checks that the accesses are annotated: when
> perf is built with clang and DWARF 5 it goes through DW_OP_addrx, with
> gcc through DW_OP_addr.  The test skips on 32-bit PIE builds, which
> load globals through the GOT, something the resolver does not follow.
>
> Fixes: d60469d7c0e5c4e8 ("perf dwarf-aux: Add die_find_variable_by_addr()")
> Fixes: d7d60803a790b716 ("perf dwarf-aux: Add die_collect_global_vars()")
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

[Severity: Low]
The second Fixes: tag has a typo in the SHA (d7d60803a790b716 cannot be
resolved); the commit that introduced die_collect_global_vars() is
d7b60803a790 ("perf dwarf-aux: Add die_collect_global_vars()").

[ ... ]

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index dd9c1d7b2da8b..d8d00646ef63b 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -140,6 +140,10 @@ static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
>  		case DW_OP_addr:
>  			pr_info("address=%#lx\n", (long)ops->number);
>  			break;
> +		case DW_OP_addrx:
> +		case DW_OP_GNU_addr_index:
> +			pr_info("address index=%#lx\n", (long)ops->number);
> +			break;
>  		default:
>  			pr_info("unknown: code=%#x, number=%#lx\n",
>  				ops->atom, (long)ops->number);
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 48a11c95a6768..5db7458f01116 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -14,6 +14,11 @@
>  #include "strbuf.h"
>  #include "string2.h"
>  
> +/* DWARF 5 address index, libdw's dwarf_getlocations() returns it unresolved */
> +#ifndef DW_OP_addrx
> +#define DW_OP_addrx 0xa1
> +#endif
> +

[Severity: Medium]
Does defining the DW_OP_addrx fallback only in dwarf-aux.c break the
build of annotate-data.c against older elfutils headers?

The libdw feature check accepts elfutils starting from 0.160, while
DW_OP_addrx was added to dwarf.h in 0.171.  Moving the #ifndef DW_OP_addrx
definition to tools/perf/util/dwarf-aux.h would make it visible to both
translation units.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115014102.2879372-1-acme@kernel.org?part=3