Re: [PATCH v2 3/3] dwarf_begin, dwarf_begin_elf: DWARF_C_FLAG_NO_DEBUGALTLINK command flag

Re: [PATCH v2 3/3] dwarf_begin, dwarf_begin_elf: DWARF_C_FLAG_NO_DEBUGALTLINK command flag

[Mark Wielaard]

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

On Thu, 2014-04-10 at 13:08 +0200, Florian Weimer wrote:
> +	* libdw.h (enum Dwarf_Cmd): New member
> +	DWARF_C_FLAG_NO_DEBUGALTLINK.
> +	* libdwP.h (DWARF_C_FLAG_MASK): New macro.
> +	* dwarf_begin_elf.c (check_section): Move call to to
> +	open_debugaltlink to ...
> +	(dwarf_begin_elf): here.  Mask command flags and handle
> +	DWARF_C_FLAG_NO_DEBUGALTLINK.
> +	* dwarf_begin.c (dwarf_begin):  Mask command flags.

I don't really like this. This assumes we keep the magic opening of the
altfile in dw. But I think we should just drop it eventually and move
any magic to the dwfl side. When we provide dwarf_setalt() then users
should just explicitly always use that IMHO.

Cheers,

Mark

Re: [PATCH v2 3/3] dwarf_begin, dwarf_begin_elf: DWARF_C_FLAG_NO_DEBUGALTLINK command flag

[Mark Wielaard]

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

On Thu, 2014-04-10 at 20:26 +0200, Florian Weimer wrote:
> On 04/10/2014 08:16 PM, Mark Wielaard wrote:
> > I don't really like this. This assumes we keep the magic opening of the
> > altfile in dw. But I think we should just drop it eventually
> 
> Yes, but that's not really backwards-compatible.  Even the test suite 
> breaks.

Yes, if we make that change, then we should document clearly that people
should add alt-files explicitly, if they aren't using libdwfl functions
yet to get at the Dwarf. The current support is behind an experimental
configure flag: --enable-dwz enable experimental GNU ref_alt FORM, dwz
multi file support in libdw. So I don't feel too bad for breaking things
a little as long as it doesn't break ABI. But we will see how severe it
all is before doing it.

>  > and move any magic to the dwfl side. When we provide dwarf_setalt() 
> then users
> > should just explicitly always use that IMHO.
> 
> I have a patch for that as well, but I couldn't find a way to emulate 
> the old dwarf_begin behavior using dwfl in the tests/allfcts.c test 
> case.  Pointers to documentation or example code much appreciated. :-)

In the case of the tests/run-allfcts-multi.sh testcase I think the "fix"
is simply to be explicit about the Dwarf and alt-Dwarf under test. So
just change tests/allfcts.c to take as argument both the main Dwarf and
the alt-file, and then change the invocation to something like:
  allfcts testfile_multi_main:testfile_multi.dwz \
    libtestfile_multi_shared.so:testfile_multi.dwz \
    testfile-dwzstr:testfile-dwzstr.multi
Or if you like to extend the test, just use your newly proposed
dwarf_debugaltlink function (or how we will call it) to fetch it in the
testcase. The test is about the Dwarf and alt-file handling, so better
be explicit about it in the first place.

We really should have more libdwfl use documentation... But for a
generic dwfl setup example see src/stack.c. That does a bit more than
most, but then you can easily handle the interesting life pid and core
file cases. Or see libdwfl/argp-std.c that wraps it all in a convenient
argp handler (and shows some examples of handling online kernels). See
below that code, plus extended to handle simple elf-files (with
automatic separate .debug file searching).

// You might want to set this to influence the standard dwfl debuginfo
// search path. See "Standard callbacks" comment in elfutils/libdwfl.h
static char *debuginfo_path = NULL;

// Life pid case...
static const Dwfl_Callbacks proc_callbacks =
  {
    .find_elf = dwfl_linux_proc_find_elf,
    .find_debuginfo = dwfl_standard_find_debuginfo,
    .debuginfo_path = &debuginfo_path,
  };

  int pid = ...;

  dwfl = dwfl_begin (&proc_callbacks);
  if (dwfl == NULL)
    error (EXIT_BAD, 0, "dwfl_begin: %s", dwfl_errmsg (-1));

  int err = dwfl_linux_proc_report (dwfl, pid);
  if (err < 0)
    error (EXIT_BAD, 0, "dwfl_linux_proc_report pid %d: %s", pid,
           dwfl_errmsg (-1));
  else if (err > 0)
    error (EXIT_BAD, err, "dwfl_linux_proc_report pid %d", pid);

  if (dwfl_report_end (dwfl, NULL, NULL) != 0)
    error (EXIT_BAD, 0, "dwfl_report_end: %s", dwfl_errmsg (-1));

  // Optionally if you need the proc state:
  int err = dwfl_linux_proc_attach (dwfl, pid, false);
  if (err < 0)
    error (EXIT_BAD, 0, "dwfl_linux_proc_attach pid %d: %s", pid,
           dwfl_errmsg (-1));
  else if (err > 0)
    error (EXIT_BAD, err, "dwfl_linux_proc_attach pid %d", pid);


// Core file case...
static const Dwfl_Callbacks core_callbacks =
  {
    .find_elf = dwfl_build_id_find_elf,
    .find_debuginfo = dwfl_standard_find_debuginfo,
    .debuginfo_path = &debuginfo_path,
  };

  /* Core file opened with libelf.  */
  Elf *core = ...;
  /* Optional path to original executable, may be NULL.  */
  const char *exec = ...;

  dwfl = dwfl_begin (&core_callbacks);
  if (dwfl == NULL)
    error (EXIT_BAD, 0, "dwfl_begin: %s", dwfl_errmsg (-1));
  if (dwfl_core_file_report (dwfl, core, exec) < 0)
    error (EXIT_BAD, 0, "dwfl_core_file_report: %s", dwfl_errmsg (-1));

  if (dwfl_report_end (dwfl, NULL, NULL) != 0)
    error (EXIT_BAD, 0, "dwfl_report_end: %s", dwfl_errmsg (-1));

  // Optionally if you need the core state:
  if (dwfl_core_file_attach (dwfl, core) < 0)
    error (EXIT_BAD, 0, "dwfl_core_file_report: %s", dwfl_errmsg (-1));


// Single static ELF file case... (also handles ET_REL kernel modules)
// You can also combine the core_callbacks and file_callbacks in one
// to handle both cases. And you can keep adding more ET_DYN files with
// dwfl_report_offline as long as you report the ET_EXEC one first.
static const Dwfl_Callbacks file_callbacks =
  {
    .section_address = dwfl_offline_section_address,
    .find_debuginfo = dwfl_standard_find_debuginfo,
    .debuginfo_path = &debuginfo_path,
  };

  const char *elf_file = ...;

  dwfl = dwfl_begin (&file_callbacks);
  if (dwfl == NULL)
    error (EXIT_BAD, 0, "dwfl_begin: %s", dwfl_errmsg (-1));
  if (dwfl_report_offline (dwfl, elf_file, elf_file, -1) == NULL)
    error (EXIT_BAD, 0, "dwfl_report_offline: %s", dwfl_errmsg (-1));

  if (dwfl_report_end (dwfl, NULL, NULL) != 0)
    error (EXIT_BAD, 0, "dwfl_report_end: %s", dwfl_errmsg (-1));

Then you can simply walk all Dwarf found for the reported modules
through things like:

  Dwarf_Die *cu = NULL;
  Dwarf_Addr bias;
  while ((cu = dwfl_nextcu (dwfl, cu, &bias)) != NULL)
    {
      Dwarf_Die *die = cu;
      printf ("Compile Unit: %s\n", dwarf_diename (die));
    }

I'll work on making sure the alt-files are properly attached in these
cases, even if we drop the automatic alt-file search from libdw proper.

Cheers,

Mark

Re: [PATCH v2 3/3] dwarf_begin, dwarf_begin_elf: DWARF_C_FLAG_NO_DEBUGALTLINK command flag

[Florian Weimer]

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

On 04/10/2014 08:16 PM, Mark Wielaard wrote:
> On Thu, 2014-04-10 at 13:08 +0200, Florian Weimer wrote:
>> +	* libdw.h (enum Dwarf_Cmd): New member
>> +	DWARF_C_FLAG_NO_DEBUGALTLINK.
>> +	* libdwP.h (DWARF_C_FLAG_MASK): New macro.
>> +	* dwarf_begin_elf.c (check_section): Move call to to
>> +	open_debugaltlink to ...
>> +	(dwarf_begin_elf): here.  Mask command flags and handle
>> +	DWARF_C_FLAG_NO_DEBUGALTLINK.
>> +	* dwarf_begin.c (dwarf_begin):  Mask command flags.
>
> I don't really like this. This assumes we keep the magic opening of the
> altfile in dw. But I think we should just drop it eventually

Yes, but that's not really backwards-compatible.  Even the test suite 
breaks.

 > and move any magic to the dwfl side. When we provide dwarf_setalt() 
then users
> should just explicitly always use that IMHO.

I have a patch for that as well, but I couldn't find a way to emulate 
the old dwarf_begin behavior using dwfl in the tests/allfcts.c test 
case.  Pointers to documentation or example code much appreciated. :-)

Your dwarf_setalt comments make sense.

-- 
Florian Weimer / Red Hat Product Security Team

[PATCH v2 3/3] dwarf_begin, dwarf_begin_elf: DWARF_C_FLAG_NO_DEBUGALTLINK command flag

[Florian Weimer]

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

Signed-off-by: Florian Weimer <fweimer@redhat.com>
---
 libdw/ChangeLog         |  9 +++++++++
 libdw/dwarf_begin.c     |  2 +-
 libdw/dwarf_begin_elf.c | 36 +++++++++++++++++++++---------------
 libdw/libdw.h           |  3 +++
 libdw/libdwP.h          |  2 ++
 5 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 798e2ba..5af07a8 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -18,6 +18,15 @@
 	* dwarf_begin_elf.c (try_debugaltlink): Call dwarf_set_alt.
 	* dwarf_end.c (dwarf_end): Likewise.
 
+	* libdw.h (enum Dwarf_Cmd): New member
+	DWARF_C_FLAG_NO_DEBUGALTLINK.
+	* libdwP.h (DWARF_C_FLAG_MASK): New macro.
+	* dwarf_begin_elf.c (check_section): Move call to to
+	open_debugaltlink to ...
+	(dwarf_begin_elf): here.  Mask command flags and handle
+	DWARF_C_FLAG_NO_DEBUGALTLINK.
+	* dwarf_begin.c (dwarf_begin):  Mask command flags.
+
 2014-03-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Fix abort() on missing section headers.
diff --git a/libdw/dwarf_begin.c b/libdw/dwarf_begin.c
index 9f3050f..45320d2 100644
--- a/libdw/dwarf_begin.c
+++ b/libdw/dwarf_begin.c
@@ -47,7 +47,7 @@ dwarf_begin (fd, cmd)
   Elf_Cmd elfcmd;
   Dwarf *result = NULL;
 
-  switch (cmd)
+  switch (cmd & ~DWARF_C_FLAG_MASK)
     {
     case DWARF_C_READ:
       elfcmd = ELF_C_READ_MMAP;
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index e530a88..241ff9e 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -313,19 +313,6 @@ check_section (Dwarf *result, GElf_Ehdr *ehdr, Elf_Scn *scn, bool inscngrp)
       }
 #endif
 
-#ifdef ENABLE_DWZ
-  /* For dwz multifile support, ignore if it looks wrong.  */
-  if (result->sectiondata[IDX_gnu_debugaltlink] != NULL)
-    {
-      const char *alt_name;
-      const void *build_id;
-      size_t id_len;
-      if (INTUSE (dwarf_debugaltlink) (result, &alt_name, &build_id, &id_len)
-	  == 0)
-	return open_debugaltlink (result, alt_name, build_id, id_len);
-    }
-#endif /* ENABLE_DWZ */
-
   return result;
 }
 
@@ -416,6 +403,10 @@ dwarf_begin_elf (elf, cmd, scngrp)
 {
   GElf_Ehdr *ehdr;
   GElf_Ehdr ehdr_mem;
+  bool load_debugaltlink = (cmd & DWARF_C_FLAG_NO_DEBUGALTLINK) == 0;
+
+  /* Restrict to the actual command flags.  */
+  cmd &= ~DWARF_C_FLAG_MASK;
 
   /* Get the ELF header of the file.  We need various pieces of
      information from it.  */
@@ -468,9 +459,24 @@ dwarf_begin_elf (elf, cmd, scngrp)
 	 sections with the name are ignored.  The DWARF specification
 	 does not really say this is allowed.  */
       if (scngrp == NULL)
-	return global_read (result, elf, ehdr);
+	result = global_read (result, elf, ehdr);
       else
-	return scngrp_read (result, elf, ehdr, scngrp);
+	result = scngrp_read (result, elf, ehdr, scngrp);
+
+#ifdef ENABLE_DWZ
+      /* For dwz multifile support, ignore if it looks wrong.  */
+      if (result && load_debugaltlink
+	  && result->sectiondata[IDX_gnu_debugaltlink] != NULL)
+	{
+	  const char *alt_name;
+	  const void *build_id;
+	  size_t id_len;
+	  if (INTUSE (dwarf_debugaltlink)
+	      (result, &alt_name, &build_id, &id_len) == 0)
+	    result = open_debugaltlink (result, alt_name, build_id, id_len);
+	}
+#endif /* ENABLE_DWZ */
+      return result;
     }
   else if (cmd == DWARF_C_WRITE)
     {
diff --git a/libdw/libdw.h b/libdw/libdw.h
index 1880c9a..ce8ffb9 100644
--- a/libdw/libdw.h
+++ b/libdw/libdw.h
@@ -56,6 +56,9 @@ typedef enum
     DWARF_C_READ,		/* Read .. */
     DWARF_C_RDWR,		/* Read and write .. */
     DWARF_C_WRITE,		/* Write .. */
+
+    /* Do not load files referenced from .gnu_debugaltlink.  */
+    DWARF_C_FLAG_NO_DEBUGALTLINK = 0x40
   }
 Dwarf_Cmd;
 
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index db71466..f12fed1 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -36,6 +36,8 @@
 #include <libdw.h>
 #include <dwarf.h>
 
+/* Non-command flags in Dwarf_Cmd.  */
+#define DWARF_C_FLAG_MASK DWARF_C_FLAG_NO_DEBUGALTLINK
 
 /* gettext helper macros.  */
 #define _(Str) dgettext ("elfutils", Str)
-- 
1.9.0