On 08/11/2024 18:05, Yonghong Song wrote:
> The dwarf location checking part of function parameter__new() is refactored
> to another function. The current dwarf location checking only for
> the first expression of the first location. One later patch may traverse
> all expressions of all locations, so refactoring makes later change easier.
>
> No functional change.
>
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> ---
> dwarf_loader.c | 77 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index ec8641b..4bb9096 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1157,16 +1157,56 @@ static struct template_parameter_pack *template_parameter_pack__new(Dwarf_Die *d
> return pack;
> }
>
> +static bool check_dwarf_locations(Dwarf_Attribute *attr, struct parameter *parm,
> + struct cu *cu, int param_idx)
Nit: I think this should be renamed to something like
parameter__locations() in keeping with existing naming scheme.
Could also include the parm->has_loc assignment there instead
of keeping it in paramter__new().
> +{
> + Dwarf_Addr base, start, end;
> + struct location loc;
> +
> + /* dwarf_getlocations() handles location lists; here we are
> + * only interested in the first expr.
> + */
> + if (parm->has_loc &&
> +#if _ELFUTILS_PREREQ(0, 157)
> + dwarf_getlocations(attr, 0, &base, &start, &end,
> + &loc.expr, &loc.exprlen) > 0 &&
> +#else
> + dwarf_getlocation(attr, &loc.expr, &loc.exprlen) == 0 &&
> +#endif
> + loc.exprlen != 0) {
> + int expected_reg = cu->register_params[param_idx];
> + Dwarf_Op *expr = loc.expr;
> +
> + switch (expr->atom) {
> + case DW_OP_reg0 ... DW_OP_reg31:
> + /* mark parameters that use an unexpected
> + * register to hold a parameter; these will
> + * be problematic for users of BTF as they
> + * violate expectations about register
> + * contents.
> + */
> + if (expected_reg >= 0 && expected_reg != expr->atom)
> + parm->unexpected_reg = 1;
> + break;
> + default:
> + parm->optimized = 1;
> + break;
> + }
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> struct conf_load *conf, int param_idx)
> {
> struct parameter *parm = tag__alloc(cu, sizeof(*parm));
>
> if (parm != NULL) {
> - Dwarf_Addr base, start, end;
> bool has_const_value;
> Dwarf_Attribute attr;
> - struct location loc;
>
> tag__init(&parm->tag, cu, die);
> parm->name = attr_string(die, DW_AT_name, conf);
> @@ -1208,38 +1248,9 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> */
> has_const_value = dwarf_attr(die, DW_AT_const_value, &attr) != NULL;
> parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
> - /* dwarf_getlocations() handles location lists; here we are
> - * only interested in the first expr.
> - */
> - if (parm->has_loc &&
> -#if _ELFUTILS_PREREQ(0, 157)
> - dwarf_getlocations(&attr, 0, &base, &start, &end,
> - &loc.expr, &loc.exprlen) > 0 &&
> -#else
> - dwarf_getlocation(&attr, &loc.expr, &loc.exprlen) == 0 &&
> -#endif
> - loc.exprlen != 0) {
> - int expected_reg = cu->register_params[param_idx];
> - Dwarf_Op *expr = loc.expr;
> -
> - switch (expr->atom) {
> - case DW_OP_reg0 ... DW_OP_reg31:
> - /* mark parameters that use an unexpected
> - * register to hold a parameter; these will
> - * be problematic for users of BTF as they
> - * violate expectations about register
> - * contents.
> - */
> - if (expected_reg >= 0 && expected_reg != expr->atom)
> - parm->unexpected_reg = 1;
> - break;
> - default:
> - parm->optimized = 1;
> - break;
> - }
> - } else if (has_const_value) {
> +
> + if (!check_dwarf_locations(&attr, parm, cu, param_idx) && has_const_value)
I think it might be easier to follow the logic if we moved the const
check alongside the location checks in the separate location check
function. That way all our optimization-related checking happens in one
place.
> parm->optimized = 1;
> - }
> }
>
> return parm;
