public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
From: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
To: elfutils-devel@lists.fedorahosted.org
Subject: Re: [RFC] elfutils: Checks for debuginfo file without .debug extension as well
Date: Mon, 22 Feb 2016 22:42:47 +0530	[thread overview]
Message-ID: <56CB418F.209@linux.vnet.ibm.com> (raw)
In-Reply-To: 1456148709.7770.91.camel@redhat.com

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

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

             reply	other threads:[~2016-02-22 17:12 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-22 17:12 Ravi Bangoria [this message]
  -- strict thread matches above, loose matches on Subject: below --
2016-02-22 19:54 Mark Wielaard
2016-02-22 13:45 Mark Wielaard
2016-02-20 13:40 Ravi Bangoria
2016-02-19 15:11 Mark Wielaard
2016-02-19  9:37 Ravi Bangoria
2016-02-17 15:49 Mark Wielaard
2016-02-17  8:20 Ravi Bangoria
2016-02-16 16:45 Mark Wielaard
2016-02-16 16:21 Ravi Bangoria

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56CB418F.209@linux.vnet.ibm.com \
    --to=ravi.bangoria@linux.vnet.ibm.com \
    --cc=elfutils-devel@lists.fedorahosted.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).