Re: [PATCH dwarves 3/3] dwarf_loader: Check DW_OP_[GNU_]entry_value for possible parameter matching

[Yonghong Song <yonghong.song@linux.dev>]

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