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

Tom Tromey
2019-05-22T19:37:06Z

>>>>> "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