This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 2/2] Implement pahole-like 'ptype /o' option
On Monday, December 11 2017, Simon Marchi wrote:
> On 2017-12-11 18:24, Sergio Durigan Junior wrote:
>>>> +
>>>> + The output is strongly based on pahole(1). */
>>>> +
>>>> +static void
>>>> +c_print_type_struct_field_offset (struct type *type, unsigned int
>>>> field_idx,
>>>> + unsigned int *endpos, struct ui_file *stream,
>>>> + unsigned int offset_bitpos)
>>>> +{
>>>> + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type,
>>>> field_idx));
>>>> + unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx);
>>>> + unsigned int fieldsize_byte = TYPE_LENGTH (ftype);
>>>> + unsigned int fieldsize_bit;
>>>> +
>>>> + if (*endpos > 0 && *endpos < bitpos)
>>>
>>> Why do you check for *endpos > 0? Did you see a case where *endpos
>>> is 0
>>> and bitpos > 0? That would mean that there's a "hole" before the
>>> first field.
>>> Would we want to show it as a hole anyway?
>>
>> Yeah, this situation happens when we have a virtual method in a class.
>> Because of the vtable, the first field of the struct will start at
>> offset 8 (for 64-bit architectures), and in this case *endpos will be 0
>> because we won't have updated it, leading to a confusing message
>> about a
>> 8-byte hole in the beginning of the struct:
>>
>> ...
>> 50 /* offset | size */
>> 51 type = struct abc {
>> 52 public:
>> 53 /* XXX 8-byte hole */
>> 54 /* 8 | 8 */ void *field1;
>> ...
>>
>> In order to suppress this first message, I check for *endpos > 0.
>>
>> I will add a comment to the code explaining this scenario.
>
> Ah ok that makes sense. Yeah, a comment would be nice. But now I'm
> thinking that it would be nice if GDB showed the vtable. If I say the
> first field at offset 8, it would probably take me some time to get
> why, and would maybe think it's a bug. But if we showed a fake field,
> such as:
>
> /* 0 | 8 */ /* vtable */
>
> It would be immediately obvious.
OK, I did that.
BTW, a little status update.
Apparently the patch can't handle bitfields very well. I've found a few
cases where the bitfield handling gets confused, printing wrong
offsets/sizes/holes. Bitfields can be extremely complex to deal with
when it comes to offsets...
I spent hours trying to improve the patch, managed to make some
progress, but there are still corner cases to fix. For example, the
patch doesn't deal well with this case:
struct aa {
/* 0 | 1 */ char aa;
/* 1: 1 | 1 */ unsigned char a : 7;
/* 1:15 | 4 */ int b : 10;
} /* total size: 4 bytes */
In this case, the bitfield "b" would be combined with the previous
bitfield "a", like pahole reports:
struct aa {
char aa; /* 0 1 */
unsigned char a:7; /* 1: 1 1 */
/* Bitfield combined with previous fields */
int b:10; /* 0: 7 4 */
}
Confusing... I'm not sure why pahole reports b's offset to be 0.
Also, the patch doesn't understand cases like this:
struct tyu
{
unsigned int a1 : 1;
unsigned int a2 : 3;
unsigned int a9 : 23;
/* PROBLEM HAPPENS HERE */
unsigned char a0 : 2;
uint64_t a3;
unsigned int a4 : 5;
uint64_t a5 : 3;
};
In this case, we're switching types in the middle of the bitfield. The
bitfield "unsigned char a0" would be combined with the bitfields above
it, but the patch doesn't know that:
struct tyu {
/* 0:31 | 4 */ unsigned int a1 : 1;
/* 0:28 | 4 */ unsigned int a2 : 3;
/* 0: 5 | 4 */ unsigned int a9 : 23;
/* 3: 6 | 1 */ unsigned char a0 : 2;
/* XXX 3-bit hole */
/* XXX 4-byte hole */
/* 8 | 8 */ uint64_t a3;
/* 16:27 | 4 */ unsigned int a4 : 5;
/* 16:56 | 8 */ uint64_t a5 : 3;
} /* total size: 24 bytes */
Whereas pahole reports:
struct tyu {
unsigned int a1:1; /* 0:31 4 */
unsigned int a2:3; /* 0:28 4 */
unsigned int a9:23; /* 0: 5 4 */
/* XXX 253 bits hole, try to pack */
/* Bitfield combined with next fields */
unsigned char a0:2; /* 3: 3 1 */
/* XXX 6 bits hole, try to pack */
/* XXX 4 bytes hole, try to pack */
uint64_t a3; /* 8 8 */
unsigned int a4:5; /* 16:27 4 */
uint64_t a5:3; /* 16:56 8 */
};
Hm, TBH pahole itself seems a bit confused here, saying there is a
253-bit hole...
Anyway, long story short, this is much more complex that I thought it
would be (TM). I am extremely tired right now and can't continue, but I
intend to resume work tomorrow morning. But I'd like to leave a few
options on the table:
1) Remove the patch from the 8.1 wishlist, which will unblock the
branching.
2) Remove the bitfield handling from the patch, leaving it
feature-incomplete, but working.
3) Push the patch with these known limitations, document them, mark them
as KFAIL in the testcase, and open a bug to fix them (I personally
wouldn't like this).
4) Wait more time until these issues are resolved.
WDYT?
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/