Re: [PATCH] Fixup internal variables' endian (again)

[Daniel Jacobowitz <drow at false dot org>]

On Fri, Mar 03, 2006 at 06:01:32PM +0000, Andrew STUBBS wrote:
> Hi all,
> 
> This is a rework of a patch I submitted last autumn. I have updated it 
> to fit with the new convenience variable preservation and rethought the 
> requirements.
> 
> The patch fixes up the endian of all integer and pointer internal 
> variables and leaves all other types alone.

Hi Andrew,

This seems like a sensible thing to do now, and a sensible way to do
it.

> My understanding is that an internal variable cannot contain anything 
> other than a built-in type. Any other value is merely a memory 
> reference. Indeed, value_of_internalvar() goes out of it's way to ensure 
> that values loaded from memory are never saved long term. The result is 
> that there is no point in attempting to do anything with the endian of 
> these values because they will always be of the same endian as the 
> target/program being debugged.

I'm pretty sure that we can come up with ways to get structs into
internal variables, et cetera.  But, this is a best-effort thing;
I think it's OK as long as we do better than we do now, since it's
pretty apparent that what we do now is not useful.

So, just minor comments.

> +  /* Values are always stored in the target's byte order.  When connected to a
> +     target this will most likely always be correct, so there's normally no
> +     need to worry about it.
> +
> +     However, internal variables can be set up before the target endian is
> +     known and so may become out of date.  Fix it up before anybody sees.
> +
> +     Since internal variables cannot hold complex types, such as structs,
> +     unions, arrays and strings - those are always held in target memory and
> +     the variable just holds a reference, there is no need to worry about
> +     those either.
> +
> +     Floating point values vary differently across endianness - many targets
> +     just keep them the same.  If they do no work correctly on your host/target
> +     then add support as required here.  */

How do you feel about replacing those last two paragraphs with:

    Internal variables usually hold simple scalar values, and we can
    correct those.  More complex values (e.g. structures and floating
    point types) are left alone, because they would be too complicated
    to correct.

I don't think we really want someone to fix this by adding floating
point support.

> +  if (var->endian != TARGET_BYTE_ORDER)
> +    {
> +      array = value_contents_raw (val);
> +      switch (TYPE_CODE (value_type (val)))

You should use type = check_typedef (value_type (val)) here; that's the
other easy case.

> +        {
> +        case TYPE_CODE_INT:
> +        case TYPE_CODE_PTR:
> +          /* Reverse the bytes.  */
> +          for (i=0,j=TYPE_LENGTH (value_enclosing_type (val))-1; i<j; i++,j--)

Formatting; you've skipped a lot of customary whitespace here.  We have
vertical space and we know how to use it!

	  for (i=0, j=TYPE_LENGTH (value_enclosing_type (val)) - 1;
	       i < j;
	       i++, j--)

Also, I think you used eight spaces instead of a tab here; the diff
looked funny.

> -      value_print (var->value, gdb_stdout, 0, Val_pretty_default);
> +      value_print (value_of_internalvar (var), gdb_stdout, 0, Val_pretty_default);

And this line needs to be wrapped, it's too long now.

-- 
Daniel Jacobowitz
CodeSourcery