Hi Indu, Cupertino,
On 3/4/24 10:00, Indu Bhagat wrote:
> From: Cupertino Miranda <cupertino.mira...@oracle.com>
>
> [Changes from V1]
> - Refactor the code a bit.
> [End of changes from V1]
>
> PR debug/114186
>
> DWARF DIEs of type DW_TAG_subrange_type are linked together to represent
> the information about the subsequent dimensions. The CTF processing was
> so far working through them in the opposite (incorrect) order.
>
> While fixing the issue, refactor the code a bit for readability.
>
> co-authored-By: Indu Bhagat <indu.bha...@oracle.com>
Thanks for the patch and refactor. I do find v2 easier to follow.
Two very minor typos in comments, noted inline below.
Otherwise, LGTM and OK.
Thanks!
>
> gcc/
> PR debug/114186
> * dwarf2ctf.cc (gen_ctf_array_type): Invoke the ctf_add_array ()
> in the correct order of the dimensions.
> (gen_ctf_subrange_type): Refactor out handling of
> DW_TAG_subrange_type DIE to here.
>
> gcc/testsuite/
> PR debug/114186
> * gcc.dg/debug/ctf/ctf-array-6.c: Add test.
> ---
>
> Testing notes:
>
> Regression tested on x86_64-linux-gnu default target.
> Regression tested for target bpf-unknown-none (btf.exp, ctf.exp, bpf.exp).
>
> ---
> gcc/dwarf2ctf.cc | 153 +++++++++----------
> gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c | 14 ++
> 2 files changed, 84 insertions(+), 83 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c
>
> diff --git a/gcc/dwarf2ctf.cc b/gcc/dwarf2ctf.cc
> index dca86edfffa9..3985de115a79 100644
> --- a/gcc/dwarf2ctf.cc
> +++ b/gcc/dwarf2ctf.cc
> @@ -349,105 +349,92 @@ gen_ctf_pointer_type (ctf_container_ref ctfc,
> dw_die_ref ptr_type)
> return ptr_type_id;
> }
>
> -/* Generate CTF for an array type. */
> +/* Recursively generate CTF for array dimensions starting at DIE C (of type
> + DW_TAG_subrange_type) until DIE LAST (of type DW_TAG_subrange_type) is
> + reached. ARRAY_ELEMS_TYPE_ID is base type for the array. */
>
> static ctf_id_t
> -gen_ctf_array_type (ctf_container_ref ctfc, dw_die_ref array_type)
> +gen_ctf_subrange_type (ctf_container_ref ctfc, ctf_id_t array_elems_type_id,
> + dw_die_ref c, dw_die_ref last)
> {
> - dw_die_ref c;
> - ctf_id_t array_elems_type_id = CTF_NULL_TYPEID;
> + ctf_arinfo_t arinfo;
> + ctf_id_t array_node_type_id = CTF_NULL_TYPEID;
> +
> + dw_attr_node *upper_bound_at;
> + dw_die_ref array_index_type;
> + uint32_t array_num_elements;
> +
> + /* When DW_AT_upper_bound is used to specify the size of an
> + array in DWARF, it is usually an unsigned constant
> + specifying the upper bound index of the array. However,
> + for unsized arrays, such as foo[] or bar[0],
> + DW_AT_upper_bound is a signed integer constant
> + instead. */
> +
> + upper_bound_at = get_AT (c, DW_AT_upper_bound);
> + if (upper_bound_at
> + && AT_class (upper_bound_at) == dw_val_class_unsigned_const)
> + /* This is the ound index. */
typo, I guess this is meant to be "bound" ?
> + array_num_elements = get_AT_unsigned (c, DW_AT_upper_bound) + 1;
> + else if (get_AT (c, DW_AT_count))
> + array_num_elements = get_AT_unsigned (c, DW_AT_count);
> + else
> + {
> + /* This is a VLA of some kind. */
> + array_num_elements = 0;
> + }
>
> - int vector_type_p = get_AT_flag (array_type, DW_AT_GNU_vector);
> - if (vector_type_p)
> - return array_elems_type_id;
> + /* Ok, mount and register the array type. Note how the array
> + type we register here is the type of the elements in
> + subsequent "dimensions", if there are any. */
> + arinfo.ctr_nelems = array_num_elements;
>
> - dw_die_ref array_elems_type = ctf_get_AT_type (array_type);
> + array_index_type = ctf_get_AT_type (c);
> + arinfo.ctr_index = gen_ctf_type (ctfc, array_index_type);
>
> - /* First, register the type of the array elements if needed. */
> - array_elems_type_id = gen_ctf_type (ctfc, array_elems_type);
> + if (c == last)
> + arinfo.ctr_contents = array_elems_type_id;
> + else
> + arinfo.ctr_contents = gen_ctf_subrange_type (ctfc, array_elems_type_id,
> + dw_get_die_sib (c), last);
>
> - /* DWARF array types pretend C supports multi-dimensional arrays.
> - So for the type int[N][M], the array type DIE contains two
> - subrange_type children, the first with upper bound N-1 and the
> - second with upper bound M-1.
> + if (!ctf_type_exists (ctfc, c, &array_node_type_id))
> + array_node_type_id = ctf_add_array (ctfc, CTF_ADD_ROOT, &arinfo, c);
>
> - CTF, on the other hand, just encodes each array type in its own
> - array type CTF struct. Therefore we have to iterate on the
> - children and create all the needed types. */
> + return array_node_type_id;
> +}
>
> - c = dw_get_die_child (array_type);
> - gcc_assert (c);
> - do
> - {
> - ctf_arinfo_t arinfo;
> - dw_die_ref array_index_type;
> - uint32_t array_num_elements;
> +/* Generate CTF for an ARRAY_TYPE. */
>
> - c = dw_get_die_sib (c);
> +static ctf_id_t
> +gen_ctf_array_type (ctf_container_ref ctfc,
> + dw_die_ref array_type)
> +{
> + dw_die_ref first, last, array_elems_type;
> + ctf_id_t array_elems_type_id = CTF_NULL_TYPEID;
> + ctf_id_t array_type_id = CTF_NULL_TYPEID;
>
> - if (dw_get_die_tag (c) == DW_TAG_subrange_type)
> - {
> - dw_attr_node *upper_bound_at;
> -
> - array_index_type = ctf_get_AT_type (c);
> -
> - /* When DW_AT_upper_bound is used to specify the size of an
> - array in DWARF, it is usually an unsigned constant
> - specifying the upper bound index of the array. However,
> - for unsized arrays, such as foo[] or bar[0],
> - DW_AT_upper_bound is a signed integer constant
> - instead. */
> -
> - upper_bound_at = get_AT (c, DW_AT_upper_bound);
> - if (upper_bound_at
> - && AT_class (upper_bound_at) == dw_val_class_unsigned_const)
> - /* This is the upper bound index. */
> - array_num_elements = get_AT_unsigned (c, DW_AT_upper_bound) + 1;
> - else if (get_AT (c, DW_AT_count))
> - array_num_elements = get_AT_unsigned (c, DW_AT_count);
> - else
> - {
> - /* This is a VLA of some kind. */
> - array_num_elements = 0;
> - }
> - }
> - else if (dw_get_die_tag (c) == DW_TAG_enumeration_type)
> - {
> - array_index_type = 0;
> - array_num_elements = 0;
> - /* XXX writeme. */
> - gcc_assert (1);
> - }
> - else
> - gcc_unreachable ();
> + int vector_type_p = get_AT_flag (array_type, DW_AT_GNU_vector);
> + if (vector_type_p)
> + return array_elems_type_id;
>
> - /* Ok, mount and register the array type. Note how the array
> - type we register here is the type of the elements in
> - subsequent "dimensions", if there are any. */
> + /* Find the first and last array dimension DIEs. */
> + last = dw_get_die_child (array_type);
> + first = dw_get_die_sib (last);
>
> - arinfo.ctr_nelems = array_num_elements;
> - if (array_index_type)
> - arinfo.ctr_index = gen_ctf_type (ctfc, array_index_type);
> - else
> - arinfo.ctr_index = gen_ctf_type (ctfc, ctf_array_index_die);
> + /* Type de-duplication.
> + Consult the ctfc_types before adding CTF type for the first dimension.
> */
> + if (!ctf_type_exists (ctfc, first, &array_type_id))
> + {
> + array_elems_type = ctf_get_AT_type (array_type);
> + /* First, register the type of the array elements if needed. */
> + array_elems_type_id = gen_ctf_type (ctfc, array_elems_type);
>
> - arinfo.ctr_contents = array_elems_type_id;
> - if (!ctf_type_exists (ctfc, c, &array_elems_type_id))
> - array_elems_type_id = ctf_add_array (ctfc, CTF_ADD_ROOT, &arinfo,
> - c);
> + array_type_id = gen_ctf_subrange_type (ctfc, array_elems_type_id,
> first,
> + last);
> }
> - while (c != dw_get_die_child (array_type));
>
> -#if 0
> - /* Type de-duplication.
> - Consult the ctfc_types hash again before adding the CTF array type
> because
> - there can be cases where an array_type type may have been added by the
> - gen_ctf_type call above. */
> - if (!ctf_type_exists (ctfc, array_type, &type_id))
> - type_id = ctf_add_array (ctfc, CTF_ADD_ROOT, &arinfo, array_type);
> -#endif
> -
> - return array_elems_type_id;
> + return array_type_id;
> }
>
> /* Generate CTF for a typedef. */
> diff --git a/gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c
> b/gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c
> new file mode 100644
> index 000000000000..5ecbb049535d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c
> @@ -0,0 +1,14 @@
> +/* CTF generation for multidimentional array. */
nit: multidimensional (note 's' rather than 't')
> +
> +/* { dg-do compile ) */
> +/* { dg-options "-O0 -gctf -dA" } */
> +
> +/* { dg-final { scan-assembler-times "\[\t \]+0x2\[\t \]+\[^\n\]*cta_nelems"
> 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]+0x3\[\t \]+\[^\n\]*cta_nelems"
> 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]+0x4\[\t \]+\[^\n\]*cta_nelems"
> 1 } } */
> +
> +/* { dg-final { scan-assembler-times "\t.\[^\t \]+\t0x1\[\t \]+#
> cta_contents\[\\r\\n\]+\t.\[^\t \]+\t0x2\[\t \]+#
> cta_index\[\\r\\n\]+\t.\[^\t \]+\t0x4\[\t \]+# cta_nelems" 1 } } */
> +/* { dg-final { scan-assembler-times "\t.\[^\t \]+\t0x3\[\t \]+#
> cta_contents\[\\r\\n\]+\t.\[^\t \]+\t0x2\[\t \]+#
> cta_index\[\\r\\n\]+\t.\[^\t \]+\t0x3\[\t \]+# cta_nelems" 1 } } */
> +/* { dg-final { scan-assembler-times "\t.\[^\t \]+\t0x4\[\t \]+#
> cta_contents\[\\r\\n\]+\t.\[^\t \]+\t0x2\[\t \]+#
> cta_index\[\\r\\n\]+\t.\[^\t \]+\t0x2\[\t \]+# cta_nelems" 1 } } */
> +
> +int a[2][3][4];
