Re: [PATCH v2] btf: Add support to BTF_KIND_ENUM64 type

["Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>]

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