Message ID | 20220928211501.2647123-1-guillermo.e.martinez@oracle.com |
---|---|
State | New |
Headers | show |
Series | [v2] btf: Add support to BTF_KIND_ENUM64 type | expand |
On 9/28/22 2:15 PM, Guillermo E. Martinez via Gcc-patches wrote: > Hello GCC team, > > The following is patch v2 to update BTF/CTF backend supporting > BTF_KIND_ENUM64 type. Changes from v1: > > + Fix typo in commit message. > + Fix changelog entries. > > Comments will be welcomed and appreciated!, > > Kind regards, > guillermo > -- > Hi Guillermo, Thanks for your patch. Sorry for the delay in reviewing this patch. Please see my comments inlined. Indu > BTF supports 64-bits enumerators with following encoding: > > struct btf_type: > name_off: 0 or offset to a valid C identifier > info.kind_flag: 0 for unsigned, 1 for signed > info.kind: BTF_KIND_ENUM64 > info.vlen: number of enum values > size: 1/2/4/8 > > The btf_type is followed by info.vlen number of: > > struct btf_enum64 > { > uint32_t name_off; /* Offset in string section of enumerator name. */ > uint32_t val_lo32; /* lower 32-bit value for a 64-bit value Enumerator */ > uint32_t val_hi32; /* high 32-bit value for a 64-bit value Enumerator */ > }; > > So, a new btf_enum64 structure was added to represent BTF_KIND_ENUM64 > and a new field in ctf_dtdef to represent specific type's properties, in > the particular case for CTF enums it helps to distinguish when its > enumerators values are signed or unsigned, later that information is > used to encode the BTF enum type. > > gcc/ChangeLog: > > * btfout.cc (btf_calc_num_vbytes): Compute enumeration size depending of > enumerator type btf_enum{,64}. > (btf_asm_type): Update btf_kflag according to enumerators sign, > using correct BPF type in BTF_KIND_ENUMi{,64}. Typo : i after ENUM > (btf_asm_enum_const): New argument to represent the size of > the BTF enum type. > * ctfc.cc (ctf_add_enum): Use and initialization of flag field to > CTF_ENUM_F_NONE. > (ctf_add_enumerator): New argument to represent CTF flags, > updating the comment and flag vaue according to enumerators > sing. > * ctfc.h (ctf_dmdef): Update dmd_value to HOST_WIDE_INT to allow > use 32/64 bits enumerators. > (ctf_dtdef): Add flags to to describe specific type's properties. > * dwarf2ctf.cc (gen_ctf_enumeration_type): Update flags field > depending when a signed enumerator value is found. > > include/ > * btf.h (btf_enum64): Add new definition and new symbolic > constant to BTF_KIND_ENUM64 and BTF_KF_ENUM_{UN,}SIGNED. > > gcc/testsuite/ChangeLog: > > * gcc.dg/debug/btf/btf-enum-1.c: Update testcase, with correct > info.kflags encoding. > * gcc.dg/debug/btf/btf-enum64-1.c: New testcase. > --- > gcc/btfout.cc | 24 ++++++++--- > gcc/ctfc.cc | 14 ++++--- > gcc/ctfc.h | 9 +++- > gcc/dwarf2ctf.cc | 9 +++- > gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c | 2 +- > gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c | 41 +++++++++++++++++++ > include/btf.h | 19 +++++++-- > 7 files changed, 99 insertions(+), 19 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c > > diff --git a/gcc/btfout.cc b/gcc/btfout.cc > index 997a33fa089..4b11c867c23 100644 > --- a/gcc/btfout.cc > +++ b/gcc/btfout.cc > @@ -223,7 +223,9 @@ btf_calc_num_vbytes (ctf_dtdef_ref dtd) > break; > > case BTF_KIND_ENUM: > - vlen_bytes += vlen * sizeof (struct btf_enum); > + vlen_bytes += (dtd->dtd_data.ctti_size == 0x8) > + ? vlen * sizeof (struct btf_enum64) > + : vlen * sizeof (struct btf_enum); > break; > > case BTF_KIND_FUNC_PROTO: > @@ -622,6 +624,15 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) > btf_size_type = 0; > } > > + if (btf_kind == BTF_KIND_ENUM) > + { > + btf_kflag = (dtd->flags & CTF_ENUM_F_ENUMERATORS_SIGNED) > + ? BTF_KF_ENUM_SIGNED > + : BTF_KF_ENUM_UNSIGNED; > + if (dtd->dtd_data.ctti_size == 0x8) > + btf_kind = BTF_KIND_ENUM64; > + } > + See below. If you do add a new member in ctf_dmdef instead (as I propose), you should ideally iterate over the enumerators (dtd->dtd_u.dtu_members) to make sure they are all the same signedness. > dw2_asm_output_data (4, dtd->dtd_data.ctti_name, "btt_name"); > dw2_asm_output_data (4, BTF_TYPE_INFO (btf_kind, btf_kflag, btf_vlen), > "btt_info: kind=%u, kflag=%u, vlen=%u", > @@ -634,6 +645,7 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) > case BTF_KIND_UNION: > case BTF_KIND_ENUM: > case BTF_KIND_DATASEC: > + case BTF_KIND_ENUM64: > dw2_asm_output_data (4, dtd->dtd_data.ctti_size, "btt_size: %uB", > dtd->dtd_data.ctti_size); > return; > @@ -707,13 +719,13 @@ btf_asm_sou_member (ctf_container_ref ctfc, ctf_dmdef_t * dmd) > } > } > > -/* Asm'out an enum constant following a BTF_KIND_ENUM. */ > +/* Asm'out an enum constant following a BTF_KIND_ENUM{,64}. */ > > static void > -btf_asm_enum_const (ctf_dmdef_t * dmd) > +btf_asm_enum_const (unsigned int size, ctf_dmdef_t * dmd) > { > dw2_asm_output_data (4, dmd->dmd_name_offset, "bte_name"); > - dw2_asm_output_data (4, dmd->dmd_value, "bte_value"); > + dw2_asm_output_data (size, dmd->dmd_value, "bte_value"); I am not sure if this is ok to do like the above. The format specification says: The ``btf_enum64`` encoding: * ``name_off``: offset to a valid C identifier * ``val_lo32``: lower 32-bit value for a 64-bit value * ``val_hi32``: high 32-bit value for a 64-bit value What if the chosen BPF arch type is big-endian and the const value is 8 bytes ? Wouldnt such constant values to be written out incorrectly then ? I think you need to write the lower 32-bits and higher 32-bits explicitly to avoid that issue. > } > > /* Asm'out a function parameter description following a BTF_KIND_FUNC_PROTO. */ > @@ -871,7 +883,7 @@ output_asm_btf_sou_fields (ctf_container_ref ctfc, ctf_dtdef_ref dtd) > btf_asm_sou_member (ctfc, dmd); > } > > -/* Output all enumerator constants following a BTF_KIND_ENUM. */ > +/* Output all enumerator constants following a BTF_KIND_ENUM{,64}. */ > > static void > output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), > @@ -881,7 +893,7 @@ output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), > > for (dmd = dtd->dtd_u.dtu_members; > dmd != NULL; dmd = (ctf_dmdef_t *) ctf_dmd_list_next (dmd)) > - btf_asm_enum_const (dmd); > + btf_asm_enum_const (dtd->dtd_data.ctti_size, dmd); > } > > /* Output all function arguments following a BTF_KIND_FUNC_PROTO. */ > diff --git a/gcc/ctfc.cc b/gcc/ctfc.cc > index 9773358a475..253c36b6a0a 100644 > --- a/gcc/ctfc.cc > +++ b/gcc/ctfc.cc > @@ -604,6 +604,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, > gcc_assert (size <= CTF_MAX_SIZE); > > dtd->dtd_data.ctti_size = size; > + dtd->flags = CTF_ENUM_F_NONE; > > ctfc->ctfc_num_stypes++; > > @@ -612,7 +613,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, > > int > ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, > - HOST_WIDE_INT value, dw_die_ref die) > + HOST_WIDE_INT value, uint32_t flags, dw_die_ref die) > { > ctf_dmdef_t * dmd; > uint32_t kind, vlen, root; > @@ -630,10 +631,12 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, > > gcc_assert (kind == CTF_K_ENUM && vlen < CTF_MAX_VLEN); > > - /* Enum value is of type HOST_WIDE_INT in the compiler, dmd_value is int32_t > - on the other hand. Check bounds and skip adding this enum value if out of > - bounds. */ > - if ((value > INT_MAX) || (value < INT_MIN)) > + /* Enum value is of type HOST_WIDE_INT in the compiler, CTF enumerators > + values in ctf_enum_t is limited to int32_t, BTF supports signed and > + unsigned enumerators values of 32 and 64 bits, for both debug formats > + we use ctf_dmdef_t.dmd_value entry of HOST_WIDE_INT type. So check > + CTF bounds and skip adding this enum value if out of bounds. */ > + if (!btf_debuginfo_p() && ((value > INT_MAX) || (value < INT_MIN))) > { > /* FIXME - Note this TBD_CTF_REPRESENTATION_LIMIT. */ > return (1); > @@ -649,6 +652,7 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, > dmd->dmd_value = value; > > dtd->dtd_data.ctti_info = CTF_TYPE_INFO (kind, root, vlen + 1); > + dtd->flags |= flags; > ctf_dmd_list_append (&dtd->dtd_u.dtu_members, dmd); > > if ((name != NULL) && strcmp (name, "")) > diff --git a/gcc/ctfc.h b/gcc/ctfc.h > index bcf3a43ae1b..a22342b2610 100644 > --- a/gcc/ctfc.h > +++ b/gcc/ctfc.h > @@ -125,6 +125,10 @@ typedef struct GTY (()) ctf_itype > > #define CTF_FUNC_VARARG 0x1 > > +/* Enum specific flags. */ > +#define CTF_ENUM_F_NONE (0) > +#define CTF_ENUM_F_ENUMERATORS_SIGNED (1 << 0) > + > /* Struct/union/enum member definition for CTF generation. */ > > typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef > @@ -133,7 +137,7 @@ typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef > ctf_id_t dmd_type; /* Type of this member (for sou). */ > uint32_t dmd_name_offset; /* Offset of the name in str table. */ > uint64_t dmd_offset; /* Offset of this member in bits (for sou). */ > - int dmd_value; /* Value of this member (for enum). */ > + HOST_WIDE_INT dmd_value; /* Value of this member (for enum). */ > struct ctf_dmdef * dmd_next; /* A list node. */ > } ctf_dmdef_t; > I am wondering if you considered adding a member here instead - something like- bool dmd_value_signed; /* Signedness for the enumerator. */. See comment below. > @@ -162,6 +166,7 @@ struct GTY ((for_user)) ctf_dtdef > bool from_global_func; /* Whether this type was added from a global > function. */ > uint32_t linkage; /* Used in function types. 0=local, 1=global. */ > + uint32_t flags; /* Flags to describe specific type's properties. */ > union GTY ((desc ("ctf_dtu_d_union_selector (&%1)"))) > { > /* struct, union, or enum. */ Instead of carrying this information in ctf_dtdef which is the data structure for each type in CTF, how about adding a new member in struct ctf_dmdef? The "flags" member is meant for only enum types, and hence it will be more appropriate to add to ctf_dmdef as say, dmd_value_signed. > @@ -429,7 +434,7 @@ extern ctf_id_t ctf_add_sou (ctf_container_ref, uint32_t, const char *, > uint32_t, size_t, dw_die_ref); > > extern int ctf_add_enumerator (ctf_container_ref, ctf_id_t, const char *, > - HOST_WIDE_INT, dw_die_ref); > + HOST_WIDE_INT, uint32_t, dw_die_ref); > extern int ctf_add_member_offset (ctf_container_ref, dw_die_ref, const char *, > ctf_id_t, uint64_t); > extern int ctf_add_function_arg (ctf_container_ref, dw_die_ref, > diff --git a/gcc/dwarf2ctf.cc b/gcc/dwarf2ctf.cc > index 397100004c2..0ef96dd48fd 100644 > --- a/gcc/dwarf2ctf.cc > +++ b/gcc/dwarf2ctf.cc > @@ -772,6 +772,7 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) > const char *enumerator_name; > dw_attr_node *enumerator_value; > HOST_WIDE_INT value_wide_int; > + uint32_t flags = 0; > > c = dw_get_die_sib (c); > > @@ -785,10 +786,14 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) > == dw_val_class_unsigned_const_implicit)) > value_wide_int = AT_unsigned (enumerator_value); > else > - value_wide_int = AT_int (enumerator_value); > + { > + value_wide_int = AT_int (enumerator_value); > + flags |= CTF_ENUM_F_ENUMERATORS_SIGNED; > + } > > ctf_add_enumerator (ctfc, enumeration_type_id, > - enumerator_name, value_wide_int, enumeration); > + enumerator_name, value_wide_int, > + flags, enumeration); > } > while (c != dw_get_die_child (enumeration)); > } > diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c > index 728493b0804..7e940529f1b 100644 > --- a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c > +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c > @@ -4,7 +4,7 @@ > /* { dg-options "-O0 -gbtf -dA" } */ > > /* { dg-final { scan-assembler-times "\[\t \]0x6000004\[\t \]+\[^\n\]*btt_info" 1 } } */ > -/* { dg-final { scan-assembler-times "\[\t \]0x6000003\[\t \]+\[^\n\]*btt_info" 1 } } */ > +/* { dg-final { scan-assembler-times "\[\t \]0x86000003\[\t \]+\[^\n\]*btt_info" 1 } } */ > /* { dg-final { scan-assembler-times "ascii \"QAD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > /* { dg-final { scan-assembler-times "ascii \"QED.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > /* { dg-final { scan-assembler-times "ascii \"QOD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c > new file mode 100644 > index 00000000000..da103842807 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c > @@ -0,0 +1,41 @@ > +/* Test BTF generation for 64 bits enums. */ > + > +/* { dg-do compile } */ > +/* { dg-options "-O0 -gbtf -dA" } */ > + > +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum1,\[\t \]8" 1 } } */ > +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum2,\[\t \]8" 1 } } */ > +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum3,\[\t \]8" 1 } } */ > +/* { dg-final { scan-assembler-times "\[\t \]0x13000003\[\t \]+\[^\n\]*btt_info" 2 } } */ > +/* { dg-final { scan-assembler-times "\[\t \]0x93000003\[\t \]+\[^\n\]*btt_info" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"B1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"B2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"B3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"C1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"C2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"C3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"D1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"D2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "ascii \"D3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ > +/* { dg-final { scan-assembler-times "bte_value" 9 } } */ > + > +enum default_enum > +{ > + B1 = 0xffffffffaa, > + B2 = 0xbbbbbbbb, > + B3 = 0xaabbccdd, > +} myenum1 = B1; > + > +enum explicit_unsigned > +{ > + C1 = 0xffffffffbbUL, > + C2 = 0xbbbbbbbb, > + C3 = 0xaabbccdd, > +} myenum2 = C1; > + > +enum signed64 > +{ > + D1 = 0xffffffffaa, > + D2 = 0xbbbbbbbb, > + D3 = -0x1, > +} myenum3 = D1; I wonder if its meaningul to add such an enum64 test with -mlittle-endian and -mbig-endian to keep the order of val_lo32 and val_hi32 tested. > diff --git a/include/btf.h b/include/btf.h > index 78b551ced23..eba67f9d599 100644 > --- a/include/btf.h > +++ b/include/btf.h > @@ -109,7 +109,8 @@ struct btf_type > #define BTF_KIND_VAR 14 /* Variable. */ > #define BTF_KIND_DATASEC 15 /* Section such as .bss or .data. */ > #define BTF_KIND_FLOAT 16 /* Floating point. */ > -#define BTF_KIND_MAX BTF_KIND_FLOAT > +#define BTF_KIND_ENUM64 19 /* Enumeration up to 64 bits. */ > +#define BTF_KIND_MAX BTF_KIND_ENUM64 > #define NR_BTF_KINDS (BTF_KIND_MAX + 1) > > /* For some BTF_KINDs, struct btf_type is immediately followed by > @@ -130,14 +131,17 @@ struct btf_type > #define BTF_INT_BOOL (1 << 2) > > /* BTF_KIND_ENUM is followed by VLEN struct btf_enum entries, > - which describe the enumerators. Note that BTF currently only > - supports signed 32-bit enumerator values. */ > + which describe the enumerators. */ > struct btf_enum > { > uint32_t name_off; /* Offset in string section of enumerator name. */ > int32_t val; /* Enumerator value. */ > }; > > +/* BTF_KF_ENUM_ holds the flags for kflags in BTF_KIND_ENUM{,64}. */ > +#define BTF_KF_ENUM_UNSIGNED (0) > +#define BTF_KF_ENUM_SIGNED (1 << 0) > + > /* BTF_KIND_ARRAY is followed by a single struct btf_array. */ > struct btf_array > { > @@ -190,6 +194,15 @@ struct btf_var_secinfo > uint32_t size; /* Size (in bytes) of variable. */ > }; > > +/* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries, > + which describe the 64 bits enumerators. */ > +struct btf_enum64 > +{ > + uint32_t name_off; /* Offset in string section of enumerator name. */ > + uint32_t val_lo32; /* lower 32-bit value for a 64-bit value Enumerator */ > + uint32_t val_hi32; /* high 32-bit value for a 64-bit value Enumerator */ > +}; > + > #ifdef __cplusplus > } > #endif >
On 9/29/22 17:35, Indu Bhagat wrote: > On 9/28/22 2:15 PM, Guillermo E. Martinez via Gcc-patches wrote: >> Hello GCC team, >> >> The following is patch v2 to update BTF/CTF backend supporting >> BTF_KIND_ENUM64 type. Changes from v1: >> >> + Fix typo in commit message. >> + Fix changelog entries. >> >> Comments will be welcomed and appreciated!, >> >> Kind regards, >> guillermo >> -- >> > > Hi Guillermo, > Hi Indu, > Thanks for your patch. > > Sorry for the delay in reviewing this patch. Please see my comments inlined. > No worries, thanks so much for your comments!. They were really useful. > Indu > >> BTF supports 64-bits enumerators with following encoding: >> >> struct btf_type: >> name_off: 0 or offset to a valid C identifier >> info.kind_flag: 0 for unsigned, 1 for signed >> info.kind: BTF_KIND_ENUM64 >> info.vlen: number of enum values >> size: 1/2/4/8 >> >> The btf_type is followed by info.vlen number of: >> >> struct btf_enum64 >> { >> uint32_t name_off; /* Offset in string section of enumerator name. */ >> uint32_t val_lo32; /* lower 32-bit value for a 64-bit value Enumerator */ >> uint32_t val_hi32; /* high 32-bit value for a 64-bit value Enumerator */ >> }; >> >> So, a new btf_enum64 structure was added to represent BTF_KIND_ENUM64 >> and a new field in ctf_dtdef to represent specific type's properties, in >> the particular case for CTF enums it helps to distinguish when its >> enumerators values are signed or unsigned, later that information is >> used to encode the BTF enum type. >> >> gcc/ChangeLog: >> >> * btfout.cc (btf_calc_num_vbytes): Compute enumeration size depending of >> enumerator type btf_enum{,64}. >> (btf_asm_type): Update btf_kflag according to enumerators sign, >> using correct BPF type in BTF_KIND_ENUMi{,64}. > > Typo : i after ENUM > Fixed in v3. >> (btf_asm_enum_const): New argument to represent the size of >> the BTF enum type. >> * ctfc.cc (ctf_add_enum): Use and initialization of flag field to >> CTF_ENUM_F_NONE. >> (ctf_add_enumerator): New argument to represent CTF flags, >> updating the comment and flag vaue according to enumerators >> sing. >> * ctfc.h (ctf_dmdef): Update dmd_value to HOST_WIDE_INT to allow >> use 32/64 bits enumerators. >> (ctf_dtdef): Add flags to to describe specific type's properties. >> * dwarf2ctf.cc (gen_ctf_enumeration_type): Update flags field >> depending when a signed enumerator value is found. >> >> include/ >> * btf.h (btf_enum64): Add new definition and new symbolic >> constant to BTF_KIND_ENUM64 and BTF_KF_ENUM_{UN,}SIGNED. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/debug/btf/btf-enum-1.c: Update testcase, with correct >> info.kflags encoding. >> * gcc.dg/debug/btf/btf-enum64-1.c: New testcase. >> --- >> gcc/btfout.cc | 24 ++++++++--- >> gcc/ctfc.cc | 14 ++++--- >> gcc/ctfc.h | 9 +++- >> gcc/dwarf2ctf.cc | 9 +++- >> gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c | 2 +- >> gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c | 41 +++++++++++++++++++ >> include/btf.h | 19 +++++++-- >> 7 files changed, 99 insertions(+), 19 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c >> >> diff --git a/gcc/btfout.cc b/gcc/btfout.cc >> index 997a33fa089..4b11c867c23 100644 >> --- a/gcc/btfout.cc >> +++ b/gcc/btfout.cc >> @@ -223,7 +223,9 @@ btf_calc_num_vbytes (ctf_dtdef_ref dtd) >> break; >> case BTF_KIND_ENUM: >> - vlen_bytes += vlen * sizeof (struct btf_enum); >> + vlen_bytes += (dtd->dtd_data.ctti_size == 0x8) >> + ? vlen * sizeof (struct btf_enum64) >> + : vlen * sizeof (struct btf_enum); >> break; >> case BTF_KIND_FUNC_PROTO: >> @@ -622,6 +624,15 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) >> btf_size_type = 0; >> } >> + if (btf_kind == BTF_KIND_ENUM) >> + { >> + btf_kflag = (dtd->flags & CTF_ENUM_F_ENUMERATORS_SIGNED) >> + ? BTF_KF_ENUM_SIGNED >> + : BTF_KF_ENUM_UNSIGNED; >> + if (dtd->dtd_data.ctti_size == 0x8) >> + btf_kind = BTF_KIND_ENUM64; >> + } >> + > > See below. If you do add a new member in ctf_dmdef instead (as I propose), you should ideally iterate over the enumerators (dtd->dtd_u.dtu_members) to make sure they are all the same signedness. > Correct, struct `ctf_dmdef' stores enumerator's information. However in BTF spec ``info.kind_flag`` is not a property per enumerator, but enumeration type property instead, so I decided to add in `ctf_dtdef' a `flag' field to store the signeedness information for an enumeration type, then in `gen_ctf_enumeration_type' when it builds the enumerators entries,`ctf_dtdef.flag' is updated to `CTF_ENUM_F_ENUMERATORS_SIGNED' when it found a signed enumerator value meaning that enumeration type should be consider a signed type. i.e, I detect signeedness when enumeration type is being building. Of course, I can add `dmd_value_signed' field in `ctf_dmdef' but I need to iterate over all `ctf_dmdef.dmd_value_signed' (in `btf_asm_type' I guess) to properly set ``info.kind_flag`` if it's a signed enumeration type. >> dw2_asm_output_data (4, dtd->dtd_data.ctti_name, "btt_name"); >> dw2_asm_output_data (4, BTF_TYPE_INFO (btf_kind, btf_kflag, btf_vlen), >> "btt_info: kind=%u, kflag=%u, vlen=%u", >> @@ -634,6 +645,7 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) >> case BTF_KIND_UNION: >> case BTF_KIND_ENUM: >> case BTF_KIND_DATASEC: >> + case BTF_KIND_ENUM64: >> dw2_asm_output_data (4, dtd->dtd_data.ctti_size, "btt_size: %uB", >> dtd->dtd_data.ctti_size); >> return; >> @@ -707,13 +719,13 @@ btf_asm_sou_member (ctf_container_ref ctfc, ctf_dmdef_t * dmd) >> } >> } >> -/* Asm'out an enum constant following a BTF_KIND_ENUM. */ >> +/* Asm'out an enum constant following a BTF_KIND_ENUM{,64}. */ >> static void >> -btf_asm_enum_const (ctf_dmdef_t * dmd) >> +btf_asm_enum_const (unsigned int size, ctf_dmdef_t * dmd) >> { >> dw2_asm_output_data (4, dmd->dmd_name_offset, "bte_name"); >> - dw2_asm_output_data (4, dmd->dmd_value, "bte_value"); >> + dw2_asm_output_data (size, dmd->dmd_value, "bte_value"); > > I am not sure if this is ok to do like the above. The format specification says: > No, It is not right. > The ``btf_enum64`` encoding: > * ``name_off``: offset to a valid C identifier > * ``val_lo32``: lower 32-bit value for a 64-bit value > * ``val_hi32``: high 32-bit value for a 64-bit value > > What if the chosen BPF arch type is big-endian and the const value is 8 bytes ? Wouldnt such constant values to be written out incorrectly then ? I think you need to write the lower 32-bits and higher 32-bits explicitly to avoid that issue. > Totally agree. Fixed in v3. >> } >> /* Asm'out a function parameter description following a BTF_KIND_FUNC_PROTO. */ >> @@ -871,7 +883,7 @@ output_asm_btf_sou_fields (ctf_container_ref ctfc, ctf_dtdef_ref dtd) >> btf_asm_sou_member (ctfc, dmd); >> } >> -/* Output all enumerator constants following a BTF_KIND_ENUM. */ >> +/* Output all enumerator constants following a BTF_KIND_ENUM{,64}. */ >> static void >> output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), >> @@ -881,7 +893,7 @@ output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), >> for (dmd = dtd->dtd_u.dtu_members; >> dmd != NULL; dmd = (ctf_dmdef_t *) ctf_dmd_list_next (dmd)) >> - btf_asm_enum_const (dmd); >> + btf_asm_enum_const (dtd->dtd_data.ctti_size, dmd); >> } >> /* Output all function arguments following a BTF_KIND_FUNC_PROTO. */ >> diff --git a/gcc/ctfc.cc b/gcc/ctfc.cc >> index 9773358a475..253c36b6a0a 100644 >> --- a/gcc/ctfc.cc >> +++ b/gcc/ctfc.cc >> @@ -604,6 +604,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, >> gcc_assert (size <= CTF_MAX_SIZE); >> dtd->dtd_data.ctti_size = size; >> + dtd->flags = CTF_ENUM_F_NONE; >> ctfc->ctfc_num_stypes++; >> @@ -612,7 +613,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, >> int >> ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >> - HOST_WIDE_INT value, dw_die_ref die) >> + HOST_WIDE_INT value, uint32_t flags, dw_die_ref die) >> { >> ctf_dmdef_t * dmd; >> uint32_t kind, vlen, root; >> @@ -630,10 +631,12 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >> gcc_assert (kind == CTF_K_ENUM && vlen < CTF_MAX_VLEN); >> - /* Enum value is of type HOST_WIDE_INT in the compiler, dmd_value is int32_t >> - on the other hand. Check bounds and skip adding this enum value if out of >> - bounds. */ >> - if ((value > INT_MAX) || (value < INT_MIN)) >> + /* Enum value is of type HOST_WIDE_INT in the compiler, CTF enumerators >> + values in ctf_enum_t is limited to int32_t, BTF supports signed and >> + unsigned enumerators values of 32 and 64 bits, for both debug formats >> + we use ctf_dmdef_t.dmd_value entry of HOST_WIDE_INT type. So check >> + CTF bounds and skip adding this enum value if out of bounds. */ >> + if (!btf_debuginfo_p() && ((value > INT_MAX) || (value < INT_MIN))) >> { >> /* FIXME - Note this TBD_CTF_REPRESENTATION_LIMIT. */ >> return (1); >> @@ -649,6 +652,7 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >> dmd->dmd_value = value; >> dtd->dtd_data.ctti_info = CTF_TYPE_INFO (kind, root, vlen + 1); >> + dtd->flags |= flags; >> ctf_dmd_list_append (&dtd->dtd_u.dtu_members, dmd); >> if ((name != NULL) && strcmp (name, "")) >> diff --git a/gcc/ctfc.h b/gcc/ctfc.h >> index bcf3a43ae1b..a22342b2610 100644 >> --- a/gcc/ctfc.h >> +++ b/gcc/ctfc.h >> @@ -125,6 +125,10 @@ typedef struct GTY (()) ctf_itype >> #define CTF_FUNC_VARARG 0x1 >> +/* Enum specific flags. */ >> +#define CTF_ENUM_F_NONE (0) >> +#define CTF_ENUM_F_ENUMERATORS_SIGNED (1 << 0) >> + >> /* Struct/union/enum member definition for CTF generation. */ >> typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef >> @@ -133,7 +137,7 @@ typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef >> ctf_id_t dmd_type; /* Type of this member (for sou). */ >> uint32_t dmd_name_offset; /* Offset of the name in str table. */ >> uint64_t dmd_offset; /* Offset of this member in bits (for sou). */ >> - int dmd_value; /* Value of this member (for enum). */ >> + HOST_WIDE_INT dmd_value; /* Value of this member (for enum). */ >> struct ctf_dmdef * dmd_next; /* A list node. */ >> } ctf_dmdef_t; > > I am wondering if you considered adding a member here instead - something like- > > bool dmd_value_signed; /* Signedness for the enumerator. */. > > See comment below. > >> @@ -162,6 +166,7 @@ struct GTY ((for_user)) ctf_dtdef >> bool from_global_func; /* Whether this type was added from a global >> function. */ >> uint32_t linkage; /* Used in function types. 0=local, 1=global. */ >> + uint32_t flags; /* Flags to describe specific type's properties. */ >> union GTY ((desc ("ctf_dtu_d_union_selector (&%1)"))) >> { >> /* struct, union, or enum. */ > > Instead of carrying this information in ctf_dtdef which is the data structure for each type in CTF, how about adding a new member in struct ctf_dmdef? The "flags" member is meant for only enum types, and hence it will be more appropriate to add to ctf_dmdef as say, dmd_value_signed. > Yes, `ctf_dtdef' is structure for each type in CTF (including enumeration), and `ctf_dmdef' keeps information for enumerator, not for the enumeration type. >> @@ -429,7 +434,7 @@ extern ctf_id_t ctf_add_sou (ctf_container_ref, uint32_t, const char *, >> uint32_t, size_t, dw_die_ref); >> extern int ctf_add_enumerator (ctf_container_ref, ctf_id_t, const char *, >> - HOST_WIDE_INT, dw_die_ref); >> + HOST_WIDE_INT, uint32_t, dw_die_ref); >> extern int ctf_add_member_offset (ctf_container_ref, dw_die_ref, const char *, >> ctf_id_t, uint64_t); >> extern int ctf_add_function_arg (ctf_container_ref, dw_die_ref, >> diff --git a/gcc/dwarf2ctf.cc b/gcc/dwarf2ctf.cc >> index 397100004c2..0ef96dd48fd 100644 >> --- a/gcc/dwarf2ctf.cc >> +++ b/gcc/dwarf2ctf.cc >> @@ -772,6 +772,7 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) >> const char *enumerator_name; >> dw_attr_node *enumerator_value; >> HOST_WIDE_INT value_wide_int; >> + uint32_t flags = 0; >> c = dw_get_die_sib (c); >> @@ -785,10 +786,14 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) >> == dw_val_class_unsigned_const_implicit)) >> value_wide_int = AT_unsigned (enumerator_value); >> else >> - value_wide_int = AT_int (enumerator_value); >> + { >> + value_wide_int = AT_int (enumerator_value); >> + flags |= CTF_ENUM_F_ENUMERATORS_SIGNED; >> + } >> ctf_add_enumerator (ctfc, enumeration_type_id, >> - enumerator_name, value_wide_int, enumeration); >> + enumerator_name, value_wide_int, >> + flags, enumeration); >> } >> while (c != dw_get_die_child (enumeration)); >> } >> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c >> index 728493b0804..7e940529f1b 100644 >> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c >> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c >> @@ -4,7 +4,7 @@ >> /* { dg-options "-O0 -gbtf -dA" } */ >> /* { dg-final { scan-assembler-times "\[\t \]0x6000004\[\t \]+\[^\n\]*btt_info" 1 } } */ >> -/* { dg-final { scan-assembler-times "\[\t \]0x6000003\[\t \]+\[^\n\]*btt_info" 1 } } */ >> +/* { dg-final { scan-assembler-times "\[\t \]0x86000003\[\t \]+\[^\n\]*btt_info" 1 } } */ >> /* { dg-final { scan-assembler-times "ascii \"QAD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> /* { dg-final { scan-assembler-times "ascii \"QED.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> /* { dg-final { scan-assembler-times "ascii \"QOD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c >> new file mode 100644 >> index 00000000000..da103842807 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c >> @@ -0,0 +1,41 @@ >> +/* Test BTF generation for 64 bits enums. */ >> + >> +/* { dg-do compile } */ >> +/* { dg-options "-O0 -gbtf -dA" } */ >> + >> +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum1,\[\t \]8" 1 } } */ >> +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum2,\[\t \]8" 1 } } */ >> +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum3,\[\t \]8" 1 } } */ >> +/* { dg-final { scan-assembler-times "\[\t \]0x13000003\[\t \]+\[^\n\]*btt_info" 2 } } */ >> +/* { dg-final { scan-assembler-times "\[\t \]0x93000003\[\t \]+\[^\n\]*btt_info" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"B1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"B2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"B3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"C1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"C2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"C3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"D1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"D2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "ascii \"D3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ >> +/* { dg-final { scan-assembler-times "bte_value" 9 } } */ >> + >> +enum default_enum >> +{ >> + B1 = 0xffffffffaa, >> + B2 = 0xbbbbbbbb, >> + B3 = 0xaabbccdd, >> +} myenum1 = B1; >> + >> +enum explicit_unsigned >> +{ >> + C1 = 0xffffffffbbUL, >> + C2 = 0xbbbbbbbb, >> + C3 = 0xaabbccdd, >> +} myenum2 = C1; >> + >> +enum signed64 >> +{ >> + D1 = 0xffffffffaa, >> + D2 = 0xbbbbbbbb, >> + D3 = -0x1, >> +} myenum3 = D1; > > I wonder if its meaningul to add such an enum64 test with -mlittle-endian and -mbig-endian to keep the order of val_lo32 and val_hi32 tested. > Very meaningful!. I'll add use cases in v3 for both endianess. >> diff --git a/include/btf.h b/include/btf.h >> index 78b551ced23..eba67f9d599 100644 >> --- a/include/btf.h >> +++ b/include/btf.h >> @@ -109,7 +109,8 @@ struct btf_type >> #define BTF_KIND_VAR 14 /* Variable. */ >> #define BTF_KIND_DATASEC 15 /* Section such as .bss or .data. */ >> #define BTF_KIND_FLOAT 16 /* Floating point. */ >> -#define BTF_KIND_MAX BTF_KIND_FLOAT >> +#define BTF_KIND_ENUM64 19 /* Enumeration up to 64 bits. */ >> +#define BTF_KIND_MAX BTF_KIND_ENUM64 >> #define NR_BTF_KINDS (BTF_KIND_MAX + 1) >> /* For some BTF_KINDs, struct btf_type is immediately followed by >> @@ -130,14 +131,17 @@ struct btf_type >> #define BTF_INT_BOOL (1 << 2) >> /* BTF_KIND_ENUM is followed by VLEN struct btf_enum entries, >> - which describe the enumerators. Note that BTF currently only >> - supports signed 32-bit enumerator values. */ >> + which describe the enumerators. */ >> struct btf_enum >> { >> uint32_t name_off; /* Offset in string section of enumerator name. */ >> int32_t val; /* Enumerator value. */ >> }; >> +/* BTF_KF_ENUM_ holds the flags for kflags in BTF_KIND_ENUM{,64}. */ >> +#define BTF_KF_ENUM_UNSIGNED (0) >> +#define BTF_KF_ENUM_SIGNED (1 << 0) >> + >> /* BTF_KIND_ARRAY is followed by a single struct btf_array. */ >> struct btf_array >> { >> @@ -190,6 +194,15 @@ struct btf_var_secinfo >> uint32_t size; /* Size (in bytes) of variable. */ >> }; >> +/* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries, >> + which describe the 64 bits enumerators. */ >> +struct btf_enum64 >> +{ >> + uint32_t name_off; /* Offset in string section of enumerator name. */ >> + uint32_t val_lo32; /* lower 32-bit value for a 64-bit value Enumerator */ >> + uint32_t val_hi32; /* high 32-bit value for a 64-bit value Enumerator */ >> +}; >> + >> #ifdef __cplusplus >> } >> #endif >> >
Hi Guillermo, On 10/3/22 7:39 AM, Guillermo E. Martinez via Gcc-patches wrote: >>> diff --git a/gcc/ctfc.cc b/gcc/ctfc.cc >>> index 9773358a475..253c36b6a0a 100644 >>> --- a/gcc/ctfc.cc >>> +++ b/gcc/ctfc.cc >>> @@ -604,6 +604,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t >>> flag, const char * name, >>> gcc_assert (size <= CTF_MAX_SIZE); >>> dtd->dtd_data.ctti_size = size; >>> + dtd->flags = CTF_ENUM_F_NONE; >>> ctfc->ctfc_num_stypes++; >>> @@ -612,7 +613,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t >>> flag, const char * name, >>> int >>> ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const >>> char * name, >>> - HOST_WIDE_INT value, dw_die_ref die) >>> + HOST_WIDE_INT value, uint32_t flags, dw_die_ref die) >>> { >>> ctf_dmdef_t * dmd; >>> uint32_t kind, vlen, root; >>> @@ -630,10 +631,12 @@ ctf_add_enumerator (ctf_container_ref ctfc, >>> ctf_id_t enid, const char * name, >>> gcc_assert (kind == CTF_K_ENUM && vlen < CTF_MAX_VLEN); >>> - /* Enum value is of type HOST_WIDE_INT in the compiler, dmd_value >>> is int32_t >>> - on the other hand. Check bounds and skip adding this enum >>> value if out of >>> - bounds. */ >>> - if ((value > INT_MAX) || (value < INT_MIN)) >>> + /* Enum value is of type HOST_WIDE_INT in the compiler, CTF >>> enumerators >>> + values in ctf_enum_t is limited to int32_t, BTF supports signed >>> and >>> + unsigned enumerators values of 32 and 64 bits, for both debug >>> formats >>> + we use ctf_dmdef_t.dmd_value entry of HOST_WIDE_INT type. So check >>> + CTF bounds and skip adding this enum value if out of bounds. */ >>> + if (!btf_debuginfo_p() && ((value > INT_MAX) || (value < INT_MIN))) >>> { >>> /* FIXME - Note this TBD_CTF_REPRESENTATION_LIMIT. */ >>> return (1); >>> @@ -649,6 +652,7 @@ ctf_add_enumerator (ctf_container_ref ctfc, >>> ctf_id_t enid, const char * name, >>> dmd->dmd_value = value; >>> dtd->dtd_data.ctti_info = CTF_TYPE_INFO (kind, root, vlen + 1); >>> + dtd->flags |= flags; >>> ctf_dmd_list_append (&dtd->dtd_u.dtu_members, dmd); >>> if ((name != NULL) && strcmp (name, "")) >>> diff --git a/gcc/ctfc.h b/gcc/ctfc.h >>> index bcf3a43ae1b..a22342b2610 100644 >>> --- a/gcc/ctfc.h >>> +++ b/gcc/ctfc.h >>> @@ -125,6 +125,10 @@ typedef struct GTY (()) ctf_itype >>> #define CTF_FUNC_VARARG 0x1 >>> +/* Enum specific flags. */ >>> +#define CTF_ENUM_F_NONE (0) >>> +#define CTF_ENUM_F_ENUMERATORS_SIGNED (1 << 0) >>> + >>> /* Struct/union/enum member definition for CTF generation. */ >>> typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef >>> @@ -133,7 +137,7 @@ typedef struct GTY ((chain_next ("%h.dmd_next"))) >>> ctf_dmdef >>> ctf_id_t dmd_type; /* Type of this member (for sou). */ >>> uint32_t dmd_name_offset; /* Offset of the name in str table. */ >>> uint64_t dmd_offset; /* Offset of this member in bits (for >>> sou). */ >>> - int dmd_value; /* Value of this member (for enum). */ >>> + HOST_WIDE_INT dmd_value; /* Value of this member (for enum). */ >>> struct ctf_dmdef * dmd_next; /* A list node. */ >>> } ctf_dmdef_t; >> >> I am wondering if you considered adding a member here instead - >> something like- >> >> bool dmd_value_signed; /* Signedness for the enumerator. */. >> >> See comment below. >> >>> @@ -162,6 +166,7 @@ struct GTY ((for_user)) ctf_dtdef >>> bool from_global_func; /* Whether this type was added from a global >>> function. */ >>> uint32_t linkage; /* Used in function types. 0=local, >>> 1=global. */ >>> + uint32_t flags; /* Flags to describe specific type's >>> properties. */ >>> union GTY ((desc ("ctf_dtu_d_union_selector (&%1)"))) >>> { >>> /* struct, union, or enum. */ >> >> Instead of carrying this information in ctf_dtdef which is the data >> structure for each type in CTF, how about adding a new member in >> struct ctf_dmdef? The "flags" member is meant for only enum types, and >> hence it will be more appropriate to add to ctf_dmdef as say, >> dmd_value_signed. >> > > Yes, `ctf_dtdef' is structure for each type in CTF (including enumeration), > and `ctf_dmdef' keeps information for enumerator, not for the > enumeration type. Yes, please scrap my earlier suggestion of adding to ctf_dmdef_t. What do you think about adding something like 'dtd_enum_signedness' to ctf_dtdef, instead of uint32_t 'flags'; with two possible values of 0 (unsigned) and 1 (signed). I believe your intention of using the latter is to conserve some memory in the long run (by reusing the flags field for other types in future if need be)? I do, however, prefer an explicit member like dtd_enum_signedness at this time. My reasoning for keeping it explicit is that it helps code be more readable/maintainable. Thanks for your patience, Indu
On 10/11/22 13:55, Indu Bhagat wrote: > Hi Guillermo, > Hi Indu, > On 10/3/22 7:39 AM, Guillermo E. Martinez via Gcc-patches wrote: >>>> diff --git a/gcc/ctfc.cc b/gcc/ctfc.cc >>>> index 9773358a475..253c36b6a0a 100644 >>>> --- a/gcc/ctfc.cc >>>> +++ b/gcc/ctfc.cc >>>> @@ -604,6 +604,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, >>>> gcc_assert (size <= CTF_MAX_SIZE); >>>> dtd->dtd_data.ctti_size = size; >>>> + dtd->flags = CTF_ENUM_F_NONE; >>>> ctfc->ctfc_num_stypes++; >>>> @@ -612,7 +613,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, >>>> int >>>> ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >>>> - HOST_WIDE_INT value, dw_die_ref die) >>>> + HOST_WIDE_INT value, uint32_t flags, dw_die_ref die) >>>> { >>>> ctf_dmdef_t * dmd; >>>> uint32_t kind, vlen, root; >>>> @@ -630,10 +631,12 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >>>> gcc_assert (kind == CTF_K_ENUM && vlen < CTF_MAX_VLEN); >>>> - /* Enum value is of type HOST_WIDE_INT in the compiler, dmd_value is int32_t >>>> - on the other hand. Check bounds and skip adding this enum value if out of >>>> - bounds. */ >>>> - if ((value > INT_MAX) || (value < INT_MIN)) >>>> + /* Enum value is of type HOST_WIDE_INT in the compiler, CTF enumerators >>>> + values in ctf_enum_t is limited to int32_t, BTF supports signed and >>>> + unsigned enumerators values of 32 and 64 bits, for both debug formats >>>> + we use ctf_dmdef_t.dmd_value entry of HOST_WIDE_INT type. So check >>>> + CTF bounds and skip adding this enum value if out of bounds. */ >>>> + if (!btf_debuginfo_p() && ((value > INT_MAX) || (value < INT_MIN))) >>>> { >>>> /* FIXME - Note this TBD_CTF_REPRESENTATION_LIMIT. */ >>>> return (1); >>>> @@ -649,6 +652,7 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, >>>> dmd->dmd_value = value; >>>> dtd->dtd_data.ctti_info = CTF_TYPE_INFO (kind, root, vlen + 1); >>>> + dtd->flags |= flags; >>>> ctf_dmd_list_append (&dtd->dtd_u.dtu_members, dmd); >>>> if ((name != NULL) && strcmp (name, "")) >>>> diff --git a/gcc/ctfc.h b/gcc/ctfc.h >>>> index bcf3a43ae1b..a22342b2610 100644 >>>> --- a/gcc/ctfc.h >>>> +++ b/gcc/ctfc.h >>>> @@ -125,6 +125,10 @@ typedef struct GTY (()) ctf_itype >>>> #define CTF_FUNC_VARARG 0x1 >>>> +/* Enum specific flags. */ >>>> +#define CTF_ENUM_F_NONE (0) >>>> +#define CTF_ENUM_F_ENUMERATORS_SIGNED (1 << 0) >>>> + >>>> /* Struct/union/enum member definition for CTF generation. */ >>>> typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef >>>> @@ -133,7 +137,7 @@ typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef >>>> ctf_id_t dmd_type; /* Type of this member (for sou). */ >>>> uint32_t dmd_name_offset; /* Offset of the name in str table. */ >>>> uint64_t dmd_offset; /* Offset of this member in bits (for sou). */ >>>> - int dmd_value; /* Value of this member (for enum). */ >>>> + HOST_WIDE_INT dmd_value; /* Value of this member (for enum). */ >>>> struct ctf_dmdef * dmd_next; /* A list node. */ >>>> } ctf_dmdef_t; >>> >>> I am wondering if you considered adding a member here instead - something like- >>> >>> bool dmd_value_signed; /* Signedness for the enumerator. */. >>> >>> See comment below. >>> >>>> @@ -162,6 +166,7 @@ struct GTY ((for_user)) ctf_dtdef >>>> bool from_global_func; /* Whether this type was added from a global >>>> function. */ >>>> uint32_t linkage; /* Used in function types. 0=local, 1=global. */ >>>> + uint32_t flags; /* Flags to describe specific type's properties. */ >>>> union GTY ((desc ("ctf_dtu_d_union_selector (&%1)"))) >>>> { >>>> /* struct, union, or enum. */ >>> >>> Instead of carrying this information in ctf_dtdef which is the data structure for each type in CTF, how about adding a new member in struct ctf_dmdef? The "flags" member is meant for only enum types, and hence it will be more appropriate to add to ctf_dmdef as say, dmd_value_signed. >>> >> >> Yes, `ctf_dtdef' is structure for each type in CTF (including enumeration), >> and `ctf_dmdef' keeps information for enumerator, not for the enumeration type. > > Yes, please scrap my earlier suggestion of adding to ctf_dmdef_t. > > What do you think about adding something like 'dtd_enum_signedness' to ctf_dtdef, instead of uint32_t 'flags'; with two possible values of 0 (unsigned) and 1 (signed). > OK. I'll use a bool variable field named `dtd_enum_unsiged' like to ENUMERAL_TYPE does storing signedness in `.base.u.bits.unsigned_flag', later this information is used to set the DW_AT_encoding attribute to DW_ATE_{unsigned,signed}, finally with this information we can set the dtd_enum_unsiged variable in `gen_ctf_enumeration_type', so no need to iterate over enumerators constant to figure out signedness of the enumeration type. But, please let me know your thoughts in patchv3. > I believe your intention of using the latter is to conserve some memory in the long run (by reusing the flags field for other types in future if need be)? I do, however, prefer an explicit member like dtd_enum_signedness at this time. My reasoning for keeping it explicit is that it helps code be more readable/maintainable. > Sure, I will do so. > Thanks for your patience, No, thanks to you for your comments! > Indu guillermo
diff --git a/gcc/btfout.cc b/gcc/btfout.cc index 997a33fa089..4b11c867c23 100644 --- a/gcc/btfout.cc +++ b/gcc/btfout.cc @@ -223,7 +223,9 @@ btf_calc_num_vbytes (ctf_dtdef_ref dtd) break; case BTF_KIND_ENUM: - vlen_bytes += vlen * sizeof (struct btf_enum); + vlen_bytes += (dtd->dtd_data.ctti_size == 0x8) + ? vlen * sizeof (struct btf_enum64) + : vlen * sizeof (struct btf_enum); break; case BTF_KIND_FUNC_PROTO: @@ -622,6 +624,15 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) btf_size_type = 0; } + if (btf_kind == BTF_KIND_ENUM) + { + btf_kflag = (dtd->flags & CTF_ENUM_F_ENUMERATORS_SIGNED) + ? BTF_KF_ENUM_SIGNED + : BTF_KF_ENUM_UNSIGNED; + if (dtd->dtd_data.ctti_size == 0x8) + btf_kind = BTF_KIND_ENUM64; + } + dw2_asm_output_data (4, dtd->dtd_data.ctti_name, "btt_name"); dw2_asm_output_data (4, BTF_TYPE_INFO (btf_kind, btf_kflag, btf_vlen), "btt_info: kind=%u, kflag=%u, vlen=%u", @@ -634,6 +645,7 @@ btf_asm_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd) case BTF_KIND_UNION: case BTF_KIND_ENUM: case BTF_KIND_DATASEC: + case BTF_KIND_ENUM64: dw2_asm_output_data (4, dtd->dtd_data.ctti_size, "btt_size: %uB", dtd->dtd_data.ctti_size); return; @@ -707,13 +719,13 @@ btf_asm_sou_member (ctf_container_ref ctfc, ctf_dmdef_t * dmd) } } -/* Asm'out an enum constant following a BTF_KIND_ENUM. */ +/* Asm'out an enum constant following a BTF_KIND_ENUM{,64}. */ static void -btf_asm_enum_const (ctf_dmdef_t * dmd) +btf_asm_enum_const (unsigned int size, ctf_dmdef_t * dmd) { dw2_asm_output_data (4, dmd->dmd_name_offset, "bte_name"); - dw2_asm_output_data (4, dmd->dmd_value, "bte_value"); + dw2_asm_output_data (size, dmd->dmd_value, "bte_value"); } /* Asm'out a function parameter description following a BTF_KIND_FUNC_PROTO. */ @@ -871,7 +883,7 @@ output_asm_btf_sou_fields (ctf_container_ref ctfc, ctf_dtdef_ref dtd) btf_asm_sou_member (ctfc, dmd); } -/* Output all enumerator constants following a BTF_KIND_ENUM. */ +/* Output all enumerator constants following a BTF_KIND_ENUM{,64}. */ static void output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), @@ -881,7 +893,7 @@ output_asm_btf_enum_list (ctf_container_ref ARG_UNUSED (ctfc), for (dmd = dtd->dtd_u.dtu_members; dmd != NULL; dmd = (ctf_dmdef_t *) ctf_dmd_list_next (dmd)) - btf_asm_enum_const (dmd); + btf_asm_enum_const (dtd->dtd_data.ctti_size, dmd); } /* Output all function arguments following a BTF_KIND_FUNC_PROTO. */ diff --git a/gcc/ctfc.cc b/gcc/ctfc.cc index 9773358a475..253c36b6a0a 100644 --- a/gcc/ctfc.cc +++ b/gcc/ctfc.cc @@ -604,6 +604,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, gcc_assert (size <= CTF_MAX_SIZE); dtd->dtd_data.ctti_size = size; + dtd->flags = CTF_ENUM_F_NONE; ctfc->ctfc_num_stypes++; @@ -612,7 +613,7 @@ ctf_add_enum (ctf_container_ref ctfc, uint32_t flag, const char * name, int ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, - HOST_WIDE_INT value, dw_die_ref die) + HOST_WIDE_INT value, uint32_t flags, dw_die_ref die) { ctf_dmdef_t * dmd; uint32_t kind, vlen, root; @@ -630,10 +631,12 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, gcc_assert (kind == CTF_K_ENUM && vlen < CTF_MAX_VLEN); - /* Enum value is of type HOST_WIDE_INT in the compiler, dmd_value is int32_t - on the other hand. Check bounds and skip adding this enum value if out of - bounds. */ - if ((value > INT_MAX) || (value < INT_MIN)) + /* Enum value is of type HOST_WIDE_INT in the compiler, CTF enumerators + values in ctf_enum_t is limited to int32_t, BTF supports signed and + unsigned enumerators values of 32 and 64 bits, for both debug formats + we use ctf_dmdef_t.dmd_value entry of HOST_WIDE_INT type. So check + CTF bounds and skip adding this enum value if out of bounds. */ + if (!btf_debuginfo_p() && ((value > INT_MAX) || (value < INT_MIN))) { /* FIXME - Note this TBD_CTF_REPRESENTATION_LIMIT. */ return (1); @@ -649,6 +652,7 @@ ctf_add_enumerator (ctf_container_ref ctfc, ctf_id_t enid, const char * name, dmd->dmd_value = value; dtd->dtd_data.ctti_info = CTF_TYPE_INFO (kind, root, vlen + 1); + dtd->flags |= flags; ctf_dmd_list_append (&dtd->dtd_u.dtu_members, dmd); if ((name != NULL) && strcmp (name, "")) diff --git a/gcc/ctfc.h b/gcc/ctfc.h index bcf3a43ae1b..a22342b2610 100644 --- a/gcc/ctfc.h +++ b/gcc/ctfc.h @@ -125,6 +125,10 @@ typedef struct GTY (()) ctf_itype #define CTF_FUNC_VARARG 0x1 +/* Enum specific flags. */ +#define CTF_ENUM_F_NONE (0) +#define CTF_ENUM_F_ENUMERATORS_SIGNED (1 << 0) + /* Struct/union/enum member definition for CTF generation. */ typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef @@ -133,7 +137,7 @@ typedef struct GTY ((chain_next ("%h.dmd_next"))) ctf_dmdef ctf_id_t dmd_type; /* Type of this member (for sou). */ uint32_t dmd_name_offset; /* Offset of the name in str table. */ uint64_t dmd_offset; /* Offset of this member in bits (for sou). */ - int dmd_value; /* Value of this member (for enum). */ + HOST_WIDE_INT dmd_value; /* Value of this member (for enum). */ struct ctf_dmdef * dmd_next; /* A list node. */ } ctf_dmdef_t; @@ -162,6 +166,7 @@ struct GTY ((for_user)) ctf_dtdef bool from_global_func; /* Whether this type was added from a global function. */ uint32_t linkage; /* Used in function types. 0=local, 1=global. */ + uint32_t flags; /* Flags to describe specific type's properties. */ union GTY ((desc ("ctf_dtu_d_union_selector (&%1)"))) { /* struct, union, or enum. */ @@ -429,7 +434,7 @@ extern ctf_id_t ctf_add_sou (ctf_container_ref, uint32_t, const char *, uint32_t, size_t, dw_die_ref); extern int ctf_add_enumerator (ctf_container_ref, ctf_id_t, const char *, - HOST_WIDE_INT, dw_die_ref); + HOST_WIDE_INT, uint32_t, dw_die_ref); extern int ctf_add_member_offset (ctf_container_ref, dw_die_ref, const char *, ctf_id_t, uint64_t); extern int ctf_add_function_arg (ctf_container_ref, dw_die_ref, diff --git a/gcc/dwarf2ctf.cc b/gcc/dwarf2ctf.cc index 397100004c2..0ef96dd48fd 100644 --- a/gcc/dwarf2ctf.cc +++ b/gcc/dwarf2ctf.cc @@ -772,6 +772,7 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) const char *enumerator_name; dw_attr_node *enumerator_value; HOST_WIDE_INT value_wide_int; + uint32_t flags = 0; c = dw_get_die_sib (c); @@ -785,10 +786,14 @@ gen_ctf_enumeration_type (ctf_container_ref ctfc, dw_die_ref enumeration) == dw_val_class_unsigned_const_implicit)) value_wide_int = AT_unsigned (enumerator_value); else - value_wide_int = AT_int (enumerator_value); + { + value_wide_int = AT_int (enumerator_value); + flags |= CTF_ENUM_F_ENUMERATORS_SIGNED; + } ctf_add_enumerator (ctfc, enumeration_type_id, - enumerator_name, value_wide_int, enumeration); + enumerator_name, value_wide_int, + flags, enumeration); } while (c != dw_get_die_child (enumeration)); } diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c index 728493b0804..7e940529f1b 100644 --- a/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum-1.c @@ -4,7 +4,7 @@ /* { dg-options "-O0 -gbtf -dA" } */ /* { dg-final { scan-assembler-times "\[\t \]0x6000004\[\t \]+\[^\n\]*btt_info" 1 } } */ -/* { dg-final { scan-assembler-times "\[\t \]0x6000003\[\t \]+\[^\n\]*btt_info" 1 } } */ +/* { dg-final { scan-assembler-times "\[\t \]0x86000003\[\t \]+\[^\n\]*btt_info" 1 } } */ /* { dg-final { scan-assembler-times "ascii \"QAD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ /* { dg-final { scan-assembler-times "ascii \"QED.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ /* { dg-final { scan-assembler-times "ascii \"QOD.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c new file mode 100644 index 00000000000..da103842807 --- /dev/null +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-enum64-1.c @@ -0,0 +1,41 @@ +/* Test BTF generation for 64 bits enums. */ + +/* { dg-do compile } */ +/* { dg-options "-O0 -gbtf -dA" } */ + +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum1,\[\t \]8" 1 } } */ +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum2,\[\t \]8" 1 } } */ +/* { dg-final { scan-assembler-times "\[\t \].size\[\t \]myenum3,\[\t \]8" 1 } } */ +/* { dg-final { scan-assembler-times "\[\t \]0x13000003\[\t \]+\[^\n\]*btt_info" 2 } } */ +/* { dg-final { scan-assembler-times "\[\t \]0x93000003\[\t \]+\[^\n\]*btt_info" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"B1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"B2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"B3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"C1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"C2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"C3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"D1.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"D2.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "ascii \"D3.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */ +/* { dg-final { scan-assembler-times "bte_value" 9 } } */ + +enum default_enum +{ + B1 = 0xffffffffaa, + B2 = 0xbbbbbbbb, + B3 = 0xaabbccdd, +} myenum1 = B1; + +enum explicit_unsigned +{ + C1 = 0xffffffffbbUL, + C2 = 0xbbbbbbbb, + C3 = 0xaabbccdd, +} myenum2 = C1; + +enum signed64 +{ + D1 = 0xffffffffaa, + D2 = 0xbbbbbbbb, + D3 = -0x1, +} myenum3 = D1; diff --git a/include/btf.h b/include/btf.h index 78b551ced23..eba67f9d599 100644 --- a/include/btf.h +++ b/include/btf.h @@ -109,7 +109,8 @@ struct btf_type #define BTF_KIND_VAR 14 /* Variable. */ #define BTF_KIND_DATASEC 15 /* Section such as .bss or .data. */ #define BTF_KIND_FLOAT 16 /* Floating point. */ -#define BTF_KIND_MAX BTF_KIND_FLOAT +#define BTF_KIND_ENUM64 19 /* Enumeration up to 64 bits. */ +#define BTF_KIND_MAX BTF_KIND_ENUM64 #define NR_BTF_KINDS (BTF_KIND_MAX + 1) /* For some BTF_KINDs, struct btf_type is immediately followed by @@ -130,14 +131,17 @@ struct btf_type #define BTF_INT_BOOL (1 << 2) /* BTF_KIND_ENUM is followed by VLEN struct btf_enum entries, - which describe the enumerators. Note that BTF currently only - supports signed 32-bit enumerator values. */ + which describe the enumerators. */ struct btf_enum { uint32_t name_off; /* Offset in string section of enumerator name. */ int32_t val; /* Enumerator value. */ }; +/* BTF_KF_ENUM_ holds the flags for kflags in BTF_KIND_ENUM{,64}. */ +#define BTF_KF_ENUM_UNSIGNED (0) +#define BTF_KF_ENUM_SIGNED (1 << 0) + /* BTF_KIND_ARRAY is followed by a single struct btf_array. */ struct btf_array { @@ -190,6 +194,15 @@ struct btf_var_secinfo uint32_t size; /* Size (in bytes) of variable. */ }; +/* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries, + which describe the 64 bits enumerators. */ +struct btf_enum64 +{ + uint32_t name_off; /* Offset in string section of enumerator name. */ + uint32_t val_lo32; /* lower 32-bit value for a 64-bit value Enumerator */ + uint32_t val_hi32; /* high 32-bit value for a 64-bit value Enumerator */ +}; + #ifdef __cplusplus } #endif