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

Reply via email to