Re: [RFC] elfutils: Checks for debuginfo file without .debug extension as well

[Ravi Bangoria <ravi dot bangoria at linux dot vnet dot ibm dot com>]

Thanks Mark,

Please find my comments.

On Monday 22 February 2016 07:15 PM, Mark Wielaard wrote:
> Hi Ravi,
>
> On Sat, 2016-02-20 at 19:10 +0530, Ravi Bangoria wrote:
>> Thanks for the patch. Indeed this is optimized one compared to what I've
>> proposed.
>>
>> On Friday 19 February 2016 08:41 PM, Mark Wielaard wrote:
>>> Thanks, that looks like what I expected.
>>> I also got access to a somewhat newer version of ubuntu ppc64le and that
>>> has an additional issue. It has the vmlinux file installed unreadable
>>> (except for root). That causes almost the same issue, but slightly
>>> differently, now no kernel at all is found... Although in that case it
>>> can be worked around be using /usr/lib/debug as base. But the main issue
>>> is exactly as you describe.
>> Can you please provide me system configurations like ubuntu versrion,
>> kernel version, stap version, elfutils version etc. I'll check if I'll
>> be able to get similar system.
> Ubuntu 15.10 wily. 4.2.0-27-generic #32-Ubuntu SMP ppc64le
> elfutils-0.163-4ubuntu1 stap from systemtap git.

Yes you are right. I checked it. stap is not working on system with
above configurations.

>
>>> Could you try out if this variant of the patch works for you?
>> I've tested your patch and it's working fine with Ubuntu 14.04 with
>> kernel 3.13,
>> stap 2.3 and elfutils 0.165.
> Great, thanks for testing.
>
>> But I've a little doubt here. Here is the code snip of try_kernel_name
>> from libdwfl/linux-kernel-modules.c
>>
>>       static int
>>       try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
>>       {
>> LINE A:
>>         int fd = ((((dwfl->callbacks->debuginfo_path
>>                      ? *dwfl->callbacks->debuginfo_path : NULL)
>>                     ?: DEFAULT_DEBUGINFO_PATH)[0] == ':') ? -1
>>                   : TEMP_FAILURE_RETRY (open64 (*fname, O_RDONLY)));
>>
>>         if (fd < 0)
>>           {
>> LINE B:
>>             /* look for "vmlinux" files.  */
>>             fd = INTUSE(dwfl_standard_find_debuginfo) (&fakemod, NULL,
>> NULL, 0,
>>                                                        *fname, basename
>> (*fname), 0,
>> &fakemod.debug.name);
>>             if (fd < 0 && try_debug)
>> LINE C:
>>               /* look for "vmlinux.debug" files.  */
>>               fd = INTUSE(dwfl_standard_find_debuginfo) (&fakemod, NULL,
>> NULL, 0,
>>                                                          *fname, NULL, 0,
>> &fakemod.debug.name);
>>
>> try_kernel_name is doing almost same thing what I have proposed. Now let's
>> say we want to go ahead with your patch, than call to dwfl_standard_find_debuginfo
>> in LINE C will look for both vmlinux and vmlinux.debug right? But it has
>> already checked for vmlinux in LINE B. So, in this case we have to modify
>> try_kernel_name as well.
> Interesting find. And I think you are right.
>
> In our case on ubuntu ppc64le, LINE A would find the vmlinux image
> already and we would never reach LINE B or C. The separate debuginfo
> would then be found, through dwfl_standard_find_debuginfo, when we want
> to get the DWARF for the kernel.
>
> But on other arches where the main image isn't called vmlinux we would
> indeed hit them. When try_debug == true then we only need to do C now,
> otherwise we only need to do B.
>
> Updated patch attached. This time with updated commit message and
> ChangeLog entry. Does this look correct to you?

This patch looks fine to me. I've tested it on Ubuntu. I think we can go 
ahead
with this.

Regards,
Ravi