Various fixes to the Darwin port...

Various fixes to the Darwin port...

[Joel Brobecker]

I thought I'd spend a few hours on the Darwin port and see where
we stand, and try to fix the most severe of our known problems.
I ended up spending a little more time than I wanted, but it wasn't
completely in vain.

To follow are 4 patches that should help a bit in some situations.

[Darwin 3/4] [mach-o] get rid of current_oso global

[Joel Brobecker]

This fixes a crash that happens when loading symbols for a new
executable, for instance:

    % gdb-head foo
    (gdb) start
    Temporary breakpoint 1 at 0x100000ac2: file foo.adb, line 3.
    Starting program: /[...]/foo

    Temporary breakpoint 1, foo () at foo.adb:3
    3          null;
    (gdb) kill
    Kill the program being debugged? (y or n) y
    (gdb) file foo
    Load new symbol table from "/[...]/foo"? (y or n) y
    Reading symbols from /[...]/foo...
    /[...]/gdb/machoread.c:392: internal-error: macho_add_oso_symfile: Assertion
 `current_oso.symbol_table == NULL' failed.
    A problem internal to GDB has been detected,
    [...]

The current_oso global was used to store the OSO's symbol table
in order to relocate common symbols.  But it is also making the
assumption that all sections are going to be immediately relocated
as macho_add_oso_symfile does:
  - Initialize the current_oso
  - Use it through symbol_file_add_from_bfd
  - Reset the current_oso (to all fields NULL)

What actually happens is that the .debug_frame section gets read
lazily, and thus relocated at a later time.  This relocation causes
current_oso.symbol_table to be initialized (see macho_symfile_relocate)
again.  And this eventually causes to trip the...

        gdb_assert (current_oso.symbol_table == NULL);

...assertion to fail because the symbol_table was never free'ed.
To be complete, this happens because macho_symfile_relocate was
called outside of macho_add_oso_symfile's control, where the
set-global/use/unset-global dance happens.

But it looks like keeping this global around is not necessary, as
this symbol table is only used to relocate the common symbols.
We can do that prior to relocating the rest of the symbols.

gdb/ChangeLog:

        * machoread.c (struct macho_oso_data): Delete.
        (current_oso): Delete.
        (macho_relocate_common_syms): New function, mostly extracted
        out of
        (macho_add_oso_symfile): Call macho_relocate_common_syms.
        Remove code that sets and unset current_oso.
        (macho_symfile_relocate): Delete handling of common symbols,
        now moved to macho_relocate_common_syms.

Tested on x86_64-darwin.  Checked in.

---
 gdb/ChangeLog   |   11 +++++
 gdb/machoread.c |  132 ++++++++++++++++++++++++++----------------------------
 2 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ce522fb..3e306ad 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,16 @@
 2011-07-01  Joel Brobecker  <brobecker@adacore.com>
 
+	* machoread.c (struct macho_oso_data): Delete.
+	(current_oso): Delete.
+	(macho_relocate_common_syms): New function, mostly extracted
+	out of
+	(macho_add_oso_symfile): Call macho_relocate_common_syms.
+	Remove code that sets and unset current_oso.
+	(macho_symfile_relocate): Delete handling of common symbols,
+	now moved to macho_relocate_common_syms.
+
+2011-07-01  Joel Brobecker  <brobecker@adacore.com>
+
 	* darwin-nat.c (darwin_ptrace): Add documentation.
 	Set errno to zero before calling ptrace.  If ptrace returns
 	-1 and errno is zero, then change then return zero.
diff --git a/gdb/machoread.c b/gdb/machoread.c
index 1cfa21e..e1c03fd 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -71,23 +71,6 @@ oso_el;
 DEF_VEC_O (oso_el);
 static VEC (oso_el) *oso_vector;
 
-struct macho_oso_data
-{
-  /* Per objfile symbol table.  This is used to apply relocation to sections
-     It is loaded only once, then relocated, and free after sections are
-     relocated.  */
-  asymbol **symbol_table;
-
-  /* The offsets for this objfile.  Used to relocate the symbol_table.  */
-  struct oso_el *oso;
-
-  struct objfile *main_objfile;
-};
-
-/* Data for OSO being processed.  */
-
-static struct macho_oso_data current_oso;
-
 static void
 macho_new_init (struct objfile *objfile)
 {
@@ -308,6 +291,65 @@ oso_el_compare_name (const void *vl, const void *vr)
   return strcmp (l->name, r->name);
 }
 
+/* Relocate all of ABFD's common symbols immediately.
+
+   This modifies the section and address of all common symbols to become
+   absolute symbols with their address set to match the address given by
+   the main objfile's symbol table.
+
+   The reason why the common symbols have to be handled separately
+   is because relocation is performed relative to section start.
+   But there is no section in this case.  So the "relocation" of
+   these common symbols is performed by finding their address in
+   the main objfile's symbol table, where we know it's been relocated.
+
+   ABFD is an OSO's bfd.
+   MAIN_OBJFILE is the object file from which the OSO is a part.  */
+
+static void
+macho_relocate_common_syms(bfd *abfd, struct objfile *main_objfile)
+{
+  int storage;
+  int i;
+  char leading_char;
+  asymbol **symbol_table;
+
+  storage = bfd_get_symtab_upper_bound (abfd);
+  symbol_table = (asymbol **) xmalloc (storage);
+  bfd_canonicalize_symtab (abfd, symbol_table);
+
+  leading_char = bfd_get_symbol_leading_char (abfd);
+
+  for (i = 0; symbol_table[i]; i++)
+    {
+      asymbol *sym = symbol_table[i];
+
+      if (bfd_is_com_section (sym->section))
+	{
+	  /* This one must be solved.  */
+	  struct minimal_symbol *msym;
+	  const char *name = sym->name;
+
+	  if (name[0] == leading_char)
+	    name++;
+
+	  msym = lookup_minimal_symbol (name, NULL, main_objfile);
+	  if (msym == NULL)
+	    {
+	      warning (_("can't find symbol '%s' in minsymtab"), name);
+	      continue;
+	    }
+	  else
+	    {
+	      sym->section = &bfd_abs_section;
+	      sym->value = SYMBOL_VALUE_ADDRESS (msym);
+	    }
+	}
+    }
+
+  xfree (symbol_table);
+}
+
 /* Add an oso file as a symbol file.  */
 
 static void
@@ -384,14 +426,16 @@ macho_add_oso_symfile (oso_el *oso, bfd *abfd,
                            core_addr_to_string (vma), sectname);
     }
 
+  /* Deal with the common symbols now, as they need special handing.
+     Doing it now sets them up so that we don't accidently try to
+     relocate them during the normal relocation phase.  */
+  macho_relocate_common_syms (abfd, main_objfile);
+
   /* Make sure that the filename was malloc'ed.  The current filename comes
      either from an OSO symbol name or from an archive name.  Memory for both
      is not managed by gdb.  */
   abfd->filename = xstrdup (abfd->filename);
 
-  gdb_assert (current_oso.symbol_table == NULL);
-  current_oso.main_objfile = main_objfile;
-
   /* We need to clear SYMFILE_MAINLINE to avoid interractive question
      from symfile.c:symbol_file_add_with_addrs_or_offsets.  */
   objfile = symbol_file_add_from_bfd
@@ -399,13 +443,6 @@ macho_add_oso_symfile (oso_el *oso, bfd *abfd,
      main_objfile->flags & (OBJF_REORDERED | OBJF_SHARED
 			    | OBJF_READNOW | OBJF_USERLOADED),
      main_objfile);
-
-  current_oso.main_objfile = NULL;
-  if (current_oso.symbol_table)
-    {
-      xfree (current_oso.symbol_table);
-      current_oso.symbol_table = NULL;
-    }
 }
 
 /* Read symbols from the vector of oso files.  */
@@ -731,47 +768,6 @@ macho_symfile_relocate (struct objfile *objfile, asection *sectp,
     printf_unfiltered (_("Relocate section '%s' of %s\n"),
                        sectp->name, objfile->name);
 
-  if (current_oso.symbol_table == NULL)
-    {
-      int storage;
-      int i;
-      char leading_char;
-
-      storage = bfd_get_symtab_upper_bound (abfd);
-      current_oso.symbol_table = (asymbol **) xmalloc (storage);
-      bfd_canonicalize_symtab (abfd, current_oso.symbol_table);
-
-      leading_char = bfd_get_symbol_leading_char (abfd);
-
-      for (i = 0; current_oso.symbol_table[i]; i++)
-        {
-          asymbol *sym = current_oso.symbol_table[i];
-
-          if (bfd_is_com_section (sym->section))
-            {
-              /* This one must be solved.  */
-              struct minimal_symbol *msym;
-              const char *name = sym->name;
-
-              if (name[0] == leading_char)
-                name++;
-
-              msym = lookup_minimal_symbol
-                (name, NULL, current_oso.main_objfile);
-              if (msym == NULL)
-                {
-                  warning (_("can't find symbol '%s' in minsymtab"), name);
-                  continue;
-                }
-              else
-                {
-                  sym->section = &bfd_abs_section;
-                  sym->value = SYMBOL_VALUE_ADDRESS (msym);
-                }
-            }
-        }
-    }
-
   return bfd_simple_get_relocated_section_contents (abfd, sectp, buf, NULL);
 }
 
-- 
1.7.1

[Darwin 2/4] Do not crash (failed assertion) after PT_KILL ptrace error

[Joel Brobecker]

This is something I noticed while reading the code.  Putting an
assertion that the PT_KILL ptrace call never returns an error
is too strong.  It might not be a debugger bug that caused the
PT_KILL ptrace operation to fail, so a failed-assertion "crash"
would not be justified.  It also seems easy enough to continue
and get ready for the next debugging session.  So this patch
changes the assertion into a warning.

This patch also tries to handle the case where ptrace return -1,
but left errno set to zero.  According to the ptrace man page,
it is possible for some ptrace operations to return -1 in non-error
situations, and to detect those situations, it explains that errno
should be set prior to calling ptrace, and then checked again after.

gdb/ChangeLog:

        * darwin-nat.c (darwin_ptrace): Add documentation.
        Set errno to zero before calling ptrace.  If ptrace returns
        -1 and errno is zero, then change then return zero.
        (darwin_kill_inferior): Issue a warning instead of triggering
        a failed assertion when the PT_KILL ptrace operations returned
        nonzero.

Tested on x86_64-darwin.  Checked in.

---
 gdb/ChangeLog    |    9 +++++++++
 gdb/darwin-nat.c |   17 ++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5be8066..ce522fb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2011-07-01  Joel Brobecker  <brobecker@adacore.com>
 
+	* darwin-nat.c (darwin_ptrace): Add documentation.
+	Set errno to zero before calling ptrace.  If ptrace returns
+	-1 and errno is zero, then change then return zero.
+	(darwin_kill_inferior): Issue a warning instead of triggering
+	a failed assertion when the PT_KILL ptrace operations returned
+	nonzero.
+
+2011-07-01  Joel Brobecker  <brobecker@adacore.com>
+
 	* darwin-nat.c (darwin_detach): Call darwin_resume_inferior
 	only when inf->private->no_ptrace.
 
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index fc5263a..27c6e2c 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -233,13 +233,25 @@ unparse_exception_type (unsigned int i)
     }
 }
 
+/* Set errno to zero, and then call ptrace with the given arguments.
+   If inferior debugging traces are on, then also print a debug
+   trace.
+
+   The returned value is the same as the value returned by ptrace,
+   except in the case where that value is -1 but errno is zero.
+   This case is documented to be a non-error situation, so we
+   return zero in that case. */
+
 static int
 darwin_ptrace (const char *name,
 	       int request, int pid, PTRACE_TYPE_ARG3 arg3, int arg4)
 {
   int ret;
 
+  errno = 0;
   ret = ptrace (request, pid, (caddr_t) arg3, arg4);
+  if (ret == -1 && errno == 0)
+    ret = 0;
 
   inferior_debug (4, _("ptrace (%s, %d, 0x%x, %d): %d (%s)\n"),
                   name, pid, arg3, arg4, ret,
@@ -1301,7 +1313,10 @@ darwin_kill_inferior (struct target_ops *ops)
       darwin_stop_inferior (inf);
 
       res = PTRACE (PT_KILL, inf->pid, 0, 0);
-      gdb_assert (res == 0);
+      if (res != 0)
+        warning (_("Failed to kill inferior: ptrace returned %d "
+	           "[%s] (pid=%d)"),
+		 res, safe_strerror (errno), inf->pid);
 
       darwin_reply_to_all_pending_messages (inf);
 
-- 
1.7.1

Re: [Darwin 2/4] Do not crash (failed assertion) after PT_KILL ptrace error

[Mark Kettenis]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Fri,  1 Jul 2011 12:03:24 -0700
> 
> This is something I noticed while reading the code.  Putting an
> assertion that the PT_KILL ptrace call never returns an error
> is too strong.  It might not be a debugger bug that caused the
> PT_KILL ptrace operation to fail, so a failed-assertion "crash"
> would not be justified.  It also seems easy enough to continue
> and get ready for the next debugging session.  So this patch
> changes the assertion into a warning.
> 
> This patch also tries to handle the case where ptrace return -1,
> but left errno set to zero.  According to the ptrace man page,
> it is possible for some ptrace operations to return -1 in non-error
> situations, and to detect those situations, it explains that errno
> should be set prior to calling ptrace, and then checked again after.

I think I disagree here.  PT_KILL should only fail if you pass it the
wrong process ID.  So unless there is an OS bug of some sorts, this is
going to be a GDB internal error.  Do you have actual evidence there
is a kernel bug here?

> gdb/ChangeLog:
> 
>         * darwin-nat.c (darwin_ptrace): Add documentation.
>         Set errno to zero before calling ptrace.  If ptrace returns
>         -1 and errno is zero, then change then return zero.
>         (darwin_kill_inferior): Issue a warning instead of triggering
>         a failed assertion when the PT_KILL ptrace operations returned
>         nonzero.

Re: [Darwin 2/4] Do not crash (failed assertion) after PT_KILL ptrace error

[Joel Brobecker]

> I think I disagree here.  PT_KILL should only fail if you pass it the
> wrong process ID.  So unless there is an OS bug of some sorts, this is
> going to be a GDB internal error.  Do you have actual evidence there
> is a kernel bug here?

It's true I don't. And I agree that the error could be an internal error,
but it could just as well be an error in the kernel too. So I still think
that an internal error/assert is too strong. What's more, we can still
continue without any degradation in the debugger performance.  We just
failed one action which isn't critical to GDB's internal state.

-- 
Joel

Re: [Darwin 2/4] Do not crash (failed assertion) after PT_KILL ptrace error

[Joel Brobecker]

> I think I disagree here.  PT_KILL should only fail if you pass it the
> wrong process ID.  So unless there is an OS bug of some sorts, this is
> going to be a GDB internal error.  Do you have actual evidence there
> is a kernel bug here?

It's true I don't. And I agree that the error could be an internal error,
but it could just as well be an error in the kernel too. So I still think
that an internal error/assert is too strong. What's more, we can still
continue without any degradation in the debugger performance.  We just
failed one action which isn't critical to GDB's internal state.

-- 
Joel

[Darwin 1/4] detach: Do not resume inferior after ptrace detach

[Joel Brobecker]

When trying to detach from an inferior that we start from the debugger,
GDB prints the following warning:

    (gdb) detach
    Detaching from program: /[...]/foo, process 74593
    warning: Mach error at "/[...]/darwin-nat.c:445" in function "darwin_resume_inferior": (os/kern) failure (0x5)

The warning comes from the following code in darwin_detach:

     darwin_resume_inferior (inf);

This is because the process has already been resumed by the
PT_DETACH ptrace operation that has just been performed, as
documented by the ptrace(2) man page.

On the other hand, when trying to detach from an inferior that
was started outside of debugger control (thus after having attached
the debugger to that inferior), things go smoothly.  That's because
we don't use ptrace to control the process in that case, and so
the resume is perfectly justified.

This patch makes sure that we resume the inferior during the detach
only when we're NOT using ptrace.

gdb/ChangeLog:

        * darwin-nat.c (darwin_detach): Call darwin_resume_inferior
        only when inf->private->no_ptrace.

Tested on x86_64-darwin.  Checked in.

---
 gdb/ChangeLog    |    5 +++++
 gdb/darwin-nat.c |    6 +++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c37c9a9..5be8066 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-07-01  Joel Brobecker  <brobecker@adacore.com>
 
+	* darwin-nat.c (darwin_detach): Call darwin_resume_inferior
+	only when inf->private->no_ptrace.
+
+2011-07-01  Joel Brobecker  <brobecker@adacore.com>
+
 	* ada-lang.c (print_it_exception): Print temporary catchpoints
 	as "Temporary catchpoint".
 	(print_mention_exception): Likewise.
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 7be85d5..fc5263a 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1601,7 +1601,11 @@ darwin_detach (struct target_ops *ops, char *args, int from_tty)
 
   darwin_reply_to_all_pending_messages (inf);
 
-  darwin_resume_inferior (inf);
+  /* When using ptrace, we have just performed a PT_DETACH, which
+     resumes the inferior.  On the other hand, when we are not using
+     ptrace, we need to resume its execution ourselves.  */
+  if (inf->private->no_ptrace)
+    darwin_resume_inferior (inf);
 
   darwin_mourn_inferior (ops);
 }
-- 
1.7.1

[Darwin 4/4] remove comment in machoread.c (macho_symfile_read)

[Joel Brobecker]

Does not seem to be applicable to the current code anymore.

gdb/ChangeLog:

        * machoread.c (macho_symfile_read): Delete OBE comment.

Checked in.

---
 gdb/ChangeLog   |    4 ++++
 gdb/machoread.c |    3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3e306ad..02f11c5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2011-07-01  Joel Brobecker  <brobecker@adacore.com>
 
+	* machoread.c (macho_symfile_read): Delete OBE comment.
+
+2011-07-01  Joel Brobecker  <brobecker@adacore.com>
+
 	* machoread.c (struct macho_oso_data): Delete.
 	(current_oso): Delete.
 	(macho_relocate_common_syms): New function, mostly extracted
diff --git a/gdb/machoread.c b/gdb/machoread.c
index e1c03fd..a454cd4 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -745,9 +745,6 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
       dwarf2_build_psymtabs (objfile);
     }
 
-  /* Do not try to read .eh_frame/.debug_frame as they are not relocated
-     and dwarf2_build_frame_info cannot deal with unrelocated sections.  */
-
   /* Then the oso.  */
   if (oso_vector != NULL)
     macho_symfile_read_all_oso (objfile, symfile_flags);
-- 
1.7.1