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.
> 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
>
