[PATCH 0/3 v4] Common code cleanups (part 1)

[PATCH 0/3 v4] Common code cleanups (part 1)

[Gary Benson]

Hi all,

This series contains the lateset versions of the first three patches
from the common code cleanups series I've posted previously.  I intend
to rework and resubmit the remaining patches, but these three are I
think ready to go in and it would be nice to get them out of my hair.

Patch 1/3 (Introduce common/errors.h) has had everything to do with
fatal removed.  The remaining functions have updated documentation
which I hope is now ok.  Everything else is unchanged from the
previous versison.

Patch 2/3 (Remove some GDBSERVER checks from linux-ptrace) has been
updated to reverse some logic to avoid indentation changes which will
likely be reversed as further code is merged.  It has also been
updated to not mutate the additional_flags static global.

Patch 3/3 (Make gdbserver CORE_ADDR unsigned) is unchanged from the
previous version posted to this list.  It's been approved already,
but I'm including it here for completeness.

Built and regtested on RHEL6.5 x86_64.

Is this ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 2/3 v4] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
Currently the code uses a compile-time check to decide whether some
flags should be used.  This changes the code to instead let users of
the module specify an additional set of flags; and then changes gdb's
linux-nat.c to call this function.  At some later date, when the back
ends are fully merged, we will be able to remove this function again.

gdb/
2014-07-24  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* nat/linux-ptrace.c (additional_flags): New global.
	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
	additional_flags; don't check GDBSERVER.
	(linux_ptrace_set_additional_flags): New function.
	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
	Declare.
	* linux-nat.c (_initialize_linux_nat): Call
	linux_ptrace_set_additional_flags.
---
 gdb/ChangeLog          |   12 ++++++++++
 gdb/linux-nat.c        |    8 ++++++
 gdb/nat/linux-ptrace.c |   57 ++++++++++++++++++++++++++---------------------
 gdb/nat/linux-ptrace.h |    1 +
 4 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b50a88e..7db1b3d 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -5033,6 +5033,14 @@ Enables printf debugging output."),
   sigdelset (&suspend_mask, SIGCHLD);
 
   sigemptyset (&blocked_mask);
+
+  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
+     support read-only process state.  */
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+				     | PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
 }
 \f
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 3ad2113..7893073 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -37,6 +37,10 @@
    there are no supported features.  */
 static int current_ptrace_options = -1;
 
+/* Additional flags to test.  */
+
+static int additional_flags;
+
 /* Find all possible reasons we could fail to attach PID and append
    these as strings to the already initialized BUFFER.  '\0'
    termination of BUFFER must be done by the caller.  */
@@ -359,16 +363,16 @@ linux_check_ptrace_features (void)
 static void
 linux_test_for_tracesysgood (int child_pid)
 {
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACESYSGOOD.  */
-#else
   int ret;
 
+  if ((additional_flags & PTRACE_O_TRACESYSGOOD) == 0)
+    return;
+
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
 		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+
   if (ret == 0)
     current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
-#endif
 }
 
 /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
@@ -388,16 +392,15 @@ linux_test_for_tracefork (int child_pid)
   if (ret != 0)
     return;
 
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
-#else
-  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
-				    | PTRACE_O_TRACEVFORKDONE));
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
-#endif
+  if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
+    {
+      /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+					| PTRACE_O_TRACEVFORKDONE));
+      if (ret == 0)
+	current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
+    }
 
   /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
      don't know for sure that the feature is available; old
@@ -433,18 +436,9 @@ linux_test_for_tracefork (int child_pid)
 
 	  /* We got the PID from the grandchild, which means fork
 	     tracing is supported.  */
-#ifdef GDBSERVER
-	  /* Do not enable all the options for now since gdbserver does not
-	     properly support them.  This restriction will be lifted when
-	     gdbserver is augmented to support them.  */
-	  current_ptrace_options |= PTRACE_O_TRACECLONE;
-#else
-	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
-	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
-
-	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
-	     support read-only process state.  */
-#endif
+	  current_ptrace_options |= PTRACE_O_TRACECLONE |
+	    (additional_flags & ~(PTRACE_O_TRACESYSGOOD
+				  | PTRACE_O_TRACEVFORKDONE));
 
 	  /* Do some cleanup and kill the grandchild.  */
 	  my_waitpid (second_pid, &second_status, 0);
@@ -551,3 +545,14 @@ linux_ptrace_init_warnings (void)
 
   linux_ptrace_test_ret_to_nx ();
 }
+
+/* Set additional ptrace flags to use.  Some such flags may be checked
+   by the implementation above.  This function must be called before
+   any other function in this file; otherwise the flags may not take
+   effect appropriately.  */
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+  additional_flags = flags;
+}
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index cffb5ce..41b3198 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -91,5 +91,6 @@ extern int linux_supports_tracefork (void);
 extern int linux_supports_traceclone (void);
 extern int linux_supports_tracevforkdone (void);
 extern int linux_supports_tracesysgood (void);
+extern void linux_ptrace_set_additional_flags (int);
 
 #endif /* COMMON_LINUX_PTRACE_H */
-- 
1.7.1

Re: [PATCH 2/3 v4] Remove some GDBSERVER checks from linux-ptrace

[Pedro Alves]

On 07/24/2014 01:51 PM, Gary Benson wrote:
>  	  /* We got the PID from the grandchild, which means fork
>  	     tracing is supported.  */
> -#ifdef GDBSERVER
> -	  /* Do not enable all the options for now since gdbserver does not
> -	     properly support them.  This restriction will be lifted when
> -	     gdbserver is augmented to support them.  */
> -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> -#else
> -	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> -	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
> -
> -	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
> -	     support read-only process state.  */
> -#endif
> +	  current_ptrace_options |= PTRACE_O_TRACECLONE |
> +	    (additional_flags & ~(PTRACE_O_TRACESYSGOOD
> +				  | PTRACE_O_TRACEVFORKDONE));

This isn't right.  That enables e.g., PTRACE_O_TRACEEXIT, for example,
and possibly invalid flags even.

Please reverse that and spell out the flags we want here explicitly, like:

	  current_ptrace_options |= PTRACE_O_TRACECLONE;
	  current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
                                 			 | PTRACE_O_TRACEVFORK
		                                         | PTRACE_O_TRACEEXEC));

Thanks,
Pedro Alves

Re: [PATCH 2/3 v4] Remove some GDBSERVER checks from linux-ptrace

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +	  current_ptrace_options |= PTRACE_O_TRACECLONE |
>> +	    (additional_flags & ~(PTRACE_O_TRACESYSGOOD
>> +				  | PTRACE_O_TRACEVFORKDONE));

Pedro> This isn't right.  That enables e.g., PTRACE_O_TRACEEXIT, for example,
Pedro> and possibly invalid flags even.

I don't see how that can happen.  I think the "&" prevents it.

Pedro> Please reverse that and spell out the flags we want here
Pedro> explicitly, like:

Pedro> 	  current_ptrace_options |= PTRACE_O_TRACECLONE;
Pedro> 	  current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
Pedro>                                  			 | PTRACE_O_TRACEVFORK
Pedro> 		                                         | PTRACE_O_TRACEEXEC));

Though FWIW this does seem clearer.

Tom

Re: [PATCH 2/3 v4] Remove some GDBSERVER checks from linux-ptrace

[Pedro Alves]

On 07/24/2014 03:07 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> +	  current_ptrace_options |= PTRACE_O_TRACECLONE |
>>> +	    (additional_flags & ~(PTRACE_O_TRACESYSGOOD
>>> +				  | PTRACE_O_TRACEVFORKDONE));
> 
> Pedro> This isn't right.  That enables e.g., PTRACE_O_TRACEEXIT, for example,
> Pedro> and possibly invalid flags even.
> 
> I don't see how that can happen.  I think the "&" prevents it.

If the caller puts PTRACE_O_TRACEEXIT in additional_flags, that'll
enable it, even though that's not what the test that led to that
code was making sure the kernel supported.

(PTRACE_O_TRACEEXIT was added to the kernel at the same time as
PTRACE_EVENT_VFORK_DONE, not the fork/exec flags).

-- 
Thanks,
Pedro Alves

[PATCH 2/3 v5] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

Pedro Alves wrote:
> On 07/24/2014 01:51 PM, Gary Benson wrote:
> >  	  /* We got the PID from the grandchild, which means fork
> >  	     tracing is supported.  */
> > -#ifdef GDBSERVER
> > -	  /* Do not enable all the options for now since gdbserver does not
> > -	     properly support them.  This restriction will be lifted when
> > -	     gdbserver is augmented to support them.  */
> > -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> > -#else
> > -	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> > -	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
> > -
> > -	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
> > -	     support read-only process state.  */
> > -#endif
> > +	  current_ptrace_options |= PTRACE_O_TRACECLONE |
> > +	    (additional_flags & ~(PTRACE_O_TRACESYSGOOD
> > +				  | PTRACE_O_TRACEVFORKDONE));
> 
> This isn't right.  That enables e.g., PTRACE_O_TRACEEXIT, for
> example, and possibly invalid flags even.
> 
> Please reverse that and spell out the flags we want here explicitly,
> like:
> 
> 	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> 	  current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
>                                  			 | PTRACE_O_TRACEVFORK
> 		                                         | PTRACE_O_TRACEEXEC));

Like this?

gdb/
2014-07-24  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* nat/linux-ptrace.c (additional_flags): New global.
	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
	additional_flags; don't check GDBSERVER.
	(linux_ptrace_set_additional_flags): New function.
	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
	Declare.
	* linux-nat.c (_initialize_linux_nat): Call
	linux_ptrace_set_additional_flags.
---
 gdb/ChangeLog          |   12 ++++++++++
 gdb/linux-nat.c        |    8 ++++++
 gdb/nat/linux-ptrace.c |   56 ++++++++++++++++++++++++++---------------------
 gdb/nat/linux-ptrace.h |    1 +
 4 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b50a88e..7db1b3d 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -5033,6 +5033,14 @@ Enables printf debugging output."),
   sigdelset (&suspend_mask, SIGCHLD);
 
   sigemptyset (&blocked_mask);
+
+  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
+     support read-only process state.  */
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+				     | PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
 }
 \f
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 3ad2113..24b02b9 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -37,6 +37,10 @@
    there are no supported features.  */
 static int current_ptrace_options = -1;
 
+/* Additional flags to test.  */
+
+static int additional_flags;
+
 /* Find all possible reasons we could fail to attach PID and append
    these as strings to the already initialized BUFFER.  '\0'
    termination of BUFFER must be done by the caller.  */
@@ -359,16 +363,16 @@ linux_check_ptrace_features (void)
 static void
 linux_test_for_tracesysgood (int child_pid)
 {
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACESYSGOOD.  */
-#else
   int ret;
 
+  if ((additional_flags & PTRACE_O_TRACESYSGOOD) == 0)
+    return;
+
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
 		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+
   if (ret == 0)
     current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
-#endif
 }
 
 /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
@@ -388,16 +392,15 @@ linux_test_for_tracefork (int child_pid)
   if (ret != 0)
     return;
 
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
-#else
-  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
-				    | PTRACE_O_TRACEVFORKDONE));
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
-#endif
+  if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
+    {
+      /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+					| PTRACE_O_TRACEVFORKDONE));
+      if (ret == 0)
+	current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
+    }
 
   /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
      don't know for sure that the feature is available; old
@@ -433,18 +436,10 @@ linux_test_for_tracefork (int child_pid)
 
 	  /* We got the PID from the grandchild, which means fork
 	     tracing is supported.  */
-#ifdef GDBSERVER
-	  /* Do not enable all the options for now since gdbserver does not
-	     properly support them.  This restriction will be lifted when
-	     gdbserver is augmented to support them.  */
 	  current_ptrace_options |= PTRACE_O_TRACECLONE;
-#else
-	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
-	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
-
-	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
-	     support read-only process state.  */
-#endif
+	  current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
+                                                         | PTRACE_O_TRACEVFORK
+                                                         | PTRACE_O_TRACEEXEC));
 
 	  /* Do some cleanup and kill the grandchild.  */
 	  my_waitpid (second_pid, &second_status, 0);
@@ -551,3 +546,14 @@ linux_ptrace_init_warnings (void)
 
   linux_ptrace_test_ret_to_nx ();
 }
+
+/* Set additional ptrace flags to use.  Some such flags may be checked
+   by the implementation above.  This function must be called before
+   any other function in this file; otherwise the flags may not take
+   effect appropriately.  */
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+  additional_flags = flags;
+}
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index cffb5ce..41b3198 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -91,5 +91,6 @@ extern int linux_supports_tracefork (void);
 extern int linux_supports_traceclone (void);
 extern int linux_supports_tracevforkdone (void);
 extern int linux_supports_tracesysgood (void);
+extern void linux_ptrace_set_additional_flags (int);
 
 #endif /* COMMON_LINUX_PTRACE_H */
-- 
1.7.1

Re: [PATCH 2/3 v5] Remove some GDBSERVER checks from linux-ptrace

[Pedro Alves]

On 07/24/2014 03:09 PM, Gary Benson wrote:

> Like this?

Yep, exactly.  Thanks.

Thanks,
Pedro Alves

Re: [PATCH 2/3 v5] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

Pedro Alves wrote:
> On 07/24/2014 03:09 PM, Gary Benson wrote:
> > Like this?
> 
> Yep, exactly.  Thanks.

Great, pushed.

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

This introduces common/errors.h.  This holds some error- and
warning-related declarations that can be used by the code in common.
Clients of the "common" code must provide definitions for these
functions.

gdb/
2014-07-24  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/errors.h: New file.
	* common/errors.c: Likewise.
	* Makefile.in (HFILES_NO_SRCDIR) [common/errors.h]:
	New header.
	(COMMON_OBS) [errors.o]: New object file.
	(errors.o): New rule.
	* utils.h: Include errors.h.
	(perror_with_name, error, verror, warning, vwarning):
	Don't declare.
	* common/common-utils.h: Include errors.h.
	(malloc_failure, internal_error): Don't declare.

gdb/gdbserver/
2014-07-24  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* Makefile.in (SFILES) [common/errors.c]: New sourcefile.
	(OBS) [errors.o]: New object file.
	(IPA_OBS) [errors-ipa.o]: Likewise.
	(errors.o): New rule.
	(errors-ipa.o): Likewise.
	* utils.h: Include errors.h.
	(perror_with_name, error, warning): Don't declare.
	* utils.c (warning): Renamed and rewritten as...
	(vwarning): New function.
	(error): Renamed and rewritten as...
	(verror): New function.
	(internal_error): Renamed and rewritten as...
	(internal_verror): New function.
---
 gdb/ChangeLog             |   15 +++++++++
 gdb/Makefile.in           |    8 ++++-
 gdb/common/common-utils.h |    5 +--
 gdb/common/errors.c       |   59 ++++++++++++++++++++++++++++++++++++
 gdb/common/errors.h       |   72 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog   |   17 ++++++++++
 gdb/gdbserver/Makefile.in |   16 ++++++++--
 gdb/gdbserver/utils.c     |   23 ++++----------
 gdb/gdbserver/utils.h     |    4 +--
 gdb/utils.c               |   36 ----------------------
 gdb/utils.h               |   16 +---------
 11 files changed, 192 insertions(+), 79 deletions(-)
 create mode 100644 gdb/common/errors.c
 create mode 100644 gdb/common/errors.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ce15501..074ea78 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -935,7 +935,7 @@ gdb_bfd.h sparc-ravenscar-thread.h ppc-ravenscar-thread.h nat/linux-btrace.h \
 ctf.h nat/i386-cpuid.h nat/i386-gcc-cpuid.h target/resume.h \
 target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
-i386-linux-nat.h
+i386-linux-nat.h common/errors.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1034,7 +1034,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	gdb_vecs.o jit.o progspace.o skip.o probe.o \
 	common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
 	format.o registry.o btrace.o record-btrace.o waitstatus.o \
-	print-utils.o rsp-low.o
+	print-utils.o rsp-low.o errors.o
 
 TSOBS = inflow.o
 
@@ -2144,6 +2144,10 @@ rsp-low.o: ${srcdir}/common/rsp-low.c
 	$(COMPILE) $(srcdir)/common/rsp-low.c
 	$(POSTCOMPILE)
 
+errors.o: ${srcdir}/common/errors.c
+	$(COMPILE) $(srcdir)/common/errors.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 063698d..53be2f8 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -24,6 +24,7 @@
 #include "ansidecl.h"
 #include <stddef.h>
 #include <stdarg.h>
+#include "errors.h"
 
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
@@ -43,10 +44,6 @@
 #endif
 #endif
 
-extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
-extern void internal_error (const char *file, int line, const char *, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
-
 /* xmalloc(), xrealloc() and xcalloc() have already been declared in
    "libiberty.h". */
 
diff --git a/gdb/common/errors.c b/gdb/common/errors.c
new file mode 100644
index 0000000..684bdcd
--- /dev/null
+++ b/gdb/common/errors.c
@@ -0,0 +1,59 @@
+/* Error reporting facilities.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "ansidecl.h"
+#include <stdarg.h>
+#include "errors.h"
+
+/* See common/errors.h.  */
+
+void
+warning (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  vwarning (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+error (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  verror (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+internal_error (const char *file, int line, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  internal_verror (file, line, fmt, ap);
+  va_end (ap);
+}
diff --git a/gdb/common/errors.h b/gdb/common/errors.h
new file mode 100644
index 0000000..91b58c6
--- /dev/null
+++ b/gdb/common/errors.h
@@ -0,0 +1,72 @@
+/* Declarations for error-reporting facilities.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_ERRORS_H
+#define COMMON_ERRORS_H
+
+/* A problem was detected, but the requested operation can still
+   proceed.  A warning message is constructed using a printf- or
+   vprintf-style argument list.  */
+
+extern void warning (const char *fmt, ...)
+     ATTRIBUTE_PRINTF (1, 2);
+
+extern void vwarning (const char *fmt, va_list args)
+     ATTRIBUTE_PRINTF (1, 0);
+
+/* A non-predictable, non-fatal error was detected.  The requested
+   operation cannot proceed.  An error message is constructed using
+   a printf- or vprintf-style argument list.  These functions do not
+   return.  */
+
+extern void error (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+extern void verror (const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
+
+/* An internal error was detected.  Internal errors indicate
+   programming errors such as assertion failures, as opposed to
+   more general errors beyond the application's control.  These
+   functions do not return.  An error message is constructed using
+   a printf- or vprintf-style argument list.  FILE and LINE
+   indicate the file and line number where the programming error
+   was detected.  */
+
+extern void internal_error (const char *file, int line,
+			    const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
+
+extern void internal_verror (const char *file, int line,
+			     const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
+\f
+
+/* A special case of "error" which constructs the error message by
+   combining STRING with the system error message for errno.  This
+   function does not return.  */
+
+extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
+
+/* A special case of internal error used to handle memory allocation
+   failures.  This function does not return.   */
+
+extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
+
+#endif /* COMMON_ERRORS_H */
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index f9a2f17..1faa00c 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -169,7 +169,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/buffer.c $(srcdir)/nat/linux-btrace.c \
 	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
-	$(srcdir)/common/rsp-low.c
+	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -182,7 +182,8 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       target.o waitstatus.o utils.o debug.o version.o vec.o gdb_vecs.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
-      tdesc.o print-utils.o rsp-low.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
+      tdesc.o print-utils.o rsp-low.o errors.o $(XML_BUILTIN) $(DEPFILES) \
+      $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
 XM_CLIBS = @LIBS@
@@ -300,7 +301,10 @@ gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
 	${CC-LD} $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) -o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) \
 	  $(XM_CLIBS) $(LIBGNU) $(LIBIBERTY)
 
-IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o ${IPA_DEPFILES}
+IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o \
+	regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o \
+	tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o errors-ipa.o \
+	${IPA_DEPFILES}
 
 IPA_LIB=libinproctrace.so
 
@@ -489,6 +493,9 @@ print-utils-ipa.o: ../common/print-utils.c
 rsp-low-ipa.o: ../common/rsp-low.c
 	$(IPAGENT_COMPILE) $<
 	$(POSTCOMPILE)
+errors-ipa.o: ../common/errors.c
+	$(IPAGENT_COMPILE) $<
+	$(POSTCOMPILE)
 
 ax.o: ax.c
 	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
@@ -530,6 +537,9 @@ filestuff.o: ../common/filestuff.c
 agent.o: ../common/agent.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+errors.o: ../common/errors.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 waitstatus.o: ../target/waitstatus.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
index 2d0b331..9add0b9 100644
--- a/gdb/gdbserver/utils.c
+++ b/gdb/gdbserver/utils.c
@@ -77,18 +77,15 @@ perror_with_name (const char *string)
   error ("%s.", combined);
 }
 
-/* Print an error message and return to command level.
-   STRING is the error message, used as a fprintf string,
-   and ARG is passed as an argument to it.  */
+/* Print an error message and return to top level.  */
 
 void
-error (const char *string,...)
+verror (const char *string, va_list args)
 {
 #ifndef IN_PROCESS_AGENT
   extern jmp_buf toplevel;
 #endif
-  va_list args;
-  va_start (args, string);
+
   fflush (stdout);
   vfprintf (stderr, string, args);
   fprintf (stderr, "\n");
@@ -116,31 +113,25 @@ fatal (const char *string,...)
   exit (1);
 }
 
-/* VARARGS */
+/* Print a warning message.  */
+
 void
-warning (const char *string,...)
+vwarning (const char *string, va_list args)
 {
-  va_list args;
-  va_start (args, string);
   fprintf (stderr, PREFIX);
   vfprintf (stderr, string, args);
   fprintf (stderr, "\n");
-  va_end (args);
 }
 
 /* Report a problem internal to GDBserver, and exit.  */
 
 void
-internal_error (const char *file, int line, const char *fmt, ...)
+internal_verror (const char *file, int line, const char *fmt, va_list args)
 {
-  va_list args;
-  va_start (args, fmt);
-
   fprintf (stderr,  "\
 %s:%d: A problem internal to " TOOLNAME " has been detected.\n", file, line);
   vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
-  va_end (args);
   exit (1);
 }
 
diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
index 906924b..b608540 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -20,11 +20,9 @@
 #define UTILS_H
 
 #include "print-utils.h"
+#include "errors.h"
 
-void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
-void error (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-void warning (const char *string,...) ATTRIBUTE_PRINTF (1, 2);
 char *paddress (CORE_ADDR addr);
 char *pfildes (gdb_fildes_t fd);
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 8af8827..1f1ba0e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -533,22 +533,6 @@ vwarning (const char *string, va_list args)
     }
 }
 
-/* Print a warning message.
-   The first argument STRING is the warning message, used as a fprintf string,
-   and the remaining args are passed as arguments to it.
-   The primary difference between warnings and errors is that a warning
-   does not force the return to command level.  */
-
-void
-warning (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  vwarning (string, args);
-  va_end (args);
-}
-
 /* Print an error message and return to command level.
    The first argument STRING is the error message, used as a fprintf string,
    and the remaining args are passed as arguments to it.  */
@@ -560,16 +544,6 @@ verror (const char *string, va_list args)
 }
 
 void
-error (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  throw_verror (GENERIC_ERROR, string, args);
-  va_end (args);
-}
-
-void
 error_stream (struct ui_file *stream)
 {
   char *message = ui_file_xstrdup (stream, NULL);
@@ -816,16 +790,6 @@ internal_verror (const char *file, int line, const char *fmt, va_list ap)
   throw_quit (_("Command aborted."));
 }
 
-void
-internal_error (const char *file, int line, const char *string, ...)
-{
-  va_list ap;
-
-  va_start (ap, string);
-  internal_verror (file, line, string, ap);
-  va_end (ap);
-}
-
 static struct internal_problem internal_warning_problem = {
   "internal-warning", 1, internal_problem_ask, 1, internal_problem_ask
 };
diff --git a/gdb/utils.h b/gdb/utils.h
index cc79562..e48fc43 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -24,6 +24,7 @@
 #include "cleanups.h"
 #include "exceptions.h"
 #include "print-utils.h"
+#include "errors.h"
 
 extern void initialize_utils (void);
 
@@ -269,7 +270,6 @@ extern void fprintf_symbol_filtered (struct ui_file *, const char *,
 
 extern void throw_perror_with_name (enum errors errcode, const char *string)
   ATTRIBUTE_NORETURN;
-extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
 
 extern void perror_warning_with_name (const char *string);
 
@@ -283,18 +283,8 @@ extern void (*deprecated_error_begin_hook) (void);
 
 extern char *warning_pre_print;
 
-extern void verror (const char *fmt, va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
-
-extern void error (const char *fmt, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-
 extern void error_stream (struct ui_file *) ATTRIBUTE_NORETURN;
 
-extern void internal_verror (const char *file, int line, const char *,
-			     va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
-
 extern void internal_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);
@@ -302,10 +292,6 @@ extern void internal_vwarning (const char *file, int line,
 extern void internal_warning (const char *file, int line,
 			      const char *, ...) ATTRIBUTE_PRINTF (3, 4);
 
-extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
-
-extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
-
 extern void demangler_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);
-- 
1.7.1

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Pedro Alves]

On 07/24/2014 01:51 PM, Gary Benson wrote:

> gdb/
> 2014-07-24  Tom Tromey  <tromey@redhat.com>
> 	    Gary Benson  <gbenson@redhat.com>
> 
> 	* common/errors.h: New file.
> 	* common/errors.c: Likewise.
> 	* Makefile.in (HFILES_NO_SRCDIR) [common/errors.h]:
> 	New header.
> 	(COMMON_OBS) [errors.o]: New object file.

Write:

 	(COMMON_OBS): Add errors.o.

[] has a specific meaning that is not what you're doing here.  See:

  https://www.gnu.org/prep/standards/html_node/Conditional-Changes.html#Conditional-Changes

> 	(errors.o): New rule.
> 	* utils.h: Include errors.h.
> 	(perror_with_name, error, verror, warning, vwarning):
> 	Don't declare.
> 	* common/common-utils.h: Include errors.h.
> 	(malloc_failure, internal_error): Don't declare.
> 
> gdb/gdbserver/
> 2014-07-24  Tom Tromey  <tromey@redhat.com>
> 	    Gary Benson  <gbenson@redhat.com>
> 
> 	* Makefile.in (SFILES) [common/errors.c]: New sourcefile.
> 	(OBS) [errors.o]: New object file.
> 	(IPA_OBS) [errors-ipa.o]: Likewise.

Likewise.

> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "config.h"

Sorry for the push back, but it's wrong to only include "config.h".
We need to include gnulib's config.h everywhere too.  I think
the best is to add a central common header that handles that
detail first.

-- 
Thanks,
Pedro Alves

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Doug Evans]

Pedro Alves writes:
 > > +   You should have received a copy of the GNU General Public License
 > > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > > +
 > > +#include "config.h"
 > 
 > Sorry for the push back, but it's wrong to only include "config.h".
 > We need to include gnulib's config.h everywhere too.  I think
 > the best is to add a central common header that handles that
 > detail first.

OTOH,
just including "config.h" is a real common thing to do across all packages.
I can imagine this coming up again and again.

IWBN if hacking on gdb didn't require special cases to standard
programming paradigms wherever possible.
Can we arrange for config.h to include gnulib's config.h?
Or, another thought would be to have configure generate gdb-config.h
(or some such) and then have src/gdb/config.h include both (and similarly
for gdbserver - haven't looked at the details though).

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

Doug Evans wrote:
> Pedro Alves writes:
> > > +   You should have received a copy of the GNU General Public License
> > > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > > +
> > > +#include "config.h"
> > 
> > Sorry for the push back, but it's wrong to only include "config.h".
> > We need to include gnulib's config.h everywhere too.  I think
> > the best is to add a central common header that handles that
> > detail first.
> 
> OTOH,
> just including "config.h" is a real common thing to do across all
> packages.  I can imagine this coming up again and again.
> 
> IWBN if hacking on gdb didn't require special cases to standard
> programming paradigms wherever possible.  Can we arrange for
> config.h to include gnulib's config.h?  Or, another thought would be
> to have configure generate gdb-config.h (or some such) and then have
> src/gdb/config.h include both (and similarly for gdbserver - haven't
> looked at the details though).

I'm working on this now.  My plan is to have gdb/common/common-defs.h
(which includes config.h and the correct gnulib config.h) and have
defs.h and server.h include common-defs.h as the first line.  By the
end of that series most every file will include defs.h or server.h
and no file will include config.h.

At the first instance common-defs.h will include most files included
by both defs.h and server.h.  There are various workarounds for
various things in both, and it would be good to have these workarounds
the same for all of GDB/gdbserver/etc.  I plan to omit alloca.h and
errno.h initially, as these have pretty heavy hacking around them and
they're not needed yet.  I'll probably do both at some point, but as
individual patches/serieses.

I'm also going to do some #include cleanups as part of the same
series.  For example lots of things include common-utils.h, but
that's currently included by defs.h and server.h (so I'll move it
to common-defs.h) and if every file includes common-defs.h somehow
then nothing needs common-utils.h.

Once the common-defs.h series is finished and pushed I'll rebase
this common-cleanups series on top of it.  It'll be much cleaner
and by then the number of "#ifdef GDBSERVER"s in the codebase
will be precisely one (to select the appropriate gnulib config.h
in common-defs.h).

Does all this sound ok?

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Pedro Alves]

On 07/25/2014 09:36 AM, Gary Benson wrote:

> I'm working on this now.  My plan is to have gdb/common/common-defs.h
> (which includes config.h and the correct gnulib config.h) and have
> defs.h and server.h include common-defs.h as the first line.  By the
> end of that series most every file will include defs.h or server.h
> and no file will include config.h.

I think you meant, that files in gdb will include defs.h, files in
gdbserver will include server.h, and files in the shared directories
will include common-defs.h as the first line.

> At the first instance common-defs.h will include most files included
> by both defs.h and server.h.  There are various workarounds for
> various things in both, and it would be good to have these workarounds
> the same for all of GDB/gdbserver/etc.  I plan to omit alloca.h and
> errno.h initially, as these have pretty heavy hacking around them and
> they're not needed yet.  I'll probably do both at some point, but as
> individual patches/serieses.
> 
> I'm also going to do some #include cleanups as part of the same
> series.  For example lots of things include common-utils.h, but
> that's currently included by defs.h and server.h (so I'll move it
> to common-defs.h) and if every file includes common-defs.h somehow
> then nothing needs common-utils.h.
> 
> Once the common-defs.h series is finished and pushed I'll rebase
> this common-cleanups series on top of it.  It'll be much cleaner
> and by then the number of "#ifdef GDBSERVER"s in the codebase
> will be precisely one (to select the appropriate gnulib config.h
> in common-defs.h).
> 
> Does all this sound ok?

It does to me.

-- 
Thanks,
Pedro Alves

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

Pedro Alves wrote:
> On 07/25/2014 09:36 AM, Gary Benson wrote:
> > I'm working on this now.  My plan is to have gdb/common/common-defs.h
> > (which includes config.h and the correct gnulib config.h) and have
> > defs.h and server.h include common-defs.h as the first line.  By the
> > end of that series most every file will include defs.h or server.h
> > and no file will include config.h.
> 
> I think you meant, that files in gdb will include defs.h, files in
> gdbserver will include server.h, and files in the shared directories
> will include common-defs.h as the first line.

Yes, eventually.  I plan to submit an initial series, in which
common-defs.h only includes the two config.h files, and any files
not including either defs.h or server.h as their first line will
be fixed up.  If that proves acceptable, I'll submit one or more
further series to move the various includes currently common to
defs.h and server.h into common-defs.h, and finally something to
switch common/target/nat files over to common-defs.h.  That last
bit may need to end up mixed in with parts of the common-cleanups
series.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Pedro Alves]

On 07/25/2014 12:16 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 07/25/2014 09:36 AM, Gary Benson wrote:
>>> I'm working on this now.  My plan is to have gdb/common/common-defs.h
>>> (which includes config.h and the correct gnulib config.h) and have
>>> defs.h and server.h include common-defs.h as the first line.  By the
>>> end of that series most every file will include defs.h or server.h
>>> and no file will include config.h.
>>
>> I think you meant, that files in gdb will include defs.h, files in
>> gdbserver will include server.h, and files in the shared directories
>> will include common-defs.h as the first line.
> 
> Yes, eventually.  I plan to submit an initial series, in which
> common-defs.h only includes the two config.h files, and any files
> not including either defs.h or server.h as their first line will
> be fixed up.  If that proves acceptable, I'll submit one or more
> further series to move the various includes currently common to
> defs.h and server.h into common-defs.h, and finally something to
> switch common/target/nat files over to common-defs.h.  

Sounds good to me.  I've distilled this into the wiki:

  https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29

Please do feel free to edit it / improve it.

(obviously that can evolve if the plan is objected to and changes.)

-- 
Thanks,
Pedro Alves

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

Pedro Alves wrote:
> On 07/25/2014 12:16 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > On 07/25/2014 09:36 AM, Gary Benson wrote:
> > > > I'm working on this now.  My plan is to have
> > > > gdb/common/common-defs.h (which includes config.h and the
> > > > correct gnulib config.h) and have defs.h and server.h include
> > > > common-defs.h as the first line.  By the end of that series
> > > > most every file will include defs.h or server.h and no file
> > > > will include config.h.
> > >
> > > I think you meant, that files in gdb will include defs.h, files
> > > in gdbserver will include server.h, and files in the shared
> > > directories will include common-defs.h as the first line.
> > 
> > Yes, eventually.  I plan to submit an initial series, in which
> > common-defs.h only includes the two config.h files, and any files
> > not including either defs.h or server.h as their first line will
> > be fixed up.  If that proves acceptable, I'll submit one or more
> > further series to move the various includes currently common to
> > defs.h and server.h into common-defs.h, and finally something to
> > switch common/target/nat files over to common-defs.h.
> 
> Sounds good to me.  I've distilled this into the wiki:
> 
>   https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
> 
> Please do feel free to edit it / improve it.
> 
> (obviously that can evolve if the plan is objected to and changes.)

Perfect, thanks for documenting this.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Doug Evans]

On Fri, Jul 25, 2014 at 4:46 AM, Pedro Alves <palves@redhat.com> wrote:
>> Yes, eventually.  I plan to submit an initial series, in which
>> common-defs.h only includes the two config.h files, and any files
>> not including either defs.h or server.h as their first line will
>> be fixed up.  If that proves acceptable, I'll submit one or more
>> further series to move the various includes currently common to
>> defs.h and server.h into common-defs.h, and finally something to
>> switch common/target/nat files over to common-defs.h.
>
> Sounds good to me.  I've distilled this into the wiki:
>
>   https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>
> Please do feel free to edit it / improve it.
>
> (obviously that can evolve if the plan is objected to and changes.)

fyi,
There is also:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

which will need updating, e.g., to reference what to do for common files.

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Pedro Alves]

On 07/29/2014 05:32 PM, Doug Evans wrote:

> fyi,
> There is also:
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> 
> which will need updating, e.g., to reference what to do for common files.

Indeed!  Gary, could you take care of that?  (the 'Include Files'
section down below.)

Thanks,
Pedro Alves

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

Pedro Alves wrote:
> On 07/29/2014 05:32 PM, Doug Evans wrote:
> > There is also:
> > https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> > 
> > which will need updating, e.g., to reference what to do for common
> > files.
> 
> Indeed!  Gary, could you take care of that?  (the 'Include Files'
> section down below.)

Ah, I meant to ask if this was documented somewhere.  I've updated it
for the change I just pushed.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Pedro Alves]

On 07/24/2014 06:52 PM, Doug Evans wrote:
> Pedro Alves writes:
>  > > +   You should have received a copy of the GNU General Public License
>  > > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  > > +
>  > > +#include "config.h"
>  > 
>  > Sorry for the push back, but it's wrong to only include "config.h".
>  > We need to include gnulib's config.h everywhere too.  I think
>  > the best is to add a central common header that handles that
>  > detail first.
> 
> OTOH,
> just including "config.h" is a real common thing to do across all packages.
> I can imagine this coming up again and again.
> 
> IWBN if hacking on gdb didn't require special cases to standard
> programming paradigms wherever possible.
> Can we arrange for config.h to include gnulib's config.h?
> Or, another thought would be to have configure generate gdb-config.h
> (or some such) and then have src/gdb/config.h include both (and similarly
> for gdbserver - haven't looked at the details though).

I fail to see how changing any of that would substantially
improve things.  GDB has had defs.h for a long while, and people didn't
seem to be confused by that -- it's quite common to have a
project-specific global header to include.  If we have something
like that in common, then even if we have only a single config.h,
we'd naturally include that from the common common/ header anyway
instead of in addition to including the common/ file everywhere,
also including config.h directly everywhere too.

Thanks,
Pedro Alves

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Doug Evans]

Gary Benson writes:
 > This introduces common/errors.h.  This holds some error- and
 > warning-related declarations that can be used by the code in common.
 > Clients of the "common" code must provide definitions for these
 > functions.
 > 
 > gdb/
 > 2014-07-24  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/errors.h: New file.
 > 	* common/errors.c: Likewise.
 > 	* Makefile.in (HFILES_NO_SRCDIR) [common/errors.h]:
 > 	New header.
 > 	(COMMON_OBS) [errors.o]: New object file.
 > 	(errors.o): New rule.
 > 	* utils.h: Include errors.h.
 > 	(perror_with_name, error, verror, warning, vwarning):
 > 	Don't declare.
 > 	* common/common-utils.h: Include errors.h.
 > 	(malloc_failure, internal_error): Don't declare.
 > 
 > gdb/gdbserver/
 > 2014-07-24  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* Makefile.in (SFILES) [common/errors.c]: New sourcefile.
 > 	(OBS) [errors.o]: New object file.
 > 	(IPA_OBS) [errors-ipa.o]: Likewise.
 > 	(errors.o): New rule.
 > 	(errors-ipa.o): Likewise.
 > 	* utils.h: Include errors.h.
 > 	(perror_with_name, error, warning): Don't declare.
 > 	* utils.c (warning): Renamed and rewritten as...
 > 	(vwarning): New function.
 > 	(error): Renamed and rewritten as...
 > 	(verror): New function.
 > 	(internal_error): Renamed and rewritten as...
 > 	(internal_verror): New function.
 > [...]

Hi.  I like the idea of having the va_list functions provided
by gdb/gdbserver so that the stdarg versions can be defined in
one place, but it's odd to then not have just one copy of
perror_with_name and malloc_failure given that their function comment
says:

perror_with_name:
/* A special case of "error" which constructs the error message by ...

and

malloc_failure:
/* A special case of internal error used to handle memory allocation ...

When I read "special case of ..." the first thing that comes to mind
is that the function is a utility wrapper around the corresponding
function, and given that it's not I'm left wondering "Why not?".

In the case of perror_with_name,
I see gdb's perror_with_name has some extra duties
(e.g., throw_perror_with_name clears errno/bfd-errno) but
throw_perror_with_name is only ever called by perror_with_name (AFAICT).
OTOH, gdb uses safe_strerror whereas gdbserver just uses strerror.
OTOOH, gdbserver could(/should?) have a safe_strerror too.
Taking on adding safe_strerror to gdbserver would require taking on
handling mingw (see gdb/mingw-hdep.c).  I can understand if you'd like
to leave that for another day.

In the case of malloc_failure,
it's not really an *internal* error,
even if in the case of gdb it calls internal_error, though arguably
it should do something different - it's more of a "fatal". :-) Though
I'm not suggesting trying to go down that path.  :-) It might be possible
come up with a name other than "fatal" that could apply to both gdb and
gdbserver so that malloc_failure could call it, but no need to try to
do that now.
So, *if* you want to keep *this* part of the patch basically as-is
(which is fine by me) I would rephrase this comment to something like:

/* Call this function to handle memory allocation failures.
   This function does not return.   */

I don't have too strong opinion on the wording.
If someone wants a different wording, go for it.

 > 
 > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
 > index ce15501..074ea78 100644
 > --- a/gdb/Makefile.in
 > +++ b/gdb/Makefile.in
 > @@ -935,7 +935,7 @@ gdb_bfd.h sparc-ravenscar-thread.h ppc-ravenscar-thread.h nat/linux-btrace.h \
 >  ctf.h nat/i386-cpuid.h nat/i386-gcc-cpuid.h target/resume.h \
 >  target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 >  common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 > -i386-linux-nat.h
 > +i386-linux-nat.h common/errors.h
 >  
 >  # Header files that already have srcdir in them, or which are in objdir.
 >  
 > @@ -1034,7 +1034,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 >  	gdb_vecs.o jit.o progspace.o skip.o probe.o \
 >  	common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
 >  	format.o registry.o btrace.o record-btrace.o waitstatus.o \
 > -	print-utils.o rsp-low.o
 > +	print-utils.o rsp-low.o errors.o
 >  
 >  TSOBS = inflow.o
 >  
 > @@ -2144,6 +2144,10 @@ rsp-low.o: ${srcdir}/common/rsp-low.c
 >  	$(COMPILE) $(srcdir)/common/rsp-low.c
 >  	$(POSTCOMPILE)
 >  
 > +errors.o: ${srcdir}/common/errors.c

Nit: $(srcdir)

 > +	$(COMPILE) $(srcdir)/common/errors.c
 > +	$(POSTCOMPILE)
 > +
 >  #
 >  # gdb/target/ dependencies
 >  #
 > diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
 > index 063698d..53be2f8 100644
 > --- a/gdb/common/common-utils.h
 > +++ b/gdb/common/common-utils.h
 > @@ -24,6 +24,7 @@
 >  #include "ansidecl.h"
 >  #include <stddef.h>
 >  #include <stdarg.h>
 > +#include "errors.h"
 >  
 >  /* If possible, define FUNCTION_NAME, a macro containing the name of
 >     the function being defined.  Since this macro may not always be
 > @@ -43,10 +44,6 @@
 >  #endif
 >  #endif
 >  
 > -extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
 > -extern void internal_error (const char *file, int line, const char *, ...)
 > -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
 > -
 >  /* xmalloc(), xrealloc() and xcalloc() have already been declared in
 >     "libiberty.h". */
 >  
 > diff --git a/gdb/common/errors.c b/gdb/common/errors.c
 > new file mode 100644
 > index 0000000..684bdcd
 > --- /dev/null
 > +++ b/gdb/common/errors.c
 > @@ -0,0 +1,59 @@
 > +/* Error reporting facilities.
 > +
 > +   Copyright (C) 1986-2014 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#include "config.h"
 > +#include "ansidecl.h"
 > +#include <stdarg.h>
 > +#include "errors.h"
 > +
 > +/* See common/errors.h.  */
 > +
 > +void
 > +warning (const char *fmt, ...)
 > +{
 > +  va_list ap;
 > +
 > +  va_start (ap, fmt);
 > +  vwarning (fmt, ap);
 > +  va_end (ap);
 > +}
 > +
 > +/* See common/errors.h.  */
 > +
 > +void
 > +error (const char *fmt, ...)
 > +{
 > +  va_list ap;
 > +
 > +  va_start (ap, fmt);
 > +  verror (fmt, ap);
 > +  va_end (ap);
 > +}
 > +
 > +/* See common/errors.h.  */
 > +
 > +void
 > +internal_error (const char *file, int line, const char *fmt, ...)
 > +{
 > +  va_list ap;
 > +
 > +  va_start (ap, fmt);
 > +  internal_verror (file, line, fmt, ap);
 > +  va_end (ap);
 > +}
 > diff --git a/gdb/common/errors.h b/gdb/common/errors.h
 > new file mode 100644
 > index 0000000..91b58c6
 > --- /dev/null
 > +++ b/gdb/common/errors.h
 > @@ -0,0 +1,72 @@
 > +/* Declarations for error-reporting facilities.
 > +
 > +   Copyright (C) 1986-2014 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#ifndef COMMON_ERRORS_H
 > +#define COMMON_ERRORS_H
 > +
 > +/* A problem was detected, but the requested operation can still
 > +   proceed.  A warning message is constructed using a printf- or
 > +   vprintf-style argument list.  */
 > +
 > +extern void warning (const char *fmt, ...)
 > +     ATTRIBUTE_PRINTF (1, 2);
 > +
 > +extern void vwarning (const char *fmt, va_list args)
 > +     ATTRIBUTE_PRINTF (1, 0);
 > +
 > +/* A non-predictable, non-fatal error was detected.  The requested
 > +   operation cannot proceed.  An error message is constructed using
 > +   a printf- or vprintf-style argument list.  These functions do not
 > +   return.  */
 > +
 > +extern void error (const char *fmt, ...)
 > +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 > +
 > +extern void verror (const char *fmt, va_list args)
 > +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
 > +
 > +/* An internal error was detected.  Internal errors indicate
 > +   programming errors such as assertion failures, as opposed to
 > +   more general errors beyond the application's control.  These
 > +   functions do not return.  An error message is constructed using
 > +   a printf- or vprintf-style argument list.  FILE and LINE
 > +   indicate the file and line number where the programming error
 > +   was detected.  */
 > +
 > +extern void internal_error (const char *file, int line,
 > +			    const char *fmt, ...)
 > +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
 > +
 > +extern void internal_verror (const char *file, int line,
 > +			     const char *fmt, va_list args)
 > +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
 > +\f
 > +
 > +/* A special case of "error" which constructs the error message by
 > +   combining STRING with the system error message for errno.  This
 > +   function does not return.  */
 > +
 > +extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
 > +
 > +/* A special case of internal error used to handle memory allocation
 > +   failures.  This function does not return.   */
 > +
 > +extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
 > +
 > +#endif /* COMMON_ERRORS_H */
 > diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
 > index f9a2f17..1faa00c 100644
 > --- a/gdb/gdbserver/Makefile.in
 > +++ b/gdb/gdbserver/Makefile.in
 > @@ -169,7 +169,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 >  	$(srcdir)/common/buffer.c $(srcdir)/nat/linux-btrace.c \
 >  	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
 >  	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
 > -	$(srcdir)/common/rsp-low.c
 > +	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c
 >  
 >  DEPFILES = @GDBSERVER_DEPFILES@
 >  
 > @@ -182,7 +182,8 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
 >        target.o waitstatus.o utils.o debug.o version.o vec.o gdb_vecs.o \
 >        mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
 >        common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
 > -      tdesc.o print-utils.o rsp-low.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 > +      tdesc.o print-utils.o rsp-low.o errors.o $(XML_BUILTIN) $(DEPFILES) \
 > +      $(LIBOBJS)
 >  GDBREPLAY_OBS = gdbreplay.o version.o
 >  GDBSERVER_LIBS = @GDBSERVER_LIBS@
 >  XM_CLIBS = @LIBS@
 > @@ -300,7 +301,10 @@ gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
 >  	${CC-LD} $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) -o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) \
 >  	  $(XM_CLIBS) $(LIBGNU) $(LIBIBERTY)
 >  
 > -IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o ${IPA_DEPFILES}
 > +IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o \
 > +	regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o \
 > +	tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o errors-ipa.o \
 > +	${IPA_DEPFILES}
 >  
 >  IPA_LIB=libinproctrace.so
 >  
 > @@ -489,6 +493,9 @@ print-utils-ipa.o: ../common/print-utils.c
 >  rsp-low-ipa.o: ../common/rsp-low.c
 >  	$(IPAGENT_COMPILE) $<
 >  	$(POSTCOMPILE)
 > +errors-ipa.o: ../common/errors.c
 > +	$(IPAGENT_COMPILE) $<
 > +	$(POSTCOMPILE)
 >  
 >  ax.o: ax.c
 >  	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
 > @@ -530,6 +537,9 @@ filestuff.o: ../common/filestuff.c
 >  agent.o: ../common/agent.c
 >  	$(COMPILE) $<
 >  	$(POSTCOMPILE)
 > +errors.o: ../common/errors.c
 > +	$(COMPILE) $<
 > +	$(POSTCOMPILE)
 >  waitstatus.o: ../target/waitstatus.c
 >  	$(COMPILE) $<
 >  	$(POSTCOMPILE)
 > diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
 > index 2d0b331..9add0b9 100644
 > --- a/gdb/gdbserver/utils.c
 > +++ b/gdb/gdbserver/utils.c
 > @@ -77,18 +77,15 @@ perror_with_name (const char *string)
 >    error ("%s.", combined);
 >  }
 >  
 > -/* Print an error message and return to command level.
 > -   STRING is the error message, used as a fprintf string,
 > -   and ARG is passed as an argument to it.  */
 > +/* Print an error message and return to top level.  */
 >  
 >  void
 > -error (const char *string,...)
 > +verror (const char *string, va_list args)
 >  {
 >  #ifndef IN_PROCESS_AGENT
 >    extern jmp_buf toplevel;
 >  #endif
 > -  va_list args;
 > -  va_start (args, string);
 > +
 >    fflush (stdout);
 >    vfprintf (stderr, string, args);
 >    fprintf (stderr, "\n");
 > @@ -116,31 +113,25 @@ fatal (const char *string,...)
 >    exit (1);
 >  }
 >  
 > -/* VARARGS */
 > +/* Print a warning message.  */
 > +
 >  void
 > -warning (const char *string,...)
 > +vwarning (const char *string, va_list args)
 >  {
 > -  va_list args;
 > -  va_start (args, string);
 >    fprintf (stderr, PREFIX);
 >    vfprintf (stderr, string, args);
 >    fprintf (stderr, "\n");
 > -  va_end (args);
 >  }
 >  
 >  /* Report a problem internal to GDBserver, and exit.  */
 >  
 >  void
 > -internal_error (const char *file, int line, const char *fmt, ...)
 > +internal_verror (const char *file, int line, const char *fmt, va_list args)
 >  {
 > -  va_list args;
 > -  va_start (args, fmt);
 > -
 >    fprintf (stderr,  "\
 >  %s:%d: A problem internal to " TOOLNAME " has been detected.\n", file, line);
 >    vfprintf (stderr, fmt, args);
 >    fprintf (stderr, "\n");
 > -  va_end (args);
 >    exit (1);
 >  }
 >  
 > diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
 > index 906924b..b608540 100644
 > --- a/gdb/gdbserver/utils.h
 > +++ b/gdb/gdbserver/utils.h
 > @@ -20,11 +20,9 @@
 >  #define UTILS_H
 >  
 >  #include "print-utils.h"
 > +#include "errors.h"
 >  
 > -void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
 > -void error (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 >  void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 > -void warning (const char *string,...) ATTRIBUTE_PRINTF (1, 2);
 >  char *paddress (CORE_ADDR addr);
 >  char *pfildes (gdb_fildes_t fd);
 >  
 > diff --git a/gdb/utils.c b/gdb/utils.c
 > index 8af8827..1f1ba0e 100644
 > --- a/gdb/utils.c
 > +++ b/gdb/utils.c
 > @@ -533,22 +533,6 @@ vwarning (const char *string, va_list args)
 >      }
 >  }
 >  
 > -/* Print a warning message.
 > -   The first argument STRING is the warning message, used as a fprintf string,
 > -   and the remaining args are passed as arguments to it.
 > -   The primary difference between warnings and errors is that a warning
 > -   does not force the return to command level.  */
 > -
 > -void
 > -warning (const char *string, ...)
 > -{
 > -  va_list args;
 > -
 > -  va_start (args, string);
 > -  vwarning (string, args);
 > -  va_end (args);
 > -}
 > -
 >  /* Print an error message and return to command level.
 >     The first argument STRING is the error message, used as a fprintf string,
 >     and the remaining args are passed as arguments to it.  */
 > @@ -560,16 +544,6 @@ verror (const char *string, va_list args)
 >  }
 >  
 >  void
 > -error (const char *string, ...)
 > -{
 > -  va_list args;
 > -
 > -  va_start (args, string);
 > -  throw_verror (GENERIC_ERROR, string, args);
 > -  va_end (args);
 > -}
 > -
 > -void
 >  error_stream (struct ui_file *stream)
 >  {
 >    char *message = ui_file_xstrdup (stream, NULL);
 > @@ -816,16 +790,6 @@ internal_verror (const char *file, int line, const char *fmt, va_list ap)
 >    throw_quit (_("Command aborted."));
 >  }
 >  
 > -void
 > -internal_error (const char *file, int line, const char *string, ...)
 > -{
 > -  va_list ap;
 > -
 > -  va_start (ap, string);
 > -  internal_verror (file, line, string, ap);
 > -  va_end (ap);
 > -}
 > -
 >  static struct internal_problem internal_warning_problem = {
 >    "internal-warning", 1, internal_problem_ask, 1, internal_problem_ask
 >  };
 > diff --git a/gdb/utils.h b/gdb/utils.h
 > index cc79562..e48fc43 100644
 > --- a/gdb/utils.h
 > +++ b/gdb/utils.h
 > @@ -24,6 +24,7 @@
 >  #include "cleanups.h"
 >  #include "exceptions.h"
 >  #include "print-utils.h"
 > +#include "errors.h"
 >  
 >  extern void initialize_utils (void);
 >  
 > @@ -269,7 +270,6 @@ extern void fprintf_symbol_filtered (struct ui_file *, const char *,
 >  
 >  extern void throw_perror_with_name (enum errors errcode, const char *string)
 >    ATTRIBUTE_NORETURN;
 > -extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
 >  
 >  extern void perror_warning_with_name (const char *string);
 >  
 > @@ -283,18 +283,8 @@ extern void (*deprecated_error_begin_hook) (void);
 >  
 >  extern char *warning_pre_print;
 >  
 > -extern void verror (const char *fmt, va_list ap)
 > -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
 > -
 > -extern void error (const char *fmt, ...)
 > -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 > -
 >  extern void error_stream (struct ui_file *) ATTRIBUTE_NORETURN;
 >  
 > -extern void internal_verror (const char *file, int line, const char *,
 > -			     va_list ap)
 > -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
 > -
 >  extern void internal_vwarning (const char *file, int line,
 >  			       const char *, va_list ap)
 >       ATTRIBUTE_PRINTF (3, 0);
 > @@ -302,10 +292,6 @@ extern void internal_vwarning (const char *file, int line,
 >  extern void internal_warning (const char *file, int line,
 >  			      const char *, ...) ATTRIBUTE_PRINTF (3, 4);
 >  
 > -extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
 > -
 > -extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
 > -
 >  extern void demangler_vwarning (const char *file, int line,
 >  			       const char *, va_list ap)
 >       ATTRIBUTE_PRINTF (3, 0);
 > -- 
 > 1.7.1
 > 

-- 
/dje

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Doug Evans]

On Thu, Jul 24, 2014 at 10:45 AM, Doug Evans <dje@google.com> wrote:
> Gary Benson writes:
>  > This introduces common/errors.h.  This holds some error- and
>  > warning-related declarations that can be used by the code in common.
>  > Clients of the "common" code must provide definitions for these
>  > functions.
> [...]
>  > @@ -2144,6 +2144,10 @@ rsp-low.o: ${srcdir}/common/rsp-low.c
>  >      $(COMPILE) $(srcdir)/common/rsp-low.c
>  >      $(POSTCOMPILE)
>  >
>  > +errors.o: ${srcdir}/common/errors.c
>
> Nit: $(srcdir)

Holy cow, I see gdb/Makefile.in is rather inconsistent here.
Ok, no point in cleaning this up here.
One can even argue to leave it as ${srcdir}.  Fine by me.

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Gary Benson]

Doug Evans wrote:
> ...it's odd to then not have just one copy of perror_with_name and
> malloc_failure given that their function comment says:
> 
> perror_with_name:
> /* A special case of "error" which constructs the error message by ...
> 
> and
> 
> malloc_failure:
> /* A special case of internal error used to handle memory allocation ...
> 
> When I read "special case of ..." the first thing that comes to mind
> is that the function is a utility wrapper around the corresponding
> function, and given that it's not I'm left wondering "Why not?".
[snip]
> So, *if* you want to keep *this* part of the patch basically as-is
> (which is fine by me) I would rephrase this comment to something
> like:
> 
> /* Call this function to handle memory allocation failures.
>    This function does not return.   */
> 
> I don't have too strong opinion on the wording.  If someone wants a
> different wording, go for it.

I pretty much agree with you 100% here.  I wasn't happy with the
comments for either of these, and I'll gladly change them.

> In the case of malloc_failure, it's not really an *internal* error,

No--and certainly not by the description of internal_error earlier in
the file.

> even if in the case of gdb it calls internal_error, though arguably
> it should do something different - it's more of a "fatal". :-)
> Though I'm not suggesting trying to go down that path.  :-)

I spoke with Pedro about this in Cambridge.  malloc failure is a funny
one: sometimes it's fatal, other times it's not an issue; it depends
entirely on what the memory you were trying to allocate was for.

> It might be possible come up with a name other than "fatal" that
> could apply to both gdb and gdbserver so that malloc_failure could
> call it, but no need to try to do that now.

I don't know if you saw but I removed "fatal" from GDB the other day
(http://tinyurl.com/k6neuwd) so the path is clear to add a "fatal" if
we want one.  It would be nice if such a function were smart enough to
work even if called before exceptions and cleanups were set up.  It
might be a nice general thing if all error-handling functions worked
that way (error and internal_error could work like gdbserver's "fatal"
if called early.)  That might even be a prerequisite to moving
exceptions and cleanups into common code and making gdbserver use
them.

(I'm not planning to do any of this now, this is just thinking aloud!)

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 1/3 v4] Introduce common/errors.h

[Doug Evans]

On Fri, Jul 25, 2014 at 1:51 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
> [...]
>> In the case of malloc_failure, it's not really an *internal* error,
>
> No--and certainly not by the description of internal_error earlier in
> the file.
>
>> even if in the case of gdb it calls internal_error, though arguably
>> it should do something different - it's more of a "fatal". :-)
>> Though I'm not suggesting trying to go down that path.  :-)
>
> I spoke with Pedro about this in Cambridge.  malloc failure is a funny
> one: sometimes it's fatal, other times it's not an issue; it depends
> entirely on what the memory you were trying to allocate was for.
>
>> It might be possible come up with a name other than "fatal" that
>> could apply to both gdb and gdbserver so that malloc_failure could
>> call it, but no need to try to do that now.
>
> I don't know if you saw but I removed "fatal" from GDB the other day

Indeed, saw that.

> (http://tinyurl.com/k6neuwd) so the path is clear to add a "fatal" if
> we want one.  It would be nice if such a function were smart enough to
> work even if called before exceptions and cleanups were set up.  It
> might be a nice general thing if all error-handling functions worked
> that way (error and internal_error could work like gdbserver's "fatal"
> if called early.)  That might even be a prerequisite to moving
> exceptions and cleanups into common code and making gdbserver use
> them.
>
> (I'm not planning to do any of this now, this is just thinking aloud!)

No worries!

[PATCH 3/3 v4] Make gdbserver CORE_ADDR unsigned

[Gary Benson]

gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
me; and furthermore likely to cause problems in common/, as it is
different from gdb's definition.

gdb/gdbserver/
2014-07-24  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* server.h (CORE_ADDR): Now unsigned.
---
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/server.h  |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 6eb1a16..2d55513 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -87,7 +87,7 @@ typedef unsigned char gdb_byte;
 
 /* FIXME: This should probably be autoconf'd for.  It's an integer type at
    least the size of a (void *).  */
-typedef long long CORE_ADDR;
+typedef unsigned long long CORE_ADDR;
 
 typedef long long LONGEST;
 typedef unsigned long long ULONGEST;
-- 
1.7.1

Re: [PATCH 3/3 v4] Make gdbserver CORE_ADDR unsigned

[Pedro Alves]

This one is OK.  Please push.

Thanks,
Pedro Alves

Re: [PATCH 3/3 v4] Make gdbserver CORE_ADDR unsigned

[Gary Benson]

Pedro Alves wrote:
> This one is OK.  Please push.

Pushed.

Thanks,
Gary

-- 
http://gbenson.net/