Message ID | 20240411185328.15855-1-david.faust@oracle.com |
---|---|
State | New |
Headers | show |
Series | btf: emit non-representable bitfield as void | expand |
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. > - 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 > {
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 >> { >
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)) - 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 {