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

[Indu Bhagat via Gcc-patches]

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