On 4/11/24 13:40, Indu Bhagat wrote:
> On 4/11/24 11:53, David Faust wrote:
>> This patch fixes an issue with mangled BTF that could occur when
>> a struct type contains a bitfield member which cannot be represented
>> in BTF. It is undefined what should happen in such cases, but we can
>> at least do something reasonable.
>>
>> Commit
>>
>> 936dd627cd9 "btf: do not skip members of data type with type id
>> BTF_VOID_TYPEID"
>>
>> made a similar change for un-representable non-bitfield members, but
>> had an unintended side-effect of mangling BTF for un-representable
>> bitfields: the struct (or union) would account for the offending
>> bitfield in its member count but the bitfield member itself was
>> not emitted, making the member count incorrect.
>>
>> This change ensures that non-representable bitfield members of struct
>> and union types are always emitted with BTF_VOID_TYPEID. This avoids
>> corrupting the BTF information for the entire struct or union type.
>>
>> Tested on x86_64-linux-gnu and x86_64-linux-gnu host for
>> bpf-unknown-none target.
>>
>
> Hi David,
>
> Thanks for fixing this. One comment below.
>
>> gcc/
>> * btfout.cc (btf_asm_sou_member): Always emit non-representable
>> bitfield members as having 'void' type. Refactor slightly.
>>
>> gcc/testsuite/
>> * gcc.dg/debug/btf/btf-bitfields-4.c: Add two new checks.
>> ---
>> gcc/btfout.cc | 48 +++++++++----------
>> .../gcc.dg/debug/btf/btf-bitfields-4.c | 2 +
>> 2 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index ab491f0297f..e1ada41b1f4 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -922,41 +922,39 @@ static void
>> btf_asm_sou_member (ctf_container_ref ctfc, ctf_dmdef_t * dmd, unsigned
>> int idx)
>> {
>> ctf_dtdef_ref ref_type = ctfc->ctfc_types_list[dmd->dmd_type];
>> + ctf_id_t base_type = get_btf_id (dmd->dmd_type);
>> + uint64_t sou_offset = dmd->dmd_offset;
>> +
>> + dw2_asm_output_data (4, dmd->dmd_name_offset,
>> + "MEMBER '%s' idx=%u",
>> + dmd->dmd_name, idx);
>>
>> /* Re-encode bitfields to BTF representation. */
>> if (CTF_V2_INFO_KIND (ref_type->dtd_data.ctti_info) == CTF_K_SLICE)
>> {
>> - ctf_id_t base_type = ref_type->dtd_u.dtu_slice.cts_type;
>> unsigned short word_offset = ref_type->dtd_u.dtu_slice.cts_offset;
>> unsigned short bits = ref_type->dtd_u.dtu_slice.cts_bits;
>> - uint64_t sou_offset = dmd->dmd_offset;
>> -
>> - /* Pack the bit offset and bitfield size together. */
>> - sou_offset += word_offset;
>>
>> - /* If this bitfield cannot be represented, do not output anything.
>> - The parent struct/union 'vlen' field has already been updated. */
>> if ((bits > 0xff) || (sou_offset > 0xffffff))
>
> Why dont we reuse the btf_dmd_representable_bitfield_p () function here?
>
> This one here checks for sou_off > 0xffffff, while that in
> btf_dmd_representable_bitfield_p checks for sou_offset + word_offset >
> 0xffffff. The latter is more precise.
Oops. Good catch. We should be doing the same check here, but I moved
the sou_offset += word_offset into the else below, so the word_offset
isn't included.
I avoided using btf_dmd_representable_bitfield_p only because it does
some redundant work. Clearly that was a bad idea :D
Will use btf_dmd_representable_bitfield_p in v2 so we can keep these
checks contained.
>
>
>> - return;
>> -
>> - sou_offset &= 0x00ffffff;
>> - sou_offset |= ((bits & 0xff) << 24);
>> + {
>> + /* Bitfield cannot be represented in BTF. Emit the member as having
>> + 'void' type. */
>> + base_type = BTF_VOID_TYPEID;
>> + }
>> + else
>> + {
>> + /* Pack the bit offset and bitfield size together. */
>> + sou_offset += word_offset;
>> + sou_offset &= 0x00ffffff;
>> + sou_offset |= ((bits & 0xff) << 24);
>>
>> - dw2_asm_output_data (4, dmd->dmd_name_offset,
>> - "MEMBER '%s' idx=%u",
>> - dmd->dmd_name, idx);
>> - /* Refer to the base type of the slice. */
>> - btf_asm_type_ref ("btm_type", ctfc, get_btf_id (base_type));
>> - dw2_asm_output_data (4, sou_offset, "btm_offset");
>> - }
>> - else
>> - {
>> - dw2_asm_output_data (4, dmd->dmd_name_offset,
>> - "MEMBER '%s' idx=%u",
>> - dmd->dmd_name, idx);
>> - btf_asm_type_ref ("btm_type", ctfc, get_btf_id (dmd->dmd_type));
>> - dw2_asm_output_data (4, dmd->dmd_offset, "btm_offset");
>> + /* Refer to the base type of the slice. */
>> + base_type = get_btf_id (ref_type->dtd_u.dtu_slice.cts_type);
>> + }
>> }
>> +
>> + btf_asm_type_ref ("btm_type", ctfc, base_type);
>> + dw2_asm_output_data (4, sou_offset, "btm_offset");
>> }
>>
>> /* Asm'out an enum constant following a BTF_KIND_ENUM{,64}. */
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-bitfields-4.c
>> b/gcc/testsuite/gcc.dg/debug/btf/btf-bitfields-4.c
>> index d4a6ef6a1eb..20cdfaa057a 100644
>> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-bitfields-4.c
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-bitfields-4.c
>> @@ -14,6 +14,8 @@
>>
>> /* Struct with 4 members and no bitfield (kind_flag not set). */
>> /* { dg-final { scan-assembler-times "\[\t \]0x4000004\[\t
>> \]+\[^\n\]*btt_info" 1 } } */
>> +/* { dg-final { scan-assembler-times " MEMBER" 4 } } */
>> +/* { dg-final { scan-assembler-times " MEMBER 'unsup'
>> idx=2\[\\r\\n\]+\[^\\r\\n\]*0\[\t \]+\[^\n\]*btm_type: void" 1 } } */
>>
>> struct bigly
>> {
>
