Re: [PATCH] libdwfl: Record dwfl_attach_state error and return it on failure.

Re: [PATCH] libdwfl: Record dwfl_attach_state error and return it on failure.

[Jan Kratochvil]

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

On Wed, 11 Jun 2014 15:33:28 +0200, Mark Wielaard wrote:
> When dwfl_attach_state fails functions that need the process state should
> return the error that caused the attach to fail. Use this in the backtrace
> test to signal any attach failure. This makes sure that architectures that
> don't provide unwinder support get properly detected (and the tests SKIPs)
> Also don't assert when trying to attach a non-core ELF file, but return an
> error to indicate failure.

This patch reimplements former 'process_attach_error' under the new name
'attacherr'.  It is also related to the current requirement to call
dwfl_linux_proc_attach() explicitly by the clients since:
	commit 19108019192ab273c53ae324be448d29dac806ca
	Author: Mark Wielaard <mjw@redhat.com>
	Date:   Mon Dec 30 22:00:57 2013 +0100
	    libdwfl: Add dwfl_core_file_attach and dwfl_linux_proc_attach.
So I rather do not agree with the former patch which this one fixes.


Jan

Re: [PATCH] libdwfl: Record dwfl_attach_state error and return it on failure.

[Mark Wielaard]

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

On Wed, 2014-06-11 at 22:26 +0200, Jan Kratochvil wrote:
> On Wed, 11 Jun 2014 15:33:28 +0200, Mark Wielaard wrote:
> > When dwfl_attach_state fails functions that need the process state should
> > return the error that caused the attach to fail. Use this in the backtrace
> > test to signal any attach failure. This makes sure that architectures that
> > don't provide unwinder support get properly detected (and the tests SKIPs)
> > Also don't assert when trying to attach a non-core ELF file, but return an
> > error to indicate failure.
> 
> This patch reimplements former 'process_attach_error' under the new name
> 'attacherr'.  It is also related to the current requirement to call
> dwfl_linux_proc_attach() explicitly by the clients since:
> 	commit 19108019192ab273c53ae324be448d29dac806ca
> 	Author: Mark Wielaard <mjw@redhat.com>
> 	Date:   Mon Dec 30 22:00:57 2013 +0100
> 	    libdwfl: Add dwfl_core_file_attach and dwfl_linux_proc_attach.
> So I rather do not agree with the former patch which this one fixes.

Good detective work. Indeed that commit from last year introduced the
issue because it made the attaching explicit because the implicit
attaching wasn't working out for some programs. So the implicit error
reporting wasn't necessary anymore and got removed.
https://lists.fedorahosted.org/pipermail/elfutils-devel/2013-December/003680.html
Except we forgot to account for the implicit attaching done through the
argp option parser. So this patch adds it back.

Thanks for the review. I'll commit it soon.

Cheers,

Mark

[PATCH] libdwfl: Record dwfl_attach_state error and return it on failure.

[Mark Wielaard]

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

When dwfl_attach_state fails functions that need the process state should
return the error that caused the attach to fail. Use this in the backtrace
test to signal any attach failure. This makes sure that architectures that
don't provide unwinder support get properly detected (and the tests SKIPs)
Also don't assert when trying to attach a non-core ELF file, but return an
error to indicate failure.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog           |   13 ++++++++++++
 libdwfl/dwfl_frame.c        |   46 +++++++++++++++++++++++++++++++++---------
 libdwfl/libdwflP.h          |    4 ++-
 libdwfl/linux-core-attach.c |   44 +++++++++++++++++++++++-----------------
 libdwfl/linux-pid-attach.c  |   25 +++++++++++++++++++---
 tests/ChangeLog             |    5 ++++
 tests/backtrace.c           |    5 +++-
 7 files changed, 107 insertions(+), 35 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index c13e01f..ac92a21 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,16 @@
+2014-06-11  Mark Wielaard  <mjw@redhat.com>
+
+	* dwfl_frame.c (__libdwfl_process_free): Reset dwfl->attacherr.
+	(dwfl_attach_state): Set dwfl->attacherr.
+	(dwfl_pid): Check and return dwfl->attacherr if set.
+	(dwfl_getthreads): Likewise.
+	(getthread): Likewise.
+	* libdwflP.h: Add DWFL_E_NO_CORE_FILE.
+	(struct Dwfl): Add attacherr field.
+	* linux-core-attach.c (dwfl_core_file_attach): Set dwfl->attacherr.
+	Don't assert if ELF file is not ET_CORE, just return error.
+	* linux-pid-attach.c (dwfl_linux_proc_attach): Set dwfl->attacherr.
+
 2014-06-10  Mark Wielaard  <mjw@redhat.com>
 
 	* argp-std.c (parse_opt): Ignore errors from dwfl_core_file_attach
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index fd0b9ae..f6f86c0 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -1,5 +1,5 @@
 /* Get Dwarf Frame state for target PID or core file.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -117,6 +117,7 @@ __libdwfl_process_free (Dwfl_Process *process)
   if (process->ebl_close)
     ebl_closebackend (process->ebl);
   free (process);
+  dwfl->attacherr = DWFL_E_NOERROR;
 }
 
 /* Allocate new Dwfl_Process for DWFL.  */
@@ -134,17 +135,24 @@ bool
 dwfl_attach_state (Dwfl *dwfl, Elf *elf, pid_t pid,
 		   const Dwfl_Thread_Callbacks *thread_callbacks, void *arg)
 {
-  if (thread_callbacks == NULL || thread_callbacks->next_thread == NULL
-      || thread_callbacks->set_initial_registers == NULL)
+  if (dwfl->process != NULL)
     {
-      __libdwfl_seterrno (DWFL_E_INVALID_ARGUMENT);
+      __libdwfl_seterrno (DWFL_E_ATTACH_STATE_CONFLICT);
       return false;
     }
-  if (dwfl->process != NULL)
+
+  /* Reset any previous error, we are just going to try again.  */
+  dwfl->attacherr = DWFL_E_NOERROR;
+  if (thread_callbacks == NULL || thread_callbacks->next_thread == NULL
+      || thread_callbacks->set_initial_registers == NULL)
     {
-      __libdwfl_seterrno (DWFL_E_ATTACH_STATE_CONFLICT);
+      dwfl->attacherr = DWFL_E_INVALID_ARGUMENT;
+    fail:
+      dwfl->attacherr = __libdwfl_canon_error (dwfl->attacherr);
+      __libdwfl_seterrno (dwfl->attacherr);
       return false;
     }
+
   Ebl *ebl;
   bool ebl_close;
   if (elf != NULL)
@@ -180,8 +188,8 @@ dwfl_attach_state (Dwfl *dwfl, Elf *elf, pid_t pid,
   if (ebl == NULL)
     {
       /* Not identified EBL from any of the modules.  */
-      __libdwfl_seterrno (DWFL_E_PROCESS_NO_ARCH);
-      return false;
+      dwfl->attacherr = DWFL_E_PROCESS_NO_ARCH;
+      goto fail;
     }
   process_alloc (dwfl);
   Dwfl_Process *process = dwfl->process;
@@ -189,8 +197,8 @@ dwfl_attach_state (Dwfl *dwfl, Elf *elf, pid_t pid,
     {
       if (ebl_close)
 	ebl_closebackend (ebl);
-      __libdwfl_seterrno (DWFL_E_NOMEM);
-      return false;
+      dwfl->attacherr = DWFL_E_NOMEM;
+      goto fail;
     }
   process->ebl = ebl;
   process->ebl_close = ebl_close;
@@ -204,6 +212,12 @@ INTDEF(dwfl_attach_state)
 pid_t
 dwfl_pid (Dwfl *dwfl)
 {
+  if (dwfl->attacherr != DWFL_E_NOERROR)
+    {
+      __libdwfl_seterrno (dwfl->attacherr);
+      return -1;
+    }
+
   if (dwfl->process == NULL)
     {
       __libdwfl_seterrno (DWFL_E_NO_ATTACH_STATE);
@@ -238,6 +252,12 @@ int
 dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
 		 void *arg)
 {
+  if (dwfl->attacherr != DWFL_E_NOERROR)
+    {
+      __libdwfl_seterrno (dwfl->attacherr);
+      return -1;
+    }
+
   Dwfl_Process *process = dwfl->process;
   if (process == NULL)
     {
@@ -309,6 +329,12 @@ getthread (Dwfl *dwfl, pid_t tid,
 	   int (*callback) (Dwfl_Thread *thread, void *arg),
 	   void *arg)
 {
+  if (dwfl->attacherr != DWFL_E_NOERROR)
+    {
+      __libdwfl_seterrno (dwfl->attacherr);
+      return -1;
+    }
+
   Dwfl_Process *process = dwfl->process;
   if (process == NULL)
     {
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 9b03d8a..30c0f8a 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -91,7 +91,8 @@ typedef struct Dwfl_Process Dwfl_Process;
   DWFL_ERROR (ATTACH_STATE_CONFLICT, N_("Dwfl already has attached state"))   \
   DWFL_ERROR (NO_ATTACH_STATE, N_("Dwfl has no attached state"))	      \
   DWFL_ERROR (NO_UNWIND, N_("Unwinding not supported for this architecture")) \
-  DWFL_ERROR (INVALID_ARGUMENT, N_("Invalid argument"))
+  DWFL_ERROR (INVALID_ARGUMENT, N_("Invalid argument"))			      \
+  DWFL_ERROR (NO_CORE_FILE, N_("Not an ET_CORE ELF file"))
 
 #define DWFL_ERROR(name, text) DWFL_E_##name,
 typedef enum { DWFL_ERRORS DWFL_E_NUM } Dwfl_Error;
@@ -110,6 +111,7 @@ struct Dwfl
   Dwfl_Module *modulelist;    /* List in order used by full traversals.  */
 
   Dwfl_Process *process;
+  Dwfl_Error attacherr;      /* Previous error attaching process.  */
 
   GElf_Addr offline_next_address;
 
diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index 1002788..7ef3f25 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -1,5 +1,5 @@
 /* Get Dwarf Frame state for target core file.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -309,33 +309,41 @@ static const Dwfl_Thread_Callbacks core_thread_callbacks =
 int
 dwfl_core_file_attach (Dwfl *dwfl, Elf *core)
 {
+  Dwfl_Error err = DWFL_E_NOERROR;
   Ebl *ebl = ebl_openbackend (core);
   if (ebl == NULL)
     {
-      __libdwfl_seterrno (DWFL_E_LIBEBL);
+      err = DWFL_E_LIBEBL;
+    fail_err:
+      if (dwfl->process == NULL && dwfl->attacherr == DWFL_E_NOERROR)
+	dwfl->attacherr = __libdwfl_canon_error (err);
+      __libdwfl_seterrno (err);
       return -1;
     }
   size_t nregs = ebl_frame_nregs (ebl);
   if (nregs == 0)
     {
-      __libdwfl_seterrno (DWFL_E_NO_UNWIND);
+      err = DWFL_E_NO_UNWIND;
+    fail:
       ebl_closebackend (ebl);
-      return -1;
+      goto fail_err;
     }
   GElf_Ehdr ehdr_mem, *ehdr = gelf_getehdr (core, &ehdr_mem);
   if (ehdr == NULL)
     {
-      __libdwfl_seterrno (DWFL_E_LIBELF);
-      ebl_closebackend (ebl);
-      return -1;
+      err = DWFL_E_LIBELF;
+      goto fail;
+    }
+  if (ehdr->e_type != ET_CORE)
+    {
+      err = DWFL_E_NO_CORE_FILE;
+      goto fail;
     }
-  assert (ehdr->e_type == ET_CORE);
   size_t phnum;
   if (elf_getphdrnum (core, &phnum) < 0)
     {
-      __libdwfl_seterrno (DWFL_E_LIBELF);
-      ebl_closebackend (ebl);
-      return -1;
+      err = DWFL_E_LIBELF;
+      goto fail;
     }
   pid_t pid = -1;
   Elf_Data *note_data = NULL;
@@ -351,8 +359,8 @@ dwfl_core_file_attach (Dwfl *dwfl, Elf *core)
     }
   if (note_data == NULL)
     {
-      ebl_closebackend (ebl);
-      return DWFL_E_LIBELF;
+      err = DWFL_E_LIBELF;
+      goto fail;
     }
   size_t offset = 0;
   GElf_Nhdr nhdr;
@@ -394,16 +402,14 @@ dwfl_core_file_attach (Dwfl *dwfl, Elf *core)
   if (pid == -1)
     {
       /* No valid NT_PRPSINFO recognized in this CORE.  */
-      __libdwfl_seterrno (DWFL_E_BADELF);
-      ebl_closebackend (ebl);
-      return -1;
+      err = DWFL_E_BADELF;
+      goto fail;
     }
   struct core_arg *core_arg = malloc (sizeof *core_arg);
   if (core_arg == NULL)
     {
-      __libdwfl_seterrno (DWFL_E_NOMEM);
-      ebl_closebackend (ebl);
-      return -1;
+      err = DWFL_E_NOMEM;
+      goto fail;
     }
   core_arg->core = core;
   core_arg->note_data = note_data;
diff --git a/libdwfl/linux-pid-attach.c b/libdwfl/linux-pid-attach.c
index 8aee721..d60955e 100644
--- a/libdwfl/linux-pid-attach.c
+++ b/libdwfl/linux-pid-attach.c
@@ -290,13 +290,23 @@ dwfl_linux_proc_attach (Dwfl *dwfl, pid_t pid, bool assume_ptrace_stopped)
 {
   char buffer[36];
   FILE *procfile;
+  int err = 0; /* The errno to return and set for dwfl->attcherr.  */
 
   /* Make sure to report the actual PID (thread group leader) to
      dwfl_attach_state.  */
   snprintf (buffer, sizeof (buffer), "/proc/%ld/status", (long) pid);
   procfile = fopen (buffer, "r");
   if (procfile == NULL)
-    return errno;
+    {
+      err = errno;
+    fail:
+      if (dwfl->process == NULL && dwfl->attacherr == DWFL_E_NOERROR)
+	{
+	  errno = err;
+	  dwfl->attacherr = __libdwfl_canon_error (DWFL_E_ERRNO);
+	}
+      return err;
+    }
 
   char *line = NULL;
   size_t linelen = 0;
@@ -317,19 +327,26 @@ dwfl_linux_proc_attach (Dwfl *dwfl, pid_t pid, bool assume_ptrace_stopped)
   fclose (procfile);
 
   if (pid == 0)
-    return ESRCH;
+    {
+      err = ESRCH;
+      goto fail;
+    }
 
   char dirname[64];
   int i = snprintf (dirname, sizeof (dirname), "/proc/%ld/task", (long) pid);
   assert (i > 0 && i < (ssize_t) sizeof (dirname) - 1);
   DIR *dir = opendir (dirname);
   if (dir == NULL)
-    return errno;
+    {
+      err = errno;
+      goto fail;
+    }
   struct __libdwfl_pid_arg *pid_arg = malloc (sizeof *pid_arg);
   if (pid_arg == NULL)
     {
       closedir (dir);
-      return ENOMEM;
+      err = ENOMEM;
+      goto fail;
     }
   pid_arg->dir = dir;
   pid_arg->tid_attached = 0;
diff --git a/tests/ChangeLog b/tests/ChangeLog
index bb0ad0d..cd8a8e7 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-11  Mark Wielaard  <mjw@redhat.com>
+
+	* backtrace.c (main): Check that Dwfl was attached by calling
+	dwfl_pid and printing the error when it is not.
+
 2014-05-18  Mark Wielaard  <mjw@redhat.com>
 
 	* testfile-backtrace-demangle.cc (cxxfunc): Make non-static.
diff --git a/tests/backtrace.c b/tests/backtrace.c
index 1a4709b..ce0bd17 100644
--- a/tests/backtrace.c
+++ b/tests/backtrace.c
@@ -1,5 +1,5 @@
 /* Test program for unwinding of frames.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -459,6 +459,9 @@ main (int argc __attribute__ ((unused)), char **argv)
     };
   (void) argp_parse (&argp, argc, argv, 0, NULL, &dwfl);
   assert (dwfl != NULL);
+  /* We want to make sure the dwfl was properly attached.  */
+  if (dwfl_pid (dwfl) < 0)
+    error (2, 0, "dwfl_pid: %s", dwfl_errmsg (-1));
   dump (dwfl);
   dwfl_end (dwfl);
   return 0;
-- 
1.7.1