RFA: fix PR 9164

RFA: fix PR 9164

[Tom Tromey]

This patch fixes PR 9164.

The bug is that the result of sizeof has signed type, not unsigned.
The C and C++ standards require an unsigned type here.

This fix defers the choice of type to the language, using the existing
language-arch machinery.  I fixed the C language family, including
ObjC, but I left the other languages unchanged.  I think the language
maintainers will have to make a change here, if one is needed or
desired.  (FWIW I don't think Java needs a change, since I don't think
it is possible to invoke sizeof when Java is the selected language.)

Built and regtested on x86-64 (compile farm).
New test case included.

Please review.

thanks,
Tom

2009-01-03  Tom Tromey  <tromey@redhat.com>

	PR gdb/9164:
	* p-lang.c (pascal_language_arch_info): Initialize
	lai->size_type_default.
	* objc-exp.y (exp): Call language_size_type.
	* m2-lang.c (m2_language_arch_info): Initialize
	lai->size_type_default.
	* language.h (language_size_type): Declare.
	(struct language_arch_info) <size_type_symbol, size_type_default>:
	New fields.
	* language.c (unknown_language_arch_info): Initialize
	lai->size_type_default.
	(language_size_type): New function.
	* jv-lang.c (java_language_arch_info): Initialize
	lai->size_type_default.
	* f-lang.c (f_language_arch_info): Initialize
	lai->size_type_default.
	* eval.c (evaluate_subexp_for_sizeof): Call language_size_type.
	* c-lang.c (find_size_type): New function.
	(c_language_arch_info): Call it.
	(cplus_language_arch_info): Likewise.
	* c-exp.y (exp): Use language_size_type.
	* ada-lang.c (ada_language_arch_info): Initialize
	lai->size_type_default.

2009-01-03  Tom Tromey  <tromey@redhat.com>

	* gdb.base/sizeof.exp (check_sizeof): Verify that sizeof returns
	unsigned type.

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index bcbd709..8571eda 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11000,6 +11000,8 @@ ada_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "boolean";
   lai->bool_type_default = builtin->builtin_bool;
+
+  lai->size_type_default = builtin->builtin_int;
 }
 \f
 				/* Language vector */
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 0db8c0b..212d7b5 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -558,7 +558,8 @@ exp	:	VARIABLE
 
 exp	:	SIZEOF '(' type ')'	%prec UNARY
 			{ write_exp_elt_opcode (OP_LONG);
-			  write_exp_elt_type (parse_type->builtin_int);
+			  write_exp_elt_type (language_size_type (parse_language,
+								  parse_gdbarch));
 			  CHECK_TYPEDEF ($3);
 			  write_exp_elt_longcst ((LONGEST) TYPE_LENGTH ($3));
 			  write_exp_elt_opcode (OP_LONG); }
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index dc7b059..f3020f6 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -223,6 +223,25 @@ const struct op_print c_op_print_tab[] =
   {NULL, 0, 0, 0}
 };
 \f
+/* A helper function to compute the default size type for C-like
+   languages.  */
+static void
+find_size_type (const struct builtin_type *builtin,
+		struct language_arch_info *lai)
+{
+  lai->size_type_symbol = "size_t";
+
+  /* For the default, pick an unsigned integral type the same width as
+     a data pointer, if possible.  If none match, use unsigned int.  */
+  if (TYPE_LENGTH (builtin->builtin_unsigned_long)
+      == TYPE_LENGTH (builtin->builtin_data_ptr))
+    lai->size_type_default = builtin->builtin_unsigned_long;
+  else if (TYPE_LENGTH (builtin->builtin_unsigned_long_long))
+    lai->size_type_default = builtin->builtin_unsigned_long_long;
+  else
+    lai->size_type_default = builtin->builtin_unsigned_int;
+}
+
 enum c_primitive_types {
   c_primitive_type_int,
   c_primitive_type_long,
@@ -278,6 +297,8 @@ c_language_arch_info (struct gdbarch *gdbarch,
   lai->primitive_type_vector [c_primitive_type_declong] = builtin->builtin_declong;
 
   lai->bool_type_default = builtin->builtin_int;
+
+  find_size_type (builtin, lai);
 }
 
 const struct language_defn c_language_defn =
@@ -396,6 +417,8 @@ cplus_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "bool";
   lai->bool_type_default = builtin->builtin_bool;
+
+  find_size_type (builtin, lai);
 }
 
 const struct language_defn cplus_language_defn =
diff --git a/gdb/eval.c b/gdb/eval.c
index ccb6b74..026795b 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2652,8 +2652,8 @@ evaluate_subexp_with_coercion (struct expression *exp,
 static struct value *
 evaluate_subexp_for_sizeof (struct expression *exp, int *pos)
 {
-  /* FIXME: This should be size_t.  */
-  struct type *size_type = builtin_type (exp->gdbarch)->builtin_int;
+  struct type *size_type = language_size_type (exp->language_defn,
+					       exp->gdbarch);
   enum exp_opcode op;
   int pc;
   struct type *type;
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 4d4d4d7..bf4dac5 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -302,6 +302,8 @@ f_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "logical";
   lai->bool_type_default = builtin->builtin_logical_s2;
+
+  lai->size_type_default = builtin_type (gdbarch)->builtin_int;
 }
 
 /* This is declared in c-lang.h but it is silly to import that file for what
diff --git a/gdb/jv-lang.c b/gdb/jv-lang.c
index 0470eef..4f75f7b 100644
--- a/gdb/jv-lang.c
+++ b/gdb/jv-lang.c
@@ -1083,6 +1083,8 @@ java_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "boolean";
   lai->bool_type_default = java_boolean_type;
+
+  lai->size_type_default = builtin_type (gdbarch)->builtin_int;
 }
 
 const struct exp_descriptor exp_descriptor_java = 
diff --git a/gdb/language.c b/gdb/language.c
index 46e238d..c6eec0a 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -1127,6 +1127,7 @@ unknown_language_arch_info (struct gdbarch *gdbarch,
 {
   lai->string_char_type = builtin_type (gdbarch)->builtin_char;
   lai->bool_type_default = builtin_type (gdbarch)->builtin_int;
+  lai->size_type_default = builtin_type (gdbarch)->builtin_int;
   lai->primitive_type_vector = GDBARCH_OBSTACK_CALLOC (gdbarch, 1,
 						       struct type *);
 }
@@ -1303,6 +1304,33 @@ language_bool_type (const struct language_defn *la,
   return ld->arch_info[la->la_language].bool_type_default;
 }
 
+/* Return the size type for the given language and architecture.  The
+   size type is the type used to represent the size of an object, for
+   instance the result of the C `sizeof' operator.  */
+
+struct type *
+language_size_type (const struct language_defn *la,
+		    struct gdbarch *gdbarch)
+{
+  struct language_gdbarch *ld = gdbarch_data (gdbarch,
+					      language_gdbarch_data);
+
+  if (ld->arch_info[la->la_language].size_type_symbol)
+    {
+      struct symbol *sym;
+      sym = lookup_symbol (ld->arch_info[la->la_language].size_type_symbol,
+			   NULL, VAR_DOMAIN, NULL);
+      if (sym)
+	{
+	  struct type *type = SYMBOL_TYPE (sym);
+	  if (type && TYPE_CODE (type) == TYPE_CODE_INT)
+	    return type;
+	}
+    }
+
+  return ld->arch_info[la->la_language].size_type_default;
+}
+
 struct type *
 language_lookup_primitive_type_by_name (const struct language_defn *la,
 					struct gdbarch *gdbarch,
diff --git a/gdb/language.h b/gdb/language.h
index c92c57c..f09c029 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -134,6 +134,11 @@ struct language_arch_info
   const char *bool_type_symbol;
   /* Otherwise, this is the default boolean builtin type.  */
   struct type *bool_type_default;
+
+  /* Symbol name of type to use as size type, if defined.  */
+  const char *size_type_symbol;
+  /* Otherwise, this is the default builtin size type.  */
+  struct type *size_type_default;
 };
 
 /* Structure tying together assorted information about a language.  */
@@ -331,6 +336,9 @@ struct type *language_bool_type (const struct language_defn *l,
 struct type *language_string_char_type (const struct language_defn *l,
 					struct gdbarch *gdbarch);
 
+struct type *language_size_type (const struct language_defn *l,
+				 struct gdbarch *gdbarch);
+
 struct type *language_lookup_primitive_type_by_name (const struct language_defn *l,
 						     struct gdbarch *gdbarch,
 						     const char *name);
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index e09b64b..fa6fcaf 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -349,6 +349,8 @@ m2_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "BOOLEAN";
   lai->bool_type_default = builtin->builtin_bool;
+
+  lai->size_type_default = builtin->builtin_int;
 }
 
 const struct exp_descriptor exp_descriptor_modula2 = 
diff --git a/gdb/objc-exp.y b/gdb/objc-exp.y
index 1527a04..aa4f738 100644
--- a/gdb/objc-exp.y
+++ b/gdb/objc-exp.y
@@ -577,7 +577,8 @@ exp	:	SELECTOR
 
 exp	:	SIZEOF '(' type ')'	%prec UNARY
 			{ write_exp_elt_opcode (OP_LONG);
-			  write_exp_elt_type (parse_type->builtin_int);
+			  write_exp_elt_type (language_size_type (parse_language,
+								  parse_gdbarch));
 			  CHECK_TYPEDEF ($3);
 			  write_exp_elt_longcst ((LONGEST) TYPE_LENGTH ($3));
 			  write_exp_elt_opcode (OP_LONG); }
diff --git a/gdb/p-lang.c b/gdb/p-lang.c
index cd4285d..bd82c48 100644
--- a/gdb/p-lang.c
+++ b/gdb/p-lang.c
@@ -397,6 +397,8 @@ pascal_language_arch_info (struct gdbarch *gdbarch,
 
   lai->bool_type_symbol = "boolean";
   lai->bool_type_default = builtin->builtin_bool;
+
+  lai->size_type_default = builtin->builtin_int;
 }
 
 const struct language_defn pascal_language_defn =
diff --git a/gdb/testsuite/gdb.base/sizeof.exp b/gdb/testsuite/gdb.base/sizeof.exp
index c6432bf..421d3ea 100644
--- a/gdb/testsuite/gdb.base/sizeof.exp
+++ b/gdb/testsuite/gdb.base/sizeof.exp
@@ -118,6 +118,10 @@ check_sizeof "float" ${sizeof_float}
 check_sizeof "double" ${sizeof_double}
 check_sizeof "long double" ${sizeof_long_double}
 
+# Check that sizeof has unsigned type.
+gdb_test "print -sizeof(int)" \
+  "\\$\[0-9\]* = (\[0-9\]*)"
+
 proc check_valueof { exp val } {
     global gdb_prompt
 

Re: RFA: fix PR 9164

[Daniel Jacobowitz]

On Sat, Jan 03, 2009 at 02:19:53PM -0700, Tom Tromey wrote:
> This patch fixes PR 9164.
> 
> The bug is that the result of sizeof has signed type, not unsigned.
> The C and C++ standards require an unsigned type here.
> 
> This fix defers the choice of type to the language, using the existing
> language-arch machinery.  I fixed the C language family, including
> ObjC, but I left the other languages unchanged.  I think the language
> maintainers will have to make a change here, if one is needed or
> desired.  (FWIW I don't think Java needs a change, since I don't think
> it is possible to invoke sizeof when Java is the selected language.)
> 
> Built and regtested on x86-64 (compile farm).
> New test case included.

Before adding all this, do you know of any language where the signed
behavior is correct?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: fix PR 9164

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Tom> This fix defers the choice of type to the language, using the existing
Tom> language-arch machinery.  I fixed the C language family, including
Tom> ObjC, but I left the other languages unchanged.  I think the language
Tom> maintainers will have to make a change here, if one is needed or
Tom> desired.  (FWIW I don't think Java needs a change, since I don't think
Tom> it is possible to invoke sizeof when Java is the selected language.)

Daniel> Before adding all this, do you know of any language where the signed
Daniel> behavior is correct?

Nope.  I don't know that a signed type is incorrect, either.  I really
know nothing about the other languages here, so I chose to preserve
the current behavior.

Even if all languages want an unsigned type, I still think something
like this patch is the way to go, because for C languages you
generally want the inferior's "size_t".

That is, this is the way to go assuming that we continue to have
language-dependent expressions.  If it weren't for UNOP_SIZEOF, I
probably would have made this a C-specific patch.  Another idea would
be to add an evaluate_subexp_c, and have it handle UNOP_SIZEOF.

Tom

Re: RFA: fix PR 9164

[Joel Brobecker]

> Tom> This fix defers the choice of type to the language, using the existing
> Tom> language-arch machinery.  I fixed the C language family, including
> Tom> ObjC, but I left the other languages unchanged.  I think the language
> Tom> maintainers will have to make a change here, if one is needed or
> Tom> desired.  (FWIW I don't think Java needs a change, since I don't think
> Tom> it is possible to invoke sizeof when Java is the selected language.)
> 
> Daniel> Before adding all this, do you know of any language where the signed
> Daniel> behavior is correct?
> 
> Nope.  I don't know that a signed type is incorrect, either.  I really
> know nothing about the other languages here, so I chose to preserve
> the current behavior.

In Ada, the equivalent of the sizeof operator is the 'Size attribute.
This attribute is a function that returns the size in bits and its
type is a ``universal_integer''. universal_integer is not a type
you can use in declarations, but it allows any integer values, so
I think that it would be reasonable to make the 'size attribute
return a value of Integer type (thus signed). I checked the code,
and we actually use builtin_type_int32 (we used to use builtin_type_int
but this type is hard to get to, now, as you need access to the gdbarch).

-- 
Joel

Re: RFA: fix PR 9164

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> Nope.  I don't know that a signed type is incorrect, either.  I really
Tom> know nothing about the other languages here, so I chose to preserve
Tom> the current behavior.

Joel> In Ada, the equivalent of the sizeof operator is the 'Size attribute.

FWIW -- I believe this change only affects languages that might
generate UNOP_SIZEOF in an expression.  According to grep that means C
(the whole family plus ObjC), Fortran, Modula-2, and Pascal.

So, I think this change should not affect Ada.

Tom

Re: RFA: fix PR 9164

[Mark Kettenis]

> Date: Mon, 5 Jan 2009 08:23:36 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > Tom> This fix defers the choice of type to the language, using the existing
> > Tom> language-arch machinery.  I fixed the C language family, including
> > Tom> ObjC, but I left the other languages unchanged.  I think the language
> > Tom> maintainers will have to make a change here, if one is needed or
> > Tom> desired.  (FWIW I don't think Java needs a change, since I don't think
> > Tom> it is possible to invoke sizeof when Java is the selected language.)
> > 
> > Daniel> Before adding all this, do you know of any language where the signed
> > Daniel> behavior is correct?
> > 
> > Nope.  I don't know that a signed type is incorrect, either.  I really
> > know nothing about the other languages here, so I chose to preserve
> > the current behavior.
> 
> In Ada, the equivalent of the sizeof operator is the 'Size attribute.
> This attribute is a function that returns the size in bits and its
> type is a ``universal_integer''. universal_integer is not a type
> you can use in declarations, but it allows any integer values, so
> I think that it would be reasonable to make the 'size attribute
> return a value of Integer type (thus signed). I checked the code,
> and we actually use builtin_type_int32 (we used to use builtin_type_int
> but this type is hard to get to, now, as you need access to the gdbarch).

Well, using built_type_int32 defenitely feels wrong for a type
representing sizes on a 64-bit system.

Re: RFA: fix PR 9164

[Joel Brobecker]

> Well, using built_type_int32 defenitely feels wrong for a type
> representing sizes on a 64-bit system.

We could extend it to a larger integer type, but this is only used
to return type sizes - the assumption is that we won't see many
objects whose size is bits does not fit in a 32bit integer.

-- 
Joel

Re: RFA: fix PR 9164

[Pedro Alves]

On Saturday 03 January 2009 21:19:53, Tom Tromey wrote:
> +static void
> +find_size_type (const struct builtin_type *builtin,
> +               struct language_arch_info *lai)
> +{
> +  lai->size_type_symbol = "size_t";
> +
> +  /* For the default, pick an unsigned integral type the same width as
> +     a data pointer, if possible.  If none match, use unsigned int.  */
> +  if (TYPE_LENGTH (builtin->builtin_unsigned_long)
> +      == TYPE_LENGTH (builtin->builtin_data_ptr))
> +    lai->size_type_default = builtin->builtin_unsigned_long;
> +  else if (TYPE_LENGTH (builtin->builtin_unsigned_long_long))
> +    lai->size_type_default = builtin->builtin_unsigned_long_long;
> +  else
> +    lai->size_type_default = builtin->builtin_unsigned_int;
> +}

I think that m32c will need a hook here.  I think that in some modes, size_t will
be 16-bit, but pointer width 24-bits.

I wonder if the above shouldn't be a gdbarch method, that defaults to
what you wrote.

-- 
Pedro Alves

Re: RFA: fix PR 9164

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I think that m32c will need a hook here.  I think that in some
Pedro> modes, size_t will be 16-bit, but pointer width 24-bits.

Pedro> I wonder if the above shouldn't be a gdbarch method, that defaults to
Pedro> what you wrote.

Yeah, I do see some gcc ports where size_t depends on the flags.
Not m32c, actually, but:

../arm/arm.h:597:#define SIZE_TYPE (TARGET_AAPCS_BASED ? "unsigned int" : "long unsigned int")

... all the other conditionally-defined ones seem to be decided by the
size of pointers.

I'll look into this more.

Tom

Re: RFA: fix PR 9164

[Pedro Alves]

On Monday 05 January 2009 18:29:41, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> I think that m32c will need a hook here.  I think that in some
> Pedro> modes, size_t will be 16-bit, but pointer width 24-bits.
> 
> Pedro> I wonder if the above shouldn't be a gdbarch method, that defaults to
> Pedro> what you wrote.
> 
> Yeah, I do see some gcc ports where size_t depends on the flags.
> Not m32c, actually, but:

Ah, you're only looking for SIZE_TYPE:

/* We can do QI, HI, and SI operations pretty much equally well, but
   GCC expects us to have a "native" format, so we pick the one that
   matches "int".  Pointers are 16 bits for R8C/M16C (when TARGET_A16
   is true) and 24 bits for M32CM/M32C (when TARGET_A24 is true), but
   24-bit pointers are stored in 32-bit words.  */
#define BITS_PER_UNIT 8
#define UNITS_PER_WORD 2
#define POINTER_SIZE (TARGET_A16 ? 16 : 32)
#define POINTERS_EXTEND_UNSIGNED 1
...
#define INT_TYPE_SIZE 16

So, SIZE_TYPE defaults to unsigned int --- 16-bit, but the pointer size
varies.  This means that objects can only be 16-bit big, but they can
exist in a larger than 16-bit address space.

-- 
Pedro Alves