[2/2] Suppress printing of empty bitfields when printing flags
Commit Message
Consider a variable containing:
flags A=1 and B=0, 2 bit ranges X=3 and Y=0
When printing, empty flags are suppressed, giving:
$1 = [ A X=3 Y=0 ]
For consistency, it makes sense to also suppress any bit ranges which are
also 0. This now gives us:
$2 = [ A X=3 ]
Consider the CPSR register on AArch64 made up of many flags and bitfields.
DIT is a single bit flag, only used on Armv8.4-A, and TCO is a 2 bit field
only used on Armv8.5-A. Printing this in the existing code on Armv8.1-A
gives:
$3 = [C Z TCO=0]
This is confusing for the Armv8.1-A user as TCO does not exist, and would
be better shown as:
$4 = [C Z]
gdb/ChangeLog:
2019-06-06 Alan Hayward <alan.hayward@arm.com>
* valprint.c (val_print_type_code_flags): Suppress printing of
empty bitfields.
---
gdb/valprint.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
--
2.20.1 (Apple Git-117)
Comments
On 6/6/19 5:23 AM, Alan Hayward wrote:
> Consider a variable containing:
> flags A=1 and B=0, 2 bit ranges X=3 and Y=0
>
> When printing, empty flags are suppressed, giving:
> $1 = [ A X=3 Y=0 ]
>
> For consistency, it makes sense to also suppress any bit ranges which are
> also 0. This now gives us:
> $2 = [ A X=3 ]
>
> Consider the CPSR register on AArch64 made up of many flags and bitfields.
> DIT is a single bit flag, only used on Armv8.4-A, and TCO is a 2 bit field
> only used on Armv8.5-A. Printing this in the existing code on Armv8.1-A
> gives:
> $3 = [C Z TCO=0]
> This is confusing for the Armv8.1-A user as TCO does not exist, and would
> be better shown as:
> $4 = [C Z]
The only downside to this is that you may have fields for which 0 is a valid
value. Consider the IOPL field in the x86 eflags/rflags register, it is a
2-bit field for which all of the values (0-3) are valid. There isn't an
"unused" field. The other alternative you could use that might be more
accurate for your use case (but more work) would be to use a feature for
CPSR itself with different feature xml descriptions for different
architecture versions and choose the right one based on the arch. That seems
like a fair bit of work though (and I'm not sure how readily available the
arch version would even be for core dumps and native targets).
> On 6 Jun 2019, at 17:03, John Baldwin <jhb@FreeBSD.org> wrote:
>
> On 6/6/19 5:23 AM, Alan Hayward wrote:
>> Consider a variable containing:
>> flags A=1 and B=0, 2 bit ranges X=3 and Y=0
>>
>> When printing, empty flags are suppressed, giving:
>> $1 = [ A X=3 Y=0 ]
>>
>> For consistency, it makes sense to also suppress any bit ranges which are
>> also 0. This now gives us:
>> $2 = [ A X=3 ]
>>
>> Consider the CPSR register on AArch64 made up of many flags and bitfields.
>> DIT is a single bit flag, only used on Armv8.4-A, and TCO is a 2 bit field
>> only used on Armv8.5-A. Printing this in the existing code on Armv8.1-A
>> gives:
>> $3 = [C Z TCO=0]
>> This is confusing for the Armv8.1-A user as TCO does not exist, and would
>> be better shown as:
>> $4 = [C Z]
>
> The only downside to this is that you may have fields for which 0 is a valid
> value. Consider the IOPL field in the x86 eflags/rflags register, it is a
> 2-bit field for which all of the values (0-3) are valid. There isn't an
> "unused" field.
That’s fair enough. I’d still say my version works for that case as it’s
obvious what the value of IOPL is if it’s missing from the print. However,
I doubt anyone is going to OK that as a change.
> The other alternative you could use that might be more
> accurate for your use case (but more work) would be to use a feature for
> CPSR itself with different feature xml descriptions for different
> architecture versions and choose the right one based on the arch. That seems
> like a fair bit of work though (and I'm not sure how readily available the
> arch version would even be for core dumps and native targets).
That feels like the correct solution. Given the xml descriptions don’t require
the xml file, that part might not be too tricky. Getting the arch version might
be though. I’ll look into it a bit more.
>
> --
> John Baldwin
@@ -1225,7 +1225,15 @@ val_print_type_code_flags (struct type *type, const gdb_byte *valaddr,
fputs_filtered ("[", stream);
for (field = 0; field < nfields; field++)
{
- if (TYPE_FIELD_NAME (type, field)[0] != '\0')
+ unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
+ ULONGEST field_val
+ = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
+ const char *field_name = TYPE_FIELD_NAME (type, field);
+
+ if (field_len < sizeof (ULONGEST) * TARGET_CHAR_BIT)
+ field_val &= ((ULONGEST) 1 << field_len) - 1;
+
+ if (field_name[0] != '\0' && field_val != 0)
{
struct type *field_type = TYPE_FIELD_TYPE (type, field);
@@ -1234,22 +1242,11 @@ val_print_type_code_flags (struct type *type, const gdb_byte *valaddr,
problematic place to notify the user of an internal error
though. Instead just fall through and print the field as an
int. */
- && TYPE_FIELD_BITSIZE (type, field) == 1)
- {
- if (val & ((ULONGEST)1 << TYPE_FIELD_BITPOS (type, field)))
- fprintf_filtered (stream, " %s",
- TYPE_FIELD_NAME (type, field));
- }
+ && field_len == 1)
+ fprintf_filtered (stream, " %s", field_name);
else
{
- unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
- ULONGEST field_val
- = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
-
- if (field_len < sizeof (ULONGEST) * TARGET_CHAR_BIT)
- field_val &= ((ULONGEST) 1 << field_len) - 1;
- fprintf_filtered (stream, " %s=",
- TYPE_FIELD_NAME (type, field));
+ fprintf_filtered (stream, " %s=", field_name);
if (TYPE_CODE (field_type) == TYPE_CODE_ENUM)
generic_val_print_enum_1 (field_type, field_val, stream);
else