Re: [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> When the type of a property is smaller than the CORE_ADDR in which the
Andrew> property value has been placed, and if the property is signed, then
Andrew> sign extend the property value from its actual type up to the size of
Andrew> CORE_ADDR.

I wonder whether this should be using ULONGEST rather than CORE_ADDR
now.

Andrew> +		    /* If we have a valid return candidate and it's value
Andrew> +		       is signed, we have to sign-extend the value because
Andrew> +		       CORE_ADDR on 64bit machine has 8 bytes but address
Andrew> +		       size of an 32bit application is bytes.  */
Andrew> +		    const int addr_size
Andrew> +		      = (dwarf2_per_cu_addr_size (baton->locexpr.per_cu)
Andrew> +			 * TARGET_CHAR_BIT);

I'm somewhat surprised there isn't an existing sign_extend function
somewhere.

Andrew> +		    const CORE_ADDR neg_mask = ((~0) <<  (addr_size - 1));

I tend to think this should say ~(CORE_ADDR) 0 rather than just ~0.
Otherwise won't this do the wrong thing if sizeof(int) < sizeof(CORE_ADDR)?

Andrew> +		    /* Check if signed bit is set and sign-extend values.  */
Andrew> +		    if (*value & (neg_mask))
Andrew> +		      *value |= (neg_mask );

Extra parens and an extra space here.

mips-tdep.c has the fun sign extension idiom:

	    value = inst & 0x7ff;
	    value = (value ^ 0x400) - 0x400;		/* Sign-extend.  */

(If the high bits are 0 you can skip the "&".)
This one avoids the ~0 business.

Another approach is to let the compiler handle it.

   LONGEST l = *value;
   l = (l << nbits) >> nbits;

However this may be relying on undefined behavior.

Yours is fine, though, I just noticed these while digging around and
wanted to point them out :-)

Tom