* [PATCH] btf: emit non-representable bitfield as void
@ 2024-04-11 18:53 David Faust
2024-04-11 20:40 ` Indu Bhagat
0 siblings, 1 reply; 3+ messages in thread
From: David Faust @ 2024-04-11 18:53 UTC (permalink / raw)
To: gcc-patches; +Cc: indu.bhagat
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.
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))
- 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
{
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btf: emit non-representable bitfield as void
2024-04-11 18:53 [PATCH] btf: emit non-representable bitfield as void David Faust
@ 2024-04-11 20:40 ` Indu Bhagat
2024-04-11 20:52 ` David Faust
0 siblings, 1 reply; 3+ messages in thread
From: Indu Bhagat @ 2024-04-11 20:40 UTC (permalink / raw)
To: David Faust, gcc-patches
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
> {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btf: emit non-representable bitfield as void
2024-04-11 20:40 ` Indu Bhagat
@ 2024-04-11 20:52 ` David Faust
0 siblings, 0 replies; 3+ messages in thread
From: David Faust @ 2024-04-11 20:52 UTC (permalink / raw)
To: Indu Bhagat; +Cc: gcc-patches
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
>> {
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-11 20:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-11 18:53 [PATCH] btf: emit non-representable bitfield as void David Faust
2024-04-11 20:40 ` Indu Bhagat
2024-04-11 20:52 ` David Faust
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).