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/13/24 9:33 AM, Alan Maguire wrote:
> On 12/11/2024 19:10, Arnaldo Carvalho de Melo wrote:
>> On Tue, Nov 12, 2024 at 06:33:38PM +0000, Alan Maguire wrote:
>>> On 12/11/2024 17:07, Yonghong Song wrote:
>>>> On 11/12/24 8:56 AM, Alan Maguire wrote:
>>>>> On 12/11/2024 01:51, Yonghong Song wrote:
>>>>>> On 11/11/24 7:39 AM, Alan Maguire wrote:
>>>>> "for one of internal 6.11 kernel, there are 62498 functions in BTF and
>>>>> perf_event_read() is not there. With this patch, there are 61552
>>>>> functions in BTF and perf_event_read() is included."
>>>>> These numbers suggest you lost nearly 1000 functions when building
>>>>> vmlinux BTF with pahole using this series. That's the part I don't
>>>>> understand - we should just see a gain in numbers of functions in
>>>>> vmlinux BTF, right? Did you mean 62552 functions rather than 61552
>>>>> perhaps?tion
>>   
>>>> Sorry, really embarrassing. it is typo. Indeed it should be 62552 functions
>>>> in BTF instead.
>>   
>>> No problem, makes perfect sense now, thanks! I'm trying to reproduce the
>>> core dumps Eduard saw now with this setup; I'll report back if I manage
>>> to do so and see if locks as Jiri and Arnaldo suggested help. If so a v2
>>> along the lines of Eduard's suggested change plus locking might be the
>>> best approach, what do you think? Thanks!
>> So the idea is to try to see what are the data structures that are
>> being corrupted in the features we use from elfutils libraries and check
>> how they are being protected via their non-default enabled experimental
>> thread safety locks and then use it before calling their functions that
>> would use those locks.
>>
>> At some point we need to do some feature check to see if the lock is
>> enabled there and avoid adding it from pahole's side.
>>
>> I.e. a transitional strategy to keep pahole -j feature that works with
>> older elfutils versions as well as with modern, thread safe ones.
>>
>> This was used with the existing libdw__lock we have in the pahole
>> codebase with, AFAIK, good results.
>>
> 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()?
   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;
}

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.

>
> Alan