Re: pr9065 patch

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> This is the typeid thing.
Chris> This patch implements typeid in gdb, letting users do things like

Chris> There are some differences, though, between the gdb typeid and the c++
Chris> typeid, the biggest is that the gdb version only partially implements
Chris> the type_info class: it doesn't actually implement the methods, it
Chris> just fakes them, and even though you can assign things like "set tp =
Chris> typeid(<expression>).name()" you can't assign "set ti =
Chris> typeid(<expression>)" where "type_info *ti;"

I think our goal ought to be no differences from the C++ compiler.

It isn't actually that useful, I think, to just print a typeid in gdb.
We have other, better, mechanisms for inspecting types.  Instead, the
reason to implement typeid in the first place is to preserve the "cut
and paste" property: if the user cuts an expression from their source
and pastes it into gdb, it should yield the same answer as their
compiled code would.

Deviations from this principle are confusing to users.

Chris> Another difference is that the real typeid returns mangled names,
Chris> leaving it to the user to demangle.  This strikes me as a pain, so I
Chris> just provide the demangled names.

It should be compatible.  GDB has a demangle command for this purpose.

Chris> +exp	:       TYPEID '(' type ')' '.' name opt_args	%prec UNARY
Chris> +                        { write_exp_elt_opcode (OP_STRING);
Chris> +			  write_exp_string ($6);
Chris> +			  write_exp_elt_opcode (OP_STRING);

I guess the problem is that type_id::name is an inlined function with no
out-of-line instance in libstdc++.  So, there is no way to call it.

I think we should have a way to make a fake method, but I don't think it
should be implemented in an ad hoc way.  Instead it should be a general
facility.  That should be a separate patch.  For the plain "introduce
typeid" patch, I think it is fine if it just barfs for anything not
provided by libstc++.

This is what Daniel was referring to when he mentioned writing
"not-outlined" methods in Python.  And, I agree that this would be the
way to go; for one thing it would let library implementers provide the
implementations, alongside their pretty-printers and whatnot.

Chris> +static struct value *
Chris> +create_type_info_class(char *type_name)

New functions need an introductory comment.

There is a missing space before the "("; this occurs in many places
in the patch.

It is best to be const-correct.  I assume the argument can be a
const char *.

Chris> +      val = allocate_value (typeid_type);
Chris> +      
Chris> +      inferior_val = value_allocate_space_in_inferior (1 + strlen (type_name));
Chris> +      inferior_addr = value_as_address (inferior_val);
Chris> +      write_memory (inferior_addr,
Chris> +		    type_name, strlen (type_name));
Chris> +      
Chris> +      memcpy (value_contents_raw (val) + bitpos/8,
Chris> +	      &inferior_addr, sizeof(inferior_addr));
Chris> +      rv = value_coerce_to_target (val);

The call to write_memory is not copying the trailing \0.

This code seems odd to me.  IIUC, the type_info for a type is stored as
a global symbol.  This way, typeid always returns the same object for a
given type, and thus the result can be compared with ==.  (This is true
on some platforms anyhow, there are tweaks for other cases, see
libsupc++.)  Making our own type_info is going to yield incorrect
results where there is a single type_info object per type.

Chris> +static struct value *
Chris> +fake_type_info_class(char *type_name, char *field_name, struct expression *exp,
Chris> +		     int is_pointer, int is_function, int field_has_args)

This should probably just go away.  As above, I don't think this is the
way we want to handle the missing methods.

Chris> +	    We're going to make the assumption that class typeinfo
Chris> +	    isn't going to change.  Given that, we need to handle:

This is a reasonable assumption for the current ABI, but it is not
reasonable to assume it universally.  If this sort of thing is still
needed, you have to hide it behind the appropriate API, see cp-abi.h.

Chris> +static struct value *
Chris> +evaluate_subexp_for_typeid_type (struct expression *exp, int *pos)
Chris> +     __attribute__ ((noinline));

Just remove this.  For future reference, you can't use __attribute__
unconditionally in (most of) gdb, you have to hide it behind a define.
See defs.h.

Chris> +  if (TYPE_CODE_STRUCT ==  TYPE_CODE (type))
Chris> +    {
Chris> +#define STRUCT_LBL "struct "
Chris> +      char * tmp = alloca (1 + strlen (STRUCT_LBL) + strlen (type_name));
Chris> +      strcpy (tmp, STRUCT_LBL);

I think it is nice to #undef such local defines after their use.
That way, it is almost like they have proper scoping.

Tom