| Message ID | 20241108180513.1197600-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Check DW_OP_[GNU_]entry_value for possible parameter matching | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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@linux.dev> > --- > 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;
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) +{ + 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) parm->optimized = 1; - } } return parm;
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@linux.dev> --- dwarf_loader.c | 77 ++++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 33 deletions(-)