Re: [PATCH, V3] ctf: fix incorrect CTF for multi-dimensional array types

2024-03-05 Thread David Faust



On 3/5/24 00:47, Indu Bhagat wrote:
> From: Cupertino Miranda 
> 
> [Changes from V2]
>   - Fixed aarch64 new FAILs reported by Linaro CI.
>   - Fixed typos and other nits pointed out in V2.
> [End of changes from V2]

OK, thanks.

> 
> 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 
> 
> 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:
>  - Linaro CI reported three new FAILs introduced by ctf-array-6.c due to
>presence of char '#' on aarch64 where the ASM_COMMENT_START differs.
>Fixed and regression tested on aarch64.
>  - Regression tested on x86_64-linux-gnu default target.
>  - Regression tested for target bpf-unknown-none (btf.exp, ctf.exp, bpf.exp).
>  - Kernel build with -gctf shows healthier CTF types for arrays.
> 
> ---
>  gcc/dwarf2ctf.cc | 158 +--
>  gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c |  14 ++
>  2 files changed, 89 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..77d6bf896893 100644
> --- a/gcc/dwarf2ctf.cc
> +++ b/gcc/dwarf2ctf.cc
> @@ -349,105 +349,97 @@ 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;
>  
> -  int vector_type_p = get_AT_flag (array_type, DW_AT_GNU_vector);
> -  if (vector_type_p)
> -return array_elems_type_id;
> +  dw_attr_node *upper_bound_at;
> +  dw_die_ref array_index_type;
> +  uint32_t array_num_elements;
>  
> -  dw_die_ref array_elems_type = ctf_get_AT_type (array_type);
> +  if (dw_get_die_tag (c) == DW_TAG_subrange_type)
> +{
> +  /* 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
> +gcc_unreachable ();
>  
> -  /* First, register the type of the array elements if needed.  */
> -  array_elems_type_id = gen_ctf_type (ctfc, array_elems_type);
> +  /* 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;
>  
> -  /* 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.
> +  array_index_type = ctf_get_AT_type (c);
> +  arinfo.ctr_index = gen_ctf_type (ctfc, array_index_type);
>  
> - 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.  */
> +  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);
>  
> -  c = dw_get_die_child (array_type);
> -  gcc_assert 

[PATCH, V3] ctf: fix incorrect CTF for multi-dimensional array types

2024-03-05 Thread Indu Bhagat
From: Cupertino Miranda 

[Changes from V2]
  - Fixed aarch64 new FAILs reported by Linaro CI.
  - Fixed typos and other nits pointed out in V2.
[End of changes from V2]

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 

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:
 - Linaro CI reported three new FAILs introduced by ctf-array-6.c due to
   presence of char '#' on aarch64 where the ASM_COMMENT_START differs.
   Fixed and regression tested on aarch64.
 - Regression tested on x86_64-linux-gnu default target.
 - Regression tested for target bpf-unknown-none (btf.exp, ctf.exp, bpf.exp).
 - Kernel build with -gctf shows healthier CTF types for arrays.

---
 gcc/dwarf2ctf.cc | 158 +--
 gcc/testsuite/gcc.dg/debug/ctf/ctf-array-6.c |  14 ++
 2 files changed, 89 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..77d6bf896893 100644
--- a/gcc/dwarf2ctf.cc
+++ b/gcc/dwarf2ctf.cc
@@ -349,105 +349,97 @@ 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;
 
-  int vector_type_p = get_AT_flag (array_type, DW_AT_GNU_vector);
-  if (vector_type_p)
-return array_elems_type_id;
+  dw_attr_node *upper_bound_at;
+  dw_die_ref array_index_type;
+  uint32_t array_num_elements;
 
-  dw_die_ref array_elems_type = ctf_get_AT_type (array_type);
+  if (dw_get_die_tag (c) == DW_TAG_subrange_type)
+{
+  /* 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
+gcc_unreachable ();
 
-  /* First, register the type of the array elements if needed.  */
-  array_elems_type_id = gen_ctf_type (ctfc, array_elems_type);
+  /* 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;
 
-  /* 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.
+  array_index_type = ctf_get_AT_type (c);
+  arinfo.ctr_index = gen_ctf_type (ctfc, array_index_type);
 
- 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.  */
+  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);
 
-  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;
+  if (!ctf_type_exists (ctfc, c, _node_type_id))
+array_node_type_id = ctf_add_array (ctfc, CTF_ADD_ROOT, , c);
 
-  c = dw_get_die_sib