Re: [patch v7 3/5] x86* unwinder: libdwfl/

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 21 Oct 2013 00:23:34 +0200, Mark Wielaard wrote:
> On Sun, Oct 13, 2013 at 12:24:55PM +0200, Jan Kratochvil wrote:
> > 	* dwfl_frame_pid.c: New file.
> > 	* dwfl_frame_core.c: New file.
> 
> Nitpick. Since these files don't actually contain the public
> functions dwfl_frame_pid or dwfl_frame_core they might be better
> named something without the dwfl_ prefix (linux-pid-attach.c and
> linux-core-attach.c, or libdwfl_attach_pid/core.c maybe?)

done:
	dwfl_frame_core.c -> linux-core-attach.c
	dwfl_frame_pid.c  -> linux-pid-attach.c


> > diff --git a/libdwfl/dwfl_frame_core.c b/libdwfl/dwfl_frame_core.c
> > +struct core_arg
> > +{
> > +  Elf *core;
> > +  Elf_Data *note_data;
> > +  size_t thread_note_offset;
> > +  Ebl *ebl;
> > +};
> > +
> > +struct thread_arg
> > +{
> > +  struct core_arg *core_arg;
> > +  size_t note_offset;
> > +};
> > +
> > +static bool
> > +core_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
> > +		  void *dwfl_arg)
> > +{
> > +  Dwfl_Process *process = dwfl->process;
> > +  struct core_arg *core_arg = dwfl_arg;
> > +  Elf *core = core_arg->core;
> > +  assert (core != NULL);
> > +  static size_t phnum;
> > +  if (elf_getphdrnum (core, &phnum) < 0)
> > +    return false;
> 
> __libdwfl_seterrno (DWFL_E_LIBELF) ?

Yes, fixed.


> > +  for (size_t cnt = 0; cnt < phnum; ++cnt)
> > +    {
> > +      GElf_Phdr phdr_mem, *phdr = gelf_getphdr (core, cnt, &phdr_mem);
> > +      if (phdr == NULL || phdr->p_type != PT_LOAD)
> > +	continue;
> > +      /* Bias is zero here, a core file itself has no bias.  */
> > +      GElf_Addr start = __libdwfl_segment_start (dwfl, phdr->p_vaddr);
> > +      GElf_Addr end = __libdwfl_segment_end (dwfl,
> > +					     phdr->p_vaddr + phdr->p_memsz);
> > +      unsigned bytes = process->ebl->class == ELFCLASS64 ? 8 : 4;
> > +      if (addr < start || addr + bytes > end)
> > +	continue;
> > +      Elf_Data *data;
> > +      data = elf_getdata_rawchunk (core, phdr->p_offset + addr - start,
> > +				   bytes, ELF_T_ADDR);
> > +      if (data == NULL)
> > +	return false;
> 
> __libdwfl_seterrno (DWFL_E_LIBELF) ?

Yes.


> > +      assert (data->d_size == bytes);
> 
> Maybe assert is too strong here? Could there be corrupt core files?

No.  In such case elf_getdata_rawchunk would return NULL.

I do not think this assert can fail.


> > +      /* FIXME: Currently any arch supported for unwinding supports
> > +	 unaligned access.  */
> > +      if (bytes == 8)
> > +	*result = *(const uint64_t *) data->d_buf;
> > +      else
> > +	*result = *(const uint32_t *) data->d_buf;
> > +      return true;
> > +    }
> > +  return false;
> > +}
> 
> It would be nice to set dwfl_errno at the end to indicate the core file
> just didn't contain that memory. DWFL_E_ADDR_OUTOFRANGE or DWFL_E_NO_MATCH?

Put there DWFL_E_ADDR_OUTOFRANGE.


> > +static pid_t
> > +core_next_thread (Dwfl *dwfl __attribute__ ((unused)),
> > +		  Dwfl_Thread *nthread __attribute__ ((unused)), void *dwfl_arg,
> > +		  void **thread_argp)
> > +{
> > +  struct core_arg *core_arg = dwfl_arg;
> > +  Elf *core = core_arg->core;
> > +  GElf_Nhdr nhdr;
> > +  size_t name_offset;
> > +  size_t desc_offset;
> > +  Elf_Data *note_data = core_arg->note_data;
> > +  size_t offset;
> > +  while (offset = core_arg->thread_note_offset, offset < note_data->d_size
> > +	 && (core_arg->thread_note_offset = gelf_getnote (note_data, offset,
> > +							  &nhdr, &name_offset,
> > +							  &desc_offset)) > 0)
> > +    {
> > +      /* Do not check NAME for now, help broken Linux kernels.  */
> > +      const char *name = note_data->d_buf + name_offset;
> > +      const char *desc = note_data->d_buf + desc_offset;
> > +      GElf_Word regs_offset;
> > +      size_t nregloc;
> > +      const Ebl_Register_Location *reglocs;
> > +      size_t nitems;
> > +      const Ebl_Core_Item *items;
> > +      if (! ebl_core_note (core_arg->ebl, &nhdr, name,
> > +			   &regs_offset, &nregloc, &reglocs, &nitems, &items))
> > +	{
> > +	  /* This note may be just not recognized, skip it.  */
> > +	  continue;
> > +	}
> > +      if (nhdr.n_type != NT_PRSTATUS)
> > +	continue;
> > +      const Ebl_Core_Item *item;
> > +      for (item = items; item < items + nitems; item++)
> > +	if (strcmp (item->name, "pid") == 0)
> > +	  break;
> > +      if (item == items + nitems)
> > +	continue;
> > +      uint32_t val32 = *(const uint32_t *) (desc + item->offset);
> > +      val32 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> > +		? be32toh (val32) : le32toh (val32));
> > +      pid_t tid = (int32_t) val32;
> > +      eu_static_assert (sizeof val32 <= sizeof tid);
> > +      struct thread_arg *thread_arg = malloc (sizeof (*thread_arg));
> > +      if (thread_arg == NULL)
> > +	{
> > +	  __libdwfl_seterrno (DWFL_E_NOMEM);
> > +	  return 0;
> > +	}
> 
> Error should be return -1.

OK, yes.


> > +      thread_arg->core_arg = core_arg;
> > +      thread_arg->note_offset = offset;
> > +      *thread_argp = thread_arg;
> > +      return tid;
> > +    }
> > +  return 0;
> > +}
> 
> > +static bool
> > +core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
> > +{
> > +  struct thread_arg *thread_arg = thread_arg_voidp;
> > +  struct core_arg *core_arg = thread_arg->core_arg;
> > +  Elf *core = core_arg->core;
> > +  size_t offset = thread_arg->note_offset;
> > +  GElf_Nhdr nhdr;
> > +  size_t name_offset;
> > +  size_t desc_offset;
> > +  Elf_Data *note_data = core_arg->note_data;
> > +  size_t nregs = ebl_frame_nregs (core_arg->ebl);
> > +  assert (offset < note_data->d_size);
> > +  size_t getnote_err = gelf_getnote (note_data, offset, &nhdr, &name_offset,
> > +				     &desc_offset);
> > +  assert (getnote_err != 0);
> 
> Too strong IMHO. Just __libdwfl_seterrno (DWFL_E_LIBELF) return false?
> core files can be corrupted.

In such case __libdwfl_attach_state_for_core would already fail the and
execution would not get here at all.  As the data is already read in memory it
is IMO really rather an assert().  Or do I miss something?


> > +  /* Do not check NAME for now, help broken Linux kernels.  */
> > +  const char *name = note_data->d_buf + name_offset;
> > +  const char *desc = note_data->d_buf + desc_offset;
> > +  GElf_Word regs_offset;
> > +  size_t nregloc;
> > +  const Ebl_Register_Location *reglocs;
> > +  size_t nitems;
> > +  const Ebl_Core_Item *items;
> > +  int core_note_err = ebl_core_note (core_arg->ebl, &nhdr, name, &regs_offset,
> > +				     &nregloc, &reglocs, &nitems, &items);
> > +  assert (core_note_err != 0);
> > +  assert (nhdr.n_type == NT_PRSTATUS);
> 
> As above too strong IMHO.

The same reason also here.


> > +  const Ebl_Core_Item *item;
> > +  for (item = items; item < items + nitems; item++)
> > +    if (strcmp (item->name, "pid") == 0)
> > +      break;
> > +  assert (item < items + nitems);
> > +  pid_t tid;
> > +  {
> > +    uint32_t val32 = *(const uint32_t *) (desc + item->offset);
> > +    val32 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> > +	     ? be32toh (val32) : le32toh (val32));
> > +    tid = (int32_t) val32;
> > +    eu_static_assert (sizeof val32 <= sizeof tid);
> > +  }
> > +  assert (tid == INTUSE(dwfl_thread_tid) (thread));
> 
> Can this happen with a corrupt core file? Then it is too strong IMHO.

No.  core_next_thread has already read the same number to figure out the TID.
Here the same core number is just read a second time.


> > +  desc += regs_offset;
> > +  for (size_t regloci = 0; regloci < nregloc; regloci++)
> > +    {
> > +      const Ebl_Register_Location *regloc = reglocs + regloci;
> > +      if (regloc->regno >= nregs)
> > +	continue;
> > +      assert (regloc->bits == 32 || regloc->bits == 64);
> > +      const char *reg_desc = desc + regloc->offset;
> > +      for (unsigned regno = regloc->regno;
> > +	   regno < MIN (regloc->regno + (regloc->count ?: 1U), nregs);
> > +	   regno++)
> > +	{
> > +	  /* PPC provides DWARF register 65 irrelevant for
> > +	     CFI which clashes with register 108 (LR) we need.
> > +	     LR (108) is provided earlier (in NT_PRSTATUS) than the # 65.
> > +	     FIXME: It depends now on their order in core notes.
> > +	     FIXME: It uses private function.  */
> > +	  if (dwfl_frame_reg_get (thread->unwound, regno, NULL))
> > +	    continue;
> 
> I dunno how (or if it is important) to fix this issue.
> The dwfl_frame_reg_get check does look irrelevant for other arches.
> But should be harmless.

This is an ugly hack which is only relevant PPC.  It could be moved at least
to the second branch for non-x86* archs.  But I somehow found it too small to
virtualize it to backends/libebl when it does not hurt other archs.


> > +	  Dwarf_Word val;
> > +	  switch (regloc->bits)
> > +	  {
> > +	    case 32:;
> > +	      uint32_t val32 = *(const uint32_t *) reg_desc;
> > +	      reg_desc += sizeof val32;
> > +	      val32 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> > +		       ? be32toh (val32) : le32toh (val32));
> > +	      /* Do a host width conversion.  */
> > +	      val = val32;
> > +	      break;
> > +	    case 64:;
> > +	      uint64_t val64 = *(const uint64_t *) reg_desc;
> > +	      reg_desc += sizeof val64;
> > +	      val64 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> > +		       ? be64toh (val64) : le64toh (val64));
> > +	      assert (sizeof (*thread->unwound->regs) == sizeof val64);
> > +	      val = val64;
> > +	      break;
> > +	    default:
> > +	      abort ();
> > +	  }
> > +	  /* Registers not valid for CFI are just ignored.  */
> > +	  INTUSE(dwfl_thread_state_registers) (thread, regno, 1, &val);
> > +	  reg_desc += regloc->pad;
> > +	}
> > +    }
> > +  return true;
> > +}
[...]
> > diff --git a/libdwfl/dwfl_frame_pid.c b/libdwfl/dwfl_frame_pid.c
> > +struct pid_arg
> > +{
> > +  DIR *dir;
> > +  size_t tids_attached_size, tids_attached_used;
> > +  pid_t *tids_attached;
> > +};
> 
> This might be overkill. I think the way we use the callbacks makes
> sure there is only one tid at a time attached.

Now, it wasn't...

I have deleted it now when at most one TID can be backtraced at a time.


> Except when there is a multi-threaded app that uses the same Dwfl in
> multiple threads.  I am not sure we fully support that.

At least from what IIRC Roland said despite some thread locking in elfutils it
does not really support multithreaded use.


> > +static bool
> > +ptrace_attach (pid_t tid)
> > +{
> > +  if (ptrace (PTRACE_ATTACH, tid, NULL, NULL) != 0)
> > +    return false;
> > +  /* FIXME: Handle missing SIGSTOP on old Linux kernels.  */
> 
> How old are these kernels?
> If this is before say 2.6.18 kernels (7 years ago now) then I
> think it isn't too urgent.

It happens in RHEL-6.  It is only very recent change where PTRACE_ATTACH can
no longer hang.


> > +  for (;;)
> > +    {
> > +      int status;
> > +      if (waitpid (tid, &status, __WALL) != tid || !WIFSTOPPED (status))
> > +	{
> > +	  ptrace (PTRACE_DETACH, tid, NULL, NULL);
> > +	  return false;
> > +	}
> > +      if (WSTOPSIG (status) == SIGSTOP)
> > +	break;
> > +      if (ptrace (PTRACE_CONT, tid, NULL,
> > +		  (void *) (uintptr_t) WSTOPSIG (status)) != 0)
> > +	{
> > +	  ptrace (PTRACE_DETACH, tid, NULL, NULL);
> > +	  return false;
> > +	}
> > +    }
> > +  return true;
> > +}
> 
> When used in pid_set_initial_registers the return value is also just
> false to indicate failure. And dwfl_thread_getframes will then just
> return -1 and not set dwfl_errno. Can we do a little better?
> Maybe use __libdwfl_seterrno (DWFL_E_ERRNO) when ptrace or waitpid fails?

Done with DWFL_E_ERRNO with blocks like:

      if (waitpid (tid, &status, __WALL) != tid || !WIFSTOPPED (status))
        {
          int saved_errno = errno;
          ptrace (PTRACE_DETACH, tid, NULL, NULL);
          errno = saved_errno;
          __libdwfl_seterrno (DWFL_E_ERRNO);
          return false;
        }


> > +static bool
> > +pid_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result, void *arg)
> > +{
> > +  struct pid_arg *pid_arg = arg;
> > +  assert (pid_arg->tids_attached_used > 0);
> > +  pid_t tid = pid_arg->tids_attached[0];
> 
> Aha, that works because you need an arbitrary tid of the pid that is
> attached. And you know there must be at least one or this callback
> wouldn't be called.

Yes.


> But is this really why the whole tids_attached datastructure is kept?

Yes.


> If so, then maybe our memory_read callback really should have a
> tid or Dwfl_Thread argument to indicate for which thread the memory
> read was intended.

It should not, it does not depend on the TID.  It would be wrong API.


> This thread must be attached already and then
> you could just use it here instead of having to keep track of all
> the tids_attached. In theory memory_read is generic per process,
> but in practice it does matter which thread the request was for
> it seems.

I think the logic is now easy with single 'tid_attached' variable when at most
one thread can be PTRACE_ATTACH-ed.


> > +  Dwfl_Process *process = dwfl->process;
> > +  if (process->ebl->class == ELFCLASS64)
> > +    {
> > +      errno = 0;
> > +      *result = ptrace (PTRACE_PEEKDATA, tid, (void *) (uintptr_t) addr, NULL);
> > +      return errno == 0;
> > +    }
> > +#if SIZEOF_LONG == 8
> > +  /* We do not care about reads unaliged to 4 bytes boundary.
> > +     But 0x...ffc read of 8 bytes could overrun a page.  */
> > +  bool lowered = (addr & 4) != 0;
> > +  if (lowered)
> > +    addr -= 4;
> > +#endif /* SIZEOF_LONG == 8 */
> > +  errno = 0;
> > +  *result = ptrace (PTRACE_PEEKDATA, tid, (void *) (uintptr_t) addr, NULL);
> > +  if (errno != 0)
> > +    return false;
> > +#if SIZEOF_LONG == 8
> > +# if BYTE_ORDER == BIG_ENDIAN
> > +  if (! lowered)
> > +    *result >>= 32;
> > +# else
> > +  if (lowered)
> > +    *result >>= 32;
> > +# endif
> > +#endif /* SIZEOF_LONG == 8 */
> > +  *result &= 0xffffffff;
> > +  return true;
> > +}
> 
> > +static pid_t
> > +pid_next_thread (Dwfl *dwfl __attribute__ ((unused)),
> > +		 Dwfl_Thread *nthread __attribute__ ((unused)), void *dwfl_arg,
> > +		 void **thread_argp)
> > +{
> > +  struct pid_arg *pid_arg = dwfl_arg;
> > +  struct dirent *dirent;
> > +  do
> > +    {
> > +      errno = 0;
> > +      dirent = readdir (pid_arg->dir);
> > +      if (dirent == NULL)
> > +	return errno == 0 ? 0 : -1;
> 
> if (errno != 0) __libdwfl_seterrno (DWFL_E_ERRNO) ?

Yes.

      if (dirent == NULL)
        {
          if (errno != 0)
            {
              __libdwfl_seterrno (DWFL_E_ERRNO);
              return -1;
            }
          return 0;
        }


> > +    }
> > +  while (strcmp (dirent->d_name, ".") == 0
> > +	 || strcmp (dirent->d_name, "..") == 0);
> 
> I assume readdir makes sure there two are always first?

I do not see such guarantee in Fedora man 3 readdir and neither in:
	http://pubs.opengroup.org/onlinepubs/007908799/xsh/readdir.html


> > +  char *end;
> > +  errno = 0;
> > +  long tidl = strtol (dirent->d_name, &end, 10);
> > +  if (errno != 0)
> > +    return -1;
> 
>  __libdwfl_seterrno (DWFL_E_ERRNO) ?

Yes.


> > +  pid_t tid = tidl;
> > +  if (tidl <= 0 || (end && *end) || tid != tidl)
> > +    return -1;
> 
> O, weird... Yeah, that would not be good, but an assert is
> probably not good either in case something weird shows up in the dir.
> Maybe DWFL_E_PARSE_PROC?

Yes.


> > +  *thread_argp = dwfl_arg;
> > +  return tid;
> > +}
> 
> > +static bool
> > +pid_thread_state_registers_cb (const int firstreg,
> > +			       unsigned nregs,
> > +			       const Dwarf_Word *regs,
> > +			       void *arg)
> > +{
> > +  Dwfl_Thread *thread = (Dwfl_Thread *) arg;
> > +  return INTUSE(dwfl_thread_state_registers) (thread, firstreg, nregs, regs);
> > +}
> 
> Might want to add a comment this is the ebl_set_initial_registers_tid
> callback.

Added:
	/* Implement the ebl_set_initial_registers_tid setfunc callback.  */


> > +static bool
> > +pid_set_initial_registers (Dwfl_Thread *thread, void *thread_arg)
> > +{
> > +  struct pid_arg *pid_arg = thread_arg;
> > +  pid_t tid = INTUSE(dwfl_thread_tid) (thread);
> > +  if (! ptrace_attach (tid))
> > +    return false;
> > +  if (pid_arg->tids_attached_used == pid_arg->tids_attached_size)
> > +    {
> > +      pid_arg->tids_attached_size *= 2;
> > +      pid_arg->tids_attached_size = MAX (64, pid_arg->tids_attached_size);
> > +      pid_arg->tids_attached = realloc (pid_arg->tids_attached,
> > +					(pid_arg->tids_attached_size
> > +					 * sizeof *pid_arg->tids_attached));
> 
> In theory the realloc could fail and return NULL.

The code is no longer present.


> > +    }
> > +  pid_arg->tids_attached[pid_arg->tids_attached_used++] = tid;
> > +  Dwfl_Process *process = thread->process;
> > +  Ebl *ebl = process->ebl;
> > +  return ebl_set_initial_registers_tid (ebl, tid,
> > +					pid_thread_state_registers_cb, thread);
> > +}
[...]
> > +bool
> > +internal_function
> > +__libdwfl_attach_state_for_pid (Dwfl *dwfl, pid_t pid)
> > +{
> > +  char dirname[64];
> > +  int i = snprintf (dirname, sizeof (dirname), "/proc/%ld/task", (long) pid);
> > +  assert (i > 0 && i < (ssize_t) sizeof (dirname) - 1);
> 
> 30 chars is enough if pids are actually long, otherwise 21 should be enough
> for everybody. (MAX_INT is 2147483647, MAX_LONG is 9223372036854775807.)

I think at least dirname[32] is required for:
/proc/-9223372036854775808/taskZ

Also stack frame is 16-bytes aligned.  It could probably be [32] but I really
do not think it matters to discuss this.


> > +  DIR *dir = opendir (dirname);
> > +  if (dir == NULL)
> > +    {
> > +      __libdwfl_seterrno (DWFL_E_PARSE_PROC);
> > +      return NULL;
> > +    }
> 
> That should be return false.

Yes.

> Do you need a special dwfl_errno for this or can you use DWFL_E_ERRNO?

Used DWFL_E_ERRNO.


> > +  struct pid_arg *pid_arg = malloc (sizeof *pid_arg);
> > +  if (pid_arg == NULL)
> > +    {
> > +      closedir (dir);
> > +      __libdwfl_seterrno (DWFL_E_NOMEM);
> > +      return false;
> > +    }
> > +  pid_arg->dir = dir;
> > +  pid_arg->tids_attached_size = 0;
> > +  pid_arg->tids_attached_used = 0;
> > +  pid_arg->tids_attached = NULL;
> > +  if (! INTUSE(dwfl_attach_state) (dwfl, EM_NONE, pid, &pid_thread_callbacks,
> > +				   pid_arg))
> > +    {
> > +      free (pid_arg);
> > +      return false;
> > +    }
> 
> Missing closedir (dir)?

Right.


> > +  return true;
> > +}


Thanks,
Jan


diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index c04f0e9..567d891 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -58,7 +58,10 @@ core_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
   assert (core != NULL);
   static size_t phnum;
   if (elf_getphdrnum (core, &phnum) < 0)
-    return false;
+    {
+      __libdwfl_seterrno (DWFL_E_LIBELF);
+      return false;
+    }
   for (size_t cnt = 0; cnt < phnum; ++cnt)
     {
       GElf_Phdr phdr_mem, *phdr = gelf_getphdr (core, cnt, &phdr_mem);
@@ -75,7 +78,10 @@ core_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
       data = elf_getdata_rawchunk (core, phdr->p_offset + addr - start,
 				   bytes, ELF_T_ADDR);
       if (data == NULL)
-	return false;
+	{
+	  __libdwfl_seterrno (DWFL_E_LIBELF);
+	  return false;
+	}
       assert (data->d_size == bytes);
       /* FIXME: Currently any arch supported for unwinding supports
 	 unaligned access.  */
@@ -85,6 +91,7 @@ core_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
 	*result = *(const uint32_t *) data->d_buf;
       return true;
     }
+  __libdwfl_seterrno (DWFL_E_ADDR_OUTOFRANGE);
   return false;
 }
 
@@ -136,7 +143,7 @@ core_next_thread (Dwfl *dwfl __attribute__ ((unused)),
       if (thread_arg == NULL)
 	{
 	  __libdwfl_seterrno (DWFL_E_NOMEM);
-	  return 0;
+	  return -1;
 	}
       thread_arg->core_arg = core_arg;
       thread_arg->note_offset = offset;
@@ -158,6 +165,7 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
   size_t desc_offset;
   Elf_Data *note_data = core_arg->note_data;
   size_t nregs = ebl_frame_nregs (core_arg->ebl);
+  assert (nregs > 0);
   assert (offset < note_data->d_size);
   size_t getnote_err = gelf_getnote (note_data, offset, &nhdr, &name_offset,
 				     &desc_offset);
diff --git a/libdwfl/linux-pid-attach.c b/libdwfl/linux-pid-attach.c
index 22101b7..2892052 100644
--- a/libdwfl/linux-pid-attach.c
+++ b/libdwfl/linux-pid-attach.c
@@ -38,22 +38,28 @@
 struct pid_arg
 {
   DIR *dir;
-  size_t tids_attached_size, tids_attached_used;
-  pid_t *tids_attached;
+  /* It is 0 if not used.  */
+  pid_t tid_attached;
 };
 
 static bool
 ptrace_attach (pid_t tid)
 {
   if (ptrace (PTRACE_ATTACH, tid, NULL, NULL) != 0)
-    return false;
+    {
+      __libdwfl_seterrno (DWFL_E_ERRNO);
+      return false;
+    }
   /* FIXME: Handle missing SIGSTOP on old Linux kernels.  */
   for (;;)
     {
       int status;
       if (waitpid (tid, &status, __WALL) != tid || !WIFSTOPPED (status))
 	{
+	  int saved_errno = errno;
 	  ptrace (PTRACE_DETACH, tid, NULL, NULL);
+	  errno = saved_errno;
+	  __libdwfl_seterrno (DWFL_E_ERRNO);
 	  return false;
 	}
       if (WSTOPSIG (status) == SIGSTOP)
@@ -61,7 +67,10 @@ ptrace_attach (pid_t tid)
       if (ptrace (PTRACE_CONT, tid, NULL,
 		  (void *) (uintptr_t) WSTOPSIG (status)) != 0)
 	{
+	  int saved_errno = errno;
 	  ptrace (PTRACE_DETACH, tid, NULL, NULL);
+	  errno = saved_errno;
+	  __libdwfl_seterrno (DWFL_E_ERRNO);
 	  return false;
 	}
     }
@@ -72,8 +81,8 @@ static bool
 pid_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result, void *arg)
 {
   struct pid_arg *pid_arg = arg;
-  assert (pid_arg->tids_attached_used > 0);
-  pid_t tid = pid_arg->tids_attached[0];
+  pid_t tid = pid_arg->tid_attached;
+  assert (tid > 0);
   Dwfl_Process *process = dwfl->process;
   if (process->ebl->class == ELFCLASS64)
     {
@@ -117,7 +126,14 @@ pid_next_thread (Dwfl *dwfl __attribute__ ((unused)),
       errno = 0;
       dirent = readdir (pid_arg->dir);
       if (dirent == NULL)
-	return errno == 0 ? 0 : -1;
+	{
+	  if (errno != 0)
+	    {
+	      __libdwfl_seterrno (DWFL_E_ERRNO);
+	      return -1;
+	    }
+	  return 0;
+	}
     }
   while (strcmp (dirent->d_name, ".") == 0
 	 || strcmp (dirent->d_name, "..") == 0);
@@ -125,14 +141,22 @@ pid_next_thread (Dwfl *dwfl __attribute__ ((unused)),
   errno = 0;
   long tidl = strtol (dirent->d_name, &end, 10);
   if (errno != 0)
-    return -1;
+    {
+      __libdwfl_seterrno (DWFL_E_ERRNO);
+      return -1;
+    }
   pid_t tid = tidl;
   if (tidl <= 0 || (end && *end) || tid != tidl)
-    return -1;
+    {
+      __libdwfl_seterrno (DWFL_E_PARSE_PROC);
+      return -1;
+    }
   *thread_argp = dwfl_arg;
   return tid;
 }
 
+/* Implement the ebl_set_initial_registers_tid setfunc callback.  */
+
 static bool
 pid_thread_state_registers_cb (const int firstreg,
 			       unsigned nregs,
@@ -147,18 +171,11 @@ static bool
 pid_set_initial_registers (Dwfl_Thread *thread, void *thread_arg)
 {
   struct pid_arg *pid_arg = thread_arg;
+  assert (! pid_arg->tid_attached);
   pid_t tid = INTUSE(dwfl_thread_tid) (thread);
   if (! ptrace_attach (tid))
     return false;
-  if (pid_arg->tids_attached_used == pid_arg->tids_attached_size)
-    {
-      pid_arg->tids_attached_size *= 2;
-      pid_arg->tids_attached_size = MAX (64, pid_arg->tids_attached_size);
-      pid_arg->tids_attached = realloc (pid_arg->tids_attached,
-					(pid_arg->tids_attached_size
-					 * sizeof *pid_arg->tids_attached));
-    }
-  pid_arg->tids_attached[pid_arg->tids_attached_used++] = tid;
+  pid_arg->tid_attached = tid;
   Dwfl_Process *process = thread->process;
   Ebl *ebl = process->ebl;
   return ebl_set_initial_registers_tid (ebl, tid,
@@ -170,7 +187,6 @@ pid_detach (Dwfl *dwfl __attribute__ ((unused)), void *dwfl_arg)
 {
   struct pid_arg *pid_arg = dwfl_arg;
   closedir (pid_arg->dir);
-  free (pid_arg->tids_attached);
   free (pid_arg);
 }
 
@@ -179,13 +195,8 @@ pid_thread_detach (Dwfl_Thread *thread, void *thread_arg)
 {
   struct pid_arg *pid_arg = thread_arg;
   pid_t tid = INTUSE(dwfl_thread_tid) (thread);
-  size_t ix;
-  for (ix = 0; ix < pid_arg->tids_attached_used; ix++)
-    if (pid_arg->tids_attached[ix] == tid)
-      break;
-  assert (ix < pid_arg->tids_attached_used);
-  pid_arg->tids_attached[ix]
-    = pid_arg->tids_attached[--pid_arg->tids_attached_used];
+  assert (pid_arg->tid_attached == tid);
+  pid_arg->tid_attached = 0;
   ptrace (PTRACE_DETACH, tid, NULL, NULL);
 }
 
@@ -208,8 +219,8 @@ __libdwfl_attach_state_for_pid (Dwfl *dwfl, pid_t pid)
   DIR *dir = opendir (dirname);
   if (dir == NULL)
     {
-      __libdwfl_seterrno (DWFL_E_PARSE_PROC);
-      return NULL;
+      __libdwfl_seterrno (DWFL_E_ERRNO);
+      return false;
     }
   struct pid_arg *pid_arg = malloc (sizeof *pid_arg);
   if (pid_arg == NULL)
@@ -219,12 +230,11 @@ __libdwfl_attach_state_for_pid (Dwfl *dwfl, pid_t pid)
       return false;
     }
   pid_arg->dir = dir;
-  pid_arg->tids_attached_size = 0;
-  pid_arg->tids_attached_used = 0;
-  pid_arg->tids_attached = NULL;
+  pid_arg->tid_attached = 0;
   if (! INTUSE(dwfl_attach_state) (dwfl, EM_NONE, pid, &pid_thread_callbacks,
 				   pid_arg))
     {
+      closedir (dir);
       free (pid_arg);
       return false;
     }