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
