[patchv3 1/8] Move utility functions to common/

[patchv3 1/8] Move utility functions to common/

[Jan Kratochvil]

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

Hi,

some parts of the former patch have been reimplemented in the meantime by
other patches so this is only a part of the former cleanup.


Jan

[-- Attachment #2: common.patch --]
[-- Type: text/plain, Size: 9655 bytes --]

gdb/
2014-02-26  Aleksandar Ristovski  <aristovski@qnx.com

	Move utility functions to common/.
	* cli/cli-utils.c (skip_spaces, skip_spaces_const): Move defs to
	common/common-utils.c.
	* cli/cli-utils.h (skip_spaces, skip_spaces_const): Move decls to
	common/common-utils.h.
	* common/common-utils.c (ctype.h): Include.
	(HIGH_BYTE_POSN, is_digit_in_base, digit_to_int, strtoulst): Move
	from utils.c.
	(skip_spaces, skip_spaces_const): Move from cli/cli-utils.c.
	* common/common-utils.h (TARGET_CHAR_BIT, HOST_CHAR_BIT): Moved
	from defs.h.
	(strtoulst): Move decl from utils.h.
	(hex2bin, bin2hex): Move decls from remote.h.
	(skip_spaces, skip_spaces_const): Move decls from cli/cli-utils.h.
	* defs.h (TARGET_CHAR_BIT, HOST_CHAR_BIT): Move to
	common/common-utils.h
	* utils.c (HIGH_BYTE_POSN, is_digit_in_base, digit_to_int)
	(strtoulst): Move to common/common-utils.c.
	* utils.h (strtoulst): Moved decl to common/common-utils.h.

--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -213,30 +213,6 @@ number_is_in_list (char *list, int number)
 
 /* See documentation in cli-utils.h.  */
 
-char *
-skip_spaces (char *chp)
-{
-  if (chp == NULL)
-    return NULL;
-  while (*chp && isspace (*chp))
-    chp++;
-  return chp;
-}
-
-/* A const-correct version of the above.  */
-
-const char *
-skip_spaces_const (const char *chp)
-{
-  if (chp == NULL)
-    return NULL;
-  while (*chp && isspace (*chp))
-    chp++;
-  return chp;
-}
-
-/* See documentation in cli-utils.h.  */
-
 const char *
 skip_to_space_const (const char *chp)
 {
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -89,15 +89,6 @@ extern int get_number_or_range (struct get_number_or_range_state *state);
 
 extern int number_is_in_list (char *list, int number);
 
-/* Skip leading whitespace characters in INP, returning an updated
-   pointer.  If INP is NULL, return NULL.  */
-
-extern char *skip_spaces (char *inp);
-
-/* A const-correct version of the above.  */
-
-extern const char *skip_spaces_const (const char *inp);
-
 /* Skip leading non-whitespace characters in INP, returning an updated
    pointer.  If INP is NULL, return NULL.  */
 
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -28,6 +28,7 @@
 
 #include <stdlib.h>
 #include <stdio.h>
+#include <ctype.h>
 
 /* The xmalloc() (libiberty.h) family of memory management routines.
 
@@ -161,3 +162,126 @@ savestring (const char *ptr, size_t len)
   p[len] = 0;
   return p;
 }
+
+/* The bit offset of the highest byte in a ULONGEST, for overflow
+   checking.  */
+
+#define HIGH_BYTE_POSN ((sizeof (ULONGEST) - 1) * HOST_CHAR_BIT)
+
+/* True (non-zero) iff DIGIT is a valid digit in radix BASE,
+   where 2 <= BASE <= 36.  */
+
+static int
+is_digit_in_base (unsigned char digit, int base)
+{
+  if (!isalnum (digit))
+    return 0;
+  if (base <= 10)
+    return (isdigit (digit) && digit < base + '0');
+  else
+    return (isdigit (digit) || tolower (digit) < base - 10 + 'a');
+}
+
+static int
+digit_to_int (unsigned char c)
+{
+  if (isdigit (c))
+    return c - '0';
+  else
+    return tolower (c) - 'a' + 10;
+}
+
+/* As for strtoul, but for ULONGEST results.  */
+
+ULONGEST
+strtoulst (const char *num, const char **trailer, int base)
+{
+  unsigned int high_part;
+  ULONGEST result;
+  int minus = 0;
+  int i = 0;
+
+  /* Skip leading whitespace.  */
+  while (isspace (num[i]))
+    i++;
+
+  /* Handle prefixes.  */
+  if (num[i] == '+')
+    i++;
+  else if (num[i] == '-')
+    {
+      minus = 1;
+      i++;
+    }
+
+  if (base == 0 || base == 16)
+    {
+      if (num[i] == '0' && (num[i + 1] == 'x' || num[i + 1] == 'X'))
+	{
+	  i += 2;
+	  if (base == 0)
+	    base = 16;
+	}
+    }
+
+  if (base == 0 && num[i] == '0')
+    base = 8;
+
+  if (base == 0)
+    base = 10;
+
+  if (base < 2 || base > 36)
+    {
+      errno = EINVAL;
+      return 0;
+    }
+
+  result = high_part = 0;
+  for (; is_digit_in_base (num[i], base); i += 1)
+    {
+      result = result * base + digit_to_int (num[i]);
+      high_part = high_part * base + (unsigned int) (result >> HIGH_BYTE_POSN);
+      result &= ((ULONGEST) 1 << HIGH_BYTE_POSN) - 1;
+      if (high_part > 0xff)
+	{
+	  errno = ERANGE;
+	  result = ~ (ULONGEST) 0;
+	  high_part = 0;
+	  minus = 0;
+	  break;
+	}
+    }
+
+  if (trailer != NULL)
+    *trailer = &num[i];
+
+  result = result + ((ULONGEST) high_part << HIGH_BYTE_POSN);
+  if (minus)
+    return -result;
+  else
+    return result;
+}
+
+/* See documentation in cli-utils.h.  */
+
+char *
+skip_spaces (char *chp)
+{
+  if (chp == NULL)
+    return NULL;
+  while (*chp && isspace (*chp))
+    chp++;
+  return chp;
+}
+
+/* A const-correct version of the above.  */
+
+const char *
+skip_spaces_const (const char *chp)
+{
+  if (chp == NULL)
+    return NULL;
+  while (*chp && isspace (*chp))
+    chp++;
+  return chp;
+}
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -25,6 +25,25 @@
 #include <stddef.h>
 #include <stdarg.h>
 
+/* Static target-system-dependent parameters for GDB.  */
+
+/* * Number of bits in a char or unsigned char for the target machine.
+   Just like CHAR_BIT in <limits.h> but describes the target machine.  */
+#if !defined (TARGET_CHAR_BIT)
+#define TARGET_CHAR_BIT 8
+#endif
+
+/* * If we picked up a copy of CHAR_BIT from a configuration file
+   (which may get it by including <limits.h>) then use it to set
+   the number of bits in a host char.  If not, use the same size
+   as the target.  */
+
+#if defined (CHAR_BIT)
+#define HOST_CHAR_BIT CHAR_BIT
+#else
+#define HOST_CHAR_BIT TARGET_CHAR_BIT
+#endif
+
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
    defined, all uses must be protected by appropriate macro definition
@@ -71,4 +90,15 @@ int xsnprintf (char *str, size_t size, const char *format, ...)
 
 char *savestring (const char *ptr, size_t len);
 
+ULONGEST strtoulst (const char *num, const char **trailer, int base);
+
+/* Skip leading whitespace characters in INP, returning an updated
+   pointer.  If INP is NULL, return NULL.  */
+
+extern char *skip_spaces (char *inp);
+
+/* A const-correct version of the above.  */
+
+extern const char *skip_spaces_const (const char *inp);
+
 #endif
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -630,25 +630,6 @@ extern void *alloca ();
 
 enum { MAX_REGISTER_SIZE = 64 };
 
-/* Static target-system-dependent parameters for GDB.  */
-
-/* * Number of bits in a char or unsigned char for the target machine.
-   Just like CHAR_BIT in <limits.h> but describes the target machine.  */
-#if !defined (TARGET_CHAR_BIT)
-#define TARGET_CHAR_BIT 8
-#endif
-
-/* * If we picked up a copy of CHAR_BIT from a configuration file
-   (which may get it by including <limits.h>) then use it to set
-   the number of bits in a host char.  If not, use the same size
-   as the target.  */
-
-#if defined (CHAR_BIT)
-#define HOST_CHAR_BIT CHAR_BIT
-#else
-#define HOST_CHAR_BIT TARGET_CHAR_BIT
-#endif
-
 /* In findvar.c.  */
 
 extern LONGEST extract_signed_integer (const gdb_byte *, int,
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3020,105 +3020,6 @@ dummy_obstack_deallocate (void *object, void *data)
   return;
 }
 
-/* The bit offset of the highest byte in a ULONGEST, for overflow
-   checking.  */
-
-#define HIGH_BYTE_POSN ((sizeof (ULONGEST) - 1) * HOST_CHAR_BIT)
-
-/* True (non-zero) iff DIGIT is a valid digit in radix BASE,
-   where 2 <= BASE <= 36.  */
-
-static int
-is_digit_in_base (unsigned char digit, int base)
-{
-  if (!isalnum (digit))
-    return 0;
-  if (base <= 10)
-    return (isdigit (digit) && digit < base + '0');
-  else
-    return (isdigit (digit) || tolower (digit) < base - 10 + 'a');
-}
-
-static int
-digit_to_int (unsigned char c)
-{
-  if (isdigit (c))
-    return c - '0';
-  else
-    return tolower (c) - 'a' + 10;
-}
-
-/* As for strtoul, but for ULONGEST results.  */
-
-ULONGEST
-strtoulst (const char *num, const char **trailer, int base)
-{
-  unsigned int high_part;
-  ULONGEST result;
-  int minus = 0;
-  int i = 0;
-
-  /* Skip leading whitespace.  */
-  while (isspace (num[i]))
-    i++;
-
-  /* Handle prefixes.  */
-  if (num[i] == '+')
-    i++;
-  else if (num[i] == '-')
-    {
-      minus = 1;
-      i++;
-    }
-
-  if (base == 0 || base == 16)
-    {
-      if (num[i] == '0' && (num[i + 1] == 'x' || num[i + 1] == 'X'))
-	{
-	  i += 2;
-	  if (base == 0)
-	    base = 16;
-	}
-    }
-
-  if (base == 0 && num[i] == '0')
-    base = 8;
-
-  if (base == 0)
-    base = 10;
-
-  if (base < 2 || base > 36)
-    {
-      errno = EINVAL;
-      return 0;
-    }
-
-  result = high_part = 0;
-  for (; is_digit_in_base (num[i], base); i += 1)
-    {
-      result = result * base + digit_to_int (num[i]);
-      high_part = high_part * base + (unsigned int) (result >> HIGH_BYTE_POSN);
-      result &= ((ULONGEST) 1 << HIGH_BYTE_POSN) - 1;
-      if (high_part > 0xff)
-	{
-	  errno = ERANGE;
-	  result = ~ (ULONGEST) 0;
-	  high_part = 0;
-	  minus = 0;
-	  break;
-	}
-    }
-
-  if (trailer != NULL)
-    *trailer = &num[i];
-
-  result = result + ((ULONGEST) high_part << HIGH_BYTE_POSN);
-  if (minus)
-    return -result;
-  else
-    return result;
-}
-
 /* Simple, portable version of dirname that does not modify its
    argument.  */
 
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -41,8 +41,6 @@ extern int streq (const char *, const char *);
 
 extern int subset_compare (char *, char *);
 
-ULONGEST strtoulst (const char *num, const char **trailer, int base);
-
 int compare_positive_ints (const void *ap, const void *bp);
 int compare_strings (const void *ap, const void *bp);
 

Re: [patchv3 1/8] Move utility functions to common/

[Pedro Alves]

Looks fine to me, with a few nits.

On 02/27/2014 09:32 PM, Jan Kratochvil wrote:

> 	(strtoulst): Move decl from utils.h.
> 	(hex2bin, bin2hex): Move decls from remote.h.

This one looks stale.

> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -25,6 +25,25 @@
>  #include <stddef.h>
>  #include <stdarg.h>
>  
> +/* Static target-system-dependent parameters for GDB.  */
> +
> +/* * Number of bits in a char or unsigned char for the target machine.
> +   Just like CHAR_BIT in <limits.h> but describes the target machine.  */
> +#if !defined (TARGET_CHAR_BIT)
> +#define TARGET_CHAR_BIT 8
> +#endif
> +
> +/* * If we picked up a copy of CHAR_BIT from a configuration file
> +   (which may get it by including <limits.h>) then use it to set

Sounds like common-utils.h should include limits.h?  defs.h does
include it.

> +   the number of bits in a host char.  If not, use the same size
> +   as the target.  */
> +
> +#if defined (CHAR_BIT)
> +#define HOST_CHAR_BIT CHAR_BIT
> +#else
> +#define HOST_CHAR_BIT TARGET_CHAR_BIT
> +#endif

I think host-defs.h might be a better home for this.  (With the
target version placed in a new target-defs.h perhaps.  I won't
insist though.)

-- 
Pedro Alves

Re: [patchv3 1/8] Move utility functions to common/

[Jan Kratochvil]

On Fri, 28 Feb 2014 13:06:22 +0100, Pedro Alves wrote:
> On 02/27/2014 09:32 PM, Jan Kratochvil wrote:
> > 	(hex2bin, bin2hex): Move decls from remote.h.
> 
> This one looks stale.

Yes, removed.


> > --- a/gdb/common/common-utils.h
> > +++ b/gdb/common/common-utils.h
> > @@ -25,6 +25,25 @@
> >  #include <stddef.h>
> >  #include <stdarg.h>
> >  
> > +/* Static target-system-dependent parameters for GDB.  */
> > +
> > +/* * Number of bits in a char or unsigned char for the target machine.
> > +   Just like CHAR_BIT in <limits.h> but describes the target machine.  */
> > +#if !defined (TARGET_CHAR_BIT)
> > +#define TARGET_CHAR_BIT 8
> > +#endif
> > +
> > +/* * If we picked up a copy of CHAR_BIT from a configuration file
> > +   (which may get it by including <limits.h>) then use it to set
> 
> Sounds like common-utils.h should include limits.h?  defs.h does
> include it.

Primarily TARGET_CHAR_BIT and HOST_CHAR_BIT are obsolete, already C90 defines
CHAR_BIT as constant 8.

IIRC there was a discussion GDB could replace these macros by the number 8,
simplifying a lot of code.  Although IIRC there was agreement only on
HOST_CHAR_BIT, that TARGET_CHAR_BIT may possibly differ in some cases
(I do not believe it now, though).  But I do not have the mail reference now.

I have added <limits.h> there when the text references it.


> > +   the number of bits in a host char.  If not, use the same size
> > +   as the target.  */
> > +
> > +#if defined (CHAR_BIT)
> > +#define HOST_CHAR_BIT CHAR_BIT
> > +#else
> > +#define HOST_CHAR_BIT TARGET_CHAR_BIT
> > +#endif
> 
> I think host-defs.h might be a better home for this.  (With the
> target version placed in a new target-defs.h perhaps.  I won't
> insist though.)

But I am not going to create new files because of an obsolete macro which
should be in the first place deleted.


Thanks,
Jan

Re: [patchv3 1/8] Move utility functions to common/

[Pedro Alves]

On 02/28/2014 08:10 PM, Jan Kratochvil wrote:
> On Fri, 28 Feb 2014 13:06:22 +0100, Pedro Alves wrote:
>> On 02/27/2014 09:32 PM, Jan Kratochvil wrote:
>>> 	(hex2bin, bin2hex): Move decls from remote.h.
>>
>> This one looks stale.
> 
> Yes, removed.
> 
> 
>>> --- a/gdb/common/common-utils.h
>>> +++ b/gdb/common/common-utils.h
>>> @@ -25,6 +25,25 @@
>>>  #include <stddef.h>
>>>  #include <stdarg.h>
>>>  
>>> +/* Static target-system-dependent parameters for GDB.  */
>>> +
>>> +/* * Number of bits in a char or unsigned char for the target machine.
>>> +   Just like CHAR_BIT in <limits.h> but describes the target machine.  */
>>> +#if !defined (TARGET_CHAR_BIT)
>>> +#define TARGET_CHAR_BIT 8
>>> +#endif
>>> +
>>> +/* * If we picked up a copy of CHAR_BIT from a configuration file
>>> +   (which may get it by including <limits.h>) then use it to set
>>
>> Sounds like common-utils.h should include limits.h?  defs.h does
>> include it.
> 
> Primarily TARGET_CHAR_BIT and HOST_CHAR_BIT are obsolete,

There are gdb ports out there with TARGET_CHAR_BIT != 8.  We don't
have any in the tree currently, but I believe most of the code
we have actually works.  I wouldn't have a problem with accepting
such a port.

See "Clarification on what a byte is in -data-*-memory-bytes ?"
(https://sourceware.org/ml/gdb/2014-01/msg00046.html)
question on gdb@ just recently, about a TARGET_CHAR_BIT == 16 port.
( guess somebody should respond :-P )

It's my belief that if we accept such a port in the tree, others would
follow suit more quickly.  It's not that uncommon to see DSPs with
that word size, and my guess is that GDB ports for those aren't
submitted because the first will need to do the actual leg work
of fixing the bit rot properly.

> already C90 defines CHAR_BIT as constant 8.

Sorry, but that's not true.  I'm looking at N1256 (C99/TC3 draft),
and in 5.2.4.2.1, it shows:

  #define CHAR_BIT 8

_but_, it also says just above:

 "The minimum magnitudes shown shall be replaced by implementation-defined
  magnitudes with the same sign."

So that means it must be _at least_ 8-bit, but it also be wider than 8.

Even if that were not in the standard, there are machines out there
(not talking about ancient 9-bit machines, I'm thinking of mostly
DSPs, with 16- or 32-bit chars).  Ignoring them because of the standard
would be just burying the head in the sand.

I could see deleting HOST_CHAR_BIT, that's a different story.  I believe
_POSIX_ requires 8-bit, but I haven't checked.  A general purpose CPU
with char bit != 8-bit is indeed unthinkable to me in this day and age.

>>> +   the number of bits in a host char.  If not, use the same size
>>> +   as the target.  */
>>> +
>>> +#if defined (CHAR_BIT)
>>> +#define HOST_CHAR_BIT CHAR_BIT
>>> +#else
>>> +#define HOST_CHAR_BIT TARGET_CHAR_BIT
>>> +#endif
>>
>> I think host-defs.h might be a better home for this.  (With the
>> target version placed in a new target-defs.h perhaps.  I won't
>> insist though.)
> 
> But I am not going to create new files because of an obsolete macro which
> should be in the first place deleted.

I don't agree with deleting TARGET_CHAR_BIT though.  (I do
think it should be a gdbarch hook in gdb instead.).  host-defs.h
already exists, and this being macro representing a fundamental
characteristic of the host, it just feels better than a header
for common string utilities and such.  But *schrug*.  I don't
intend to fight further over this.  :-)  Anywhere is fine, really.

-- 
Pedro Alves

Re: [patchv3 1/8] Move utility functions to common/

[Jan Kratochvil]

On Fri, 28 Feb 2014 22:10:56 +0100, Pedro Alves wrote:
> There are gdb ports out there with TARGET_CHAR_BIT != 8.

OK.  They do not have to be C90/C99/POSIX.


> We don't have any in the tree currently, but I believe most of the code we
> have actually works.  I wouldn't have a problem with accepting such a port.

The problem is you have not accepted such a port.  And there is (probably
unwritten) rule that any GDB code not required by current codebase can be
removed.  With a versioning system any code can be easily returned back later.


> > already C90 defines CHAR_BIT as constant 8.
> 
> Sorry, but that's not true.  I'm looking at N1256 (C99/TC3 draft),
> and in 5.2.4.2.1, it shows:
> 
>   #define CHAR_BIT 8
> 
> _but_, it also says just above:
> 
>  "The minimum magnitudes shown shall be replaced by implementation-defined
>   magnitudes with the same sign."
> 
> So that means it must be _at least_ 8-bit, but it also be wider than 8.

It may be true but I am not completely sure with it.  The whole paragraph is:

	Moreover, except for CHAR_BIT and MB_LEN_MAX, the following shall be
	replaced by expressions that have the same type as would an expression
	that is an object of the corresponding type converted according to the
	integer promotions. Their implementation-defined values shall be equal
	or greater in magnitude (absolute value) to those shown, with the same
	sign.

Whether "except for CHAR_BIT and MB_LEN_MAX" applies only to the first
sentence or even to the last sentence is not clear to me, although it may be
more like you say.


> I could see deleting HOST_CHAR_BIT, that's a different story.

OK.


> host-defs.h
> already exists, and this being macro representing a fundamental
> characteristic of the host, it just feels better than a header
> for common string utilities and such.

OK, moved it to common/host-defs.h when that file already exists.


Thanks,
Jan