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

[Mark Wielaard <mjw at redhat dot com>]

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.

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

Thanks,

Mark

From 786d65d8848efaa9d62b4121790c520d3a6782fb Mon Sep 17 00:00:00 2001
From: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Date: Tue, 16 Feb 2016 21:51:35 +0530
Subject: [PATCH] libdwfl: Checks for (kernel) debuginfo file without .debug
 extension as well

Elfutils, looking for kernel debuginfo file, tries to find it at
various places. If elfutils finds /boot/vmlinu*x* file, it checks
for debufginfo section. If debuginfo is not present, it saves it as
'main elf' and continue looking for debuginfo file having .debug
extension i.e. vmlinux-RELEASE.debug.

'Ubuntu on powerpc' installs kernel as /boot/vmlinux and installs
debuginfo without any extension as /usr/lib/debug/boot/vmlinux-RELEASE
and hence, elfutils is not able to find the debuginfo file.

Here is the lunchpad bug for the same:
  https://bugs.launchpad.net/ubuntu/+source/systemtap/+bug/1537125

This patch adds functionality to search for a kernel or debuginfo file
both with and without .debug extension.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog              | 10 ++++++++++
 libdwfl/find-debuginfo.c       | 15 ++++++++++++++-
 libdwfl/linux-kernel-modules.c | 20 ++++++++++++--------
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 69fd233..462a61d 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,13 @@
+2016-02-22  Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
+	    Mark Wielaard  <mjw@redhat.com>
+
+	* find-debuginfo.c (find_debuginfo_in_path): Remember whether
+	debuglink_file is NULL. Try file basename (without .debug) ofr
+	absolute and relative path if debug_file was NULL.
+	* linux-kernel-modules.c (try_kernel_name): If try_debug is true call
+	dwfl_standard_find_debuginfo with NULL debuglink_file, otherwise with
+	basename of fname.
+
 2016-02-13  Mark Wielaard  <mjw@redhat.com>
 
 	* linux-proc-maps.c (proc_maps_report): Free last_file when ENOEXEC.
diff --git a/libdwfl/find-debuginfo.c b/libdwfl/find-debuginfo.c
index 72461bc..80515db 100644
--- a/libdwfl/find-debuginfo.c
+++ b/libdwfl/find-debuginfo.c
@@ -163,7 +163,11 @@ find_debuginfo_in_path (Dwfl_Module *mod, const char *file_name,
 
   const char *file_basename = file_name == NULL ? NULL : basename (file_name);
   char *localname = NULL;
-  if (debuglink_file == NULL)
+
+  /* We invent a debuglink .debug name if NULL, but then want to try the
+     basename too.  */
+  bool debuglink_null = debuglink_file == NULL;
+  if (debuglink_null)
     {
       /* For a alt debug multi file we need a name, for a separate debug
 	 name we may be able to fall back on file_basename.debug.  */
@@ -231,6 +235,10 @@ find_debuginfo_in_path (Dwfl_Module *mod, const char *file_name,
 	check = *p++ == '+';
       check = check && cancheck;
 
+      /* Try the basename too, if we made up the debuglink name and this
+	 is not the main directory.  */
+      bool try_file_basename;
+
       const char *dir, *subdir, *file;
       switch (p[0])
 	{
@@ -239,6 +247,7 @@ find_debuginfo_in_path (Dwfl_Module *mod, const char *file_name,
 	  dir = file_dirname;
 	  subdir = NULL;
 	  file = debuglink_file;
+	  try_file_basename = false;
 	  break;
 	case '/':
 	  /* An absolute path says to look there for a subdirectory
@@ -268,6 +277,7 @@ find_debuginfo_in_path (Dwfl_Module *mod, const char *file_name,
 	      subdir = NULL;
 	      file = basename (debuglink_file);
 	    }
+	  try_file_basename = debuglink_null;
 	  break;
 	default:
 	  /* A relative path says to try a subdirectory of that name
@@ -275,11 +285,14 @@ find_debuginfo_in_path (Dwfl_Module *mod, const char *file_name,
 	  dir = file_dirname;
 	  subdir = p;
 	  file = debuglink_file;
+	  try_file_basename = debuglink_null;
 	  break;
 	}
 
       char *fname = NULL;
       int fd = try_open (&main_stat, dir, subdir, file, &fname);
+      if (fd < 0 && try_file_basename)
+	fd = try_open (&main_stat, dir, subdir, file_basename, &fname);
       if (fd < 0)
 	switch (errno)
 	  {
diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c
index 79faf99..54c0b90 100644
--- a/libdwfl/linux-kernel-modules.c
+++ b/libdwfl/linux-kernel-modules.c
@@ -92,17 +92,21 @@ try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
   if (fd < 0)
     {
       Dwfl_Module fakemod = { .dwfl = dwfl };
-      /* First try the file's unadorned basename as DEBUGLINK_FILE,
-	 to 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)
-	/* Next, let the call use the default of basename + ".debug",
-	   to look for "vmlinux.debug" files.  */
+
+      if (try_debug)
+	/* Passing NULL for DEBUGLINK_FILE searches for both the basenamer
+	   "vmlinux" and the default of basename + ".debug", to look for
+	   "vmlinux.debug" files.  */
 	fd = INTUSE(dwfl_standard_find_debuginfo) (&fakemod, NULL, NULL, 0,
 						   *fname, NULL, 0,
 						   &fakemod.debug.name);
+      else
+	/* Try the file's unadorned basename as DEBUGLINK_FILE,
+	   to look only for "vmlinux" files.  */
+	fd = INTUSE(dwfl_standard_find_debuginfo) (&fakemod, NULL, NULL, 0,
+						   *fname, basename (*fname),
+						   0, &fakemod.debug.name);
+
       if (fakemod.debug.name != NULL)
 	{
 	  free (*fname);
-- 
1.8.3.1