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

[Indu Bhagat <indu.bhagat@oracle.com>]

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