On 11/14/24 4:16 AM, Alan Maguire wrote:
On 13/11/2024 18:27, Yonghong Song wrote:
Thanks for the additional info! From Eduard's analysis, it seems like it
is safer to take the libdw__lock around dwarf_getlocation(s), since
multiple threads can access the CU location cache. I've tried tweaking
Eduard's modification of Yonghong's original patch and adding a second
patch to add locking; with these two patches applied
- we see the desired behaviour where perf_event_read() is present in
BTF; and
- we don't see any segmentation faults after ~700 iterations where I saw
one every 200 or so before
Yonghong, Eduard - do these changes look okay from your side? Feel free
to resubmit if so (fixing up attributions as you see fit if they look
wrong of course). Thanks!
Thanks Alan for working on this. The following are some suggestions for
patch one:
1. rename __dwarf_getlocations() to __parameter__locations()?
2. rename param_reg_at_entry to parameter__locations()?
Since it returns the register number, what about
__parameter_reg/parameter_reg()?
3. You missed the following:
static int param_reg_at_entry(Dwarf_Attribute *attr, int expected_reg)
{
...
if (first_expr) // this line
return first_expr->atom; // this line
return -1;
}
I _think_ I've preserved the behaviour described by the comment at the
start without using the first_expr code. Note that we set "ret" in the
"case DW_OP_reg0 ... DW_OP_reg31:" clause of the switch statement, so
will return that value; either directly, if the register number matches
expected reg, or eventually if we don't find any DW_OP_*entry_value
location info to return. This I think matches the described behaviour:
/* For DW_AT_location 'attr':
* - if first location is DW_OP_regXX with expected number, returns the
register;
* - if location DW_OP_entry_value(DW_OP_regXX) is in the list, returns
the register;
* - if first location is DW_OP_regXX, returns the register;
* - otherwise returns -1.
*/
...but again I may have missed something here.
I have some comments in v2 and will reply there.
Patch 2 needs adjustment as well due to the above point #3.
Otherwise, LGTM. Since you are already preparing the patch,
please go ahead to pose v2 after you fixing the above things.
Sure; if the above sounds okay, I'll submit the patches with updates.
After testing over 2000 iterations of pahole, I haven't seen a
segmentation fault so I _think_ the locking in patch 2 is sufficient to
avoid crashes.
Thanks!
Alan
Alan
