Re: [PATCH 3/5] ctf-front-end: Strip qualification from a qualified array type

[Dodji Seketeli <dodji@seketeli.org>]

Hello Guillermo,

"Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
écrit:

Thank you for this work!

The patch in general looks good to me.  I have made some minor
adjustments and applied the result to the repository.

Please find below the few comments that I have.

> Redundant qualifier is emitted by CTF frond-end building IR for array
> definitions:
>
>    const char a[32];
>
>    <qualified-type-def type-id='type-id-1' const='yes' id='type-id-2'/>
>    <qualified-type-def type-id='type-id-3' const='yes' id='type-id-6'/>
>
> Thus the diff reports:
>    [C] 'const char[32] const a'
>
> So, It strips the qualifiers from the array types.

I see.  We do something similar in the DWARF front-end.  This is because
GCC emits some redundant const qualifier in these cases.

I thought I'd give a little bit more context because I am seeing now
that this is not obvious.  I had to look it up a little to remember
fully ;-)

Sorry, I haven't given enough context in the DWARF front-end so I am
repairing my bad here.

So, here is what I am proposing as an introductory comment to this
commit log:

    Sometimes, GCC emits some redundant const qualifiers around arrays.

    For instance, consider this function:

        $ cat -n test.c
             1	const char a[32];
             2
             3	char
             4	foo()
             5	{
             6	  return a[0];
             7	}
        $

    Notice how at line 1, the type of the variable 'a' is "array of const
    char".

    Let's compile the function and emit CTF debug info:

        $ gcc -gctf -c test.c
        $

    Let's see what IR libabigail emits from the CTF information:

        $ abidw --ctf --annotate test.o  | cat -n
             1	<abi-corpus version='2.1' path='test.o' architecture='elf-amd-x86_64'>
             2	  <elf-function-symbols>
             3	    <!-- foo -->
             4	    <elf-symbol name='foo' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
             5	  </elf-function-symbols>
             6	  <elf-variable-symbols>
             7	    <!-- signed char -->
             8	    <elf-symbol name='a' size='32' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
             9	  </elf-variable-symbols>
            10	  <abi-instr address-size='64' language='LANG_C'>
            11	    <!-- char -->
            12	    <type-decl name='char' size-in-bits='8' alignment-in-bits='8' id='type-id-1'/>
            13	    <!-- const char[32] -->
            14	    <array-type-def dimensions='1' type-id='type-id-2' size-in-bits='256' alignment-in-bits='8' id='type-id-3'>
            15	      <!-- <anonymous range>[32] -->
            16	      <subrange length='32' type-id='type-id-4' id='type-id-5'/>
            17	    </array-type-def>
            18	    <!-- unsigned long int -->
            19	    <type-decl name='unsigned long int' size-in-bits='64' alignment-in-bits='64' id='type-id-4'/>
            20	    <!-- const char -->
            21	    <qualified-type-def type-id='type-id-1' const='yes' id='type-id-2'/>
            22	    <!-- const char[32] const -->
            23	    <qualified-type-def type-id='type-id-3' const='yes' id='type-id-6'/>
            24	    <!-- const char[32] const a -->
            25	    <var-decl name='a' type-id='type-id-6' mangled-name='a' visibility='default' elf-symbol-id='a'/>
            26	    <!-- char foo() -->
            27	    <function-decl name='foo' visibility='default' binding='global' size-in-bits='64' alignment-in-bits='8' elf-symbol-id='foo'>
            28	      <!-- char -->
            29	      <return type-id='type-id-1'/>
            30	    </function-decl>
            31	  </abi-instr>
            32	</abi-corpus>
        $

    Notice how at line 25, the variable 'a' is described as having the
    type which ID is 'type-id-6' defined at line 23.  It's a "const array
    of const char".

    GCC has thus added a redundant "const" qualifier to the array.

    The C language specification in paragraph [6.7.3]/8 says:

        If the specification of an array type includes any type
        qualifiers, the element type is so- qualified, not the array type.

    This means that a "const array of char" is the same as an "array of
    const char".  So a "const array of const char" is the same an "array
    of const char".

    This patch performs that removal of redundant qualifier.

[...]


> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc

[...]


> +/// Strip qualification from a qualified type, when it makes sense.
> +///
> +/// CTF constructs "const const char [N]" for "const char foo[N]"
> +/// definition; This redundant types then causes bad diagnostics
> +/// when it is compared with an DWARF IR.
> +///
> +/// This function thus strips the const qualifier from the type in
> +/// that case.  It might contain code to strip other cases like this
> +/// in the future.

I have updated this part of the comment of the function to explain a
little bit more what we want to do here:

    +/// Strip qualification from a qualified type, when it makes sense.
    +///
    +/// The C language specification says in [6.7.3]/8:
    +///
    +///     [If the specification of an array type includes any type
    +///      qualifiers, the element type is so- qualified, not the
    +///      array type.]
    +///
    +/// In more mundane words, a const array of int is the same as an
    +/// array of const int.
    +///
    +/// This function thus removes the qualifiers of the array and applies
    +/// them to the array element.  The function then pretends that the
    +/// array itself it not qualified.
    +///
    +/// It might contain code to strip other cases like this in the
    +/// future.

> +///
> +/// @param t the type to strip const qualification from.
> +///
> +/// @param rdr the @ref reader to use.
> +///
> +/// @return the stripped type or just return @p t.
> +static decl_base_sptr
> +maybe_strip_qualification(const qualified_type_def_sptr t,
> +                          reader *rdr)

I believe the "rdr" parameter is useless here.  So I removed it.  I have
updated the doxygen comment accordingly.

> +{
> +  if (!t)
> +    return t;
> +
> +  decl_base_sptr result = t;
> +  type_base_sptr u = t->get_underlying_type();
> +
> +  if (is_array_type(u) || is_typedef_of_array(u))

I believe the "|| is_typedef_of_array(u)" is not appropriate here
because this code doesn't deal with "typedef of array types" yet.  When
we deal with it (in the future, when we come across cases like that)
we'll handle it here.  So I am removing this part from now.

> +    {

Here, I am adding this comment:

+      // Let's apply the qualifiers of the array to the array element
+      // and pretend that the array itself is not qualified, as per
+      // section [6.7.3]/8 of the C specification.

> +      array_type_def_sptr array = is_array_type(u);
> +      ABG_ASSERT(array);
> +      // We should not be editing types that are already canonicalized.
> +      ABG_ASSERT(!array->get_canonical_type());
> +      type_base_sptr element_type = array->get_element_type();
> +
> +      if (qualified_type_def_sptr qualified = is_qualified_type(element_type))
> +        {
> +          qualified_type_def::CV quals = qualified->get_cv_quals();
> +          quals |= t->get_cv_quals();

I am adding this comment:

+	  // So we apply the qualifiers of the array to the array
+	  // element.

> +          qualified->set_cv_quals(quals);
> +          result = is_decl(u);
> +        }
> +    }
> +
> +  return result;
> +}

[...]

Please find below the resulting patch that I have applied to the master
branch of the git repository.

Cheers,

From 4511e4f5bf2f1575669c886cc8989581d390fbcd Mon Sep 17 00:00:00 2001
From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
Date: Wed, 16 Nov 2022 21:43:03 -0600
Subject: [PATCH] ctf-reader: Strip qualification from a qualified array type

Sometimes, GCC emits some redundant const qualifiers around arrays.

For instance, consider this function:

    $ cat -n test.c
	 1	const char a[32];
	 2
	 3	char
	 4	foo()
	 5	{
	 6	  return a[0];
	 7	}
    $

Notice how at line 1, the type of the variable 'a' is "array of const
char".

Let's compile the function and emit CTF debug info:

    $ gcc -gctf -c test.c
    $

Let's see what IR libabigail emits from the CTF information:

    $ abidw --ctf --annotate test.o  | cat -n
	 1	<abi-corpus version='2.1' path='test.o' architecture='elf-amd-x86_64'>
	 2	  <elf-function-symbols>
	 3	    <!-- foo -->
	 4	    <elf-symbol name='foo' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
	 5	  </elf-function-symbols>
	 6	  <elf-variable-symbols>
	 7	    <!-- signed char -->
	 8	    <elf-symbol name='a' size='32' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
	 9	  </elf-variable-symbols>
	10	  <abi-instr address-size='64' language='LANG_C'>
	11	    <!-- char -->
	12	    <type-decl name='char' size-in-bits='8' alignment-in-bits='8' id='type-id-1'/>
	13	    <!-- const char[32] -->
	14	    <array-type-def dimensions='1' type-id='type-id-2' size-in-bits='256' alignment-in-bits='8' id='type-id-3'>
	15	      <!-- <anonymous range>[32] -->
	16	      <subrange length='32' type-id='type-id-4' id='type-id-5'/>
	17	    </array-type-def>
	18	    <!-- unsigned long int -->
	19	    <type-decl name='unsigned long int' size-in-bits='64' alignment-in-bits='64' id='type-id-4'/>
	20	    <!-- const char -->
	21	    <qualified-type-def type-id='type-id-1' const='yes' id='type-id-2'/>
	22	    <!-- const char[32] const -->
	23	    <qualified-type-def type-id='type-id-3' const='yes' id='type-id-6'/>
	24	    <!-- const char[32] const a -->
	25	    <var-decl name='a' type-id='type-id-6' mangled-name='a' visibility='default' elf-symbol-id='a'/>
	26	    <!-- char foo() -->
	27	    <function-decl name='foo' visibility='default' binding='global' size-in-bits='64' alignment-in-bits='8' elf-symbol-id='foo'>
	28	      <!-- char -->
	29	      <return type-id='type-id-1'/>
	30	    </function-decl>
	31	  </abi-instr>
	32	</abi-corpus>
    $

Notice how at line 25, the variable 'a' is described as having the
type which ID is 'type-id-6' defined at line 23.  It's a "const array
of const char".

GCC has thus added a redundant "const" qualifier to the array.

The C language specification in paragraph [6.7.3]/8 says:

    If the specification of an array type includes any type
    qualifiers, the element type is so- qualified, not the array type.

This means that a "const array of char" is the same as an "array of
const char".  So a "const array of const char" is the same an "array
of const char".

This patch performs that removal of redundant qualifier.

	* src/abg-ctf-reader.cc (maybe_strip_qualification): New
	definition.
	(process_ctf_qualified_type): Strip redundant qualifiers.
	* tests/data/test-read-ctf/test-const-array.abi: New test.
	* tests/data/test-read-ctf/test-const-array.c: Likewise.
	* tests/data/test-read-ctf/test-const-array.o: Likewise.
	* tests/Makefile.am: Add the new test material to source
	distribution.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-ctf-reader.cc                         |  70 +++++++++++++++++-
 tests/data/Makefile.am                        |   3 +
 tests/data/test-read-ctf/test-const-array.abi |  14 ++++
 tests/data/test-read-ctf/test-const-array.c   |   2 +
 tests/data/test-read-ctf/test-const-array.o   | Bin 0 -> 1416 bytes
 tests/test-read-ctf.cc                        |  11 ++-
 6 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 tests/data/test-read-ctf/test-const-array.abi
 create mode 100644 tests/data/test-read-ctf/test-const-array.c
 create mode 100644 tests/data/test-read-ctf/test-const-array.o

diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index d000556b..7615a44e 100644
--- a/src/abg-ctf-reader.cc
+++ b/src/abg-ctf-reader.cc
@@ -1251,6 +1251,65 @@ process_ctf_array_type(reader *rdr,
   return result;
 }
 
+/// Strip qualification from a qualified type, when it makes sense.
+///
+/// The C language specification says in [6.7.3]/8:
+///
+///     [If the specification of an array type includes any type
+///      qualifiers, the element type is so- qualified, not the
+///      array type.]
+///
+/// In more mundane words, a const array of int is the same as an
+/// array of const int.
+///
+/// This function thus removes the qualifiers of the array and applies
+/// them to the array element.  The function then pretends that the
+/// array itself it not qualified.
+///
+/// It might contain code to strip other cases like this in the
+/// future.
+///
+/// @param t the type to strip const qualification from.
+///
+/// @param rdr the @ref reader to use.
+///
+/// @return the stripped type or just return @p t.
+static decl_base_sptr
+maybe_strip_qualification(const qualified_type_def_sptr t)
+{
+  if (!t)
+    return t;
+
+  decl_base_sptr result = t;
+  type_base_sptr u = t->get_underlying_type();
+
+  if (is_array_type(u))
+    {
+      // Let's apply the qualifiers of the array to the array element
+      // and pretend that the array itself is not qualified, as per
+      // section [6.7.3]/8 of the C specification.
+
+      array_type_def_sptr array = is_array_type(u);
+      ABG_ASSERT(array);
+      // We should not be editing types that are already canonicalized.
+      ABG_ASSERT(!array->get_canonical_type());
+      type_base_sptr element_type = array->get_element_type();
+
+      if (qualified_type_def_sptr qualified = is_qualified_type(element_type))
+        {
+          qualified_type_def::CV quals = qualified->get_cv_quals();
+          quals |= t->get_cv_quals();
+	  // So we apply the qualifiers of the array to the array
+	  // element.
+          qualified->set_cv_quals(quals);
+	  // Let's pretend that the array is no more qualified.
+          result = is_decl(u);
+        }
+    }
+
+  return result;
+}
+
 /// Build and return a qualified type libabigail IR.
 ///
 /// @param rdr the read context.
@@ -1293,8 +1352,15 @@ process_ctf_qualified_type(reader *rdr,
   result.reset(new qualified_type_def(utype, qualifiers, location()));
   if (result)
     {
-      decl_base_sptr qualified_type_decl = get_type_declaration(result);
-      add_decl_to_scope(qualified_type_decl, tunit->get_global_scope());
+      // Strip some potentially redundant type qualifiers from
+      // the qualified type we just built.
+      decl_base_sptr d = maybe_strip_qualification(is_qualified_type(result));
+      if (!d)
+        d = get_type_declaration(result);
+      ABG_ASSERT(d);
+
+      add_decl_to_scope(d, tunit->get_global_scope());
+      result = is_type(d);
       rdr->add_type(ctf_dictionary, ctf_type, result);
     }
 
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index b89a4dd8..8d4a2b8f 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -710,6 +710,9 @@ test-read-ctf/test-bitfield.o		\
 test-read-ctf/test-bitfield-enum.abi	\
 test-read-ctf/test-bitfield-enum.c	\
 test-read-ctf/test-bitfield-enum.o	\
+test-read-ctf/test-const-array.abi	\
+test-read-ctf/test-const-array.c	\
+test-read-ctf/test-const-array.o	\
 \
 test-annotate/test0.abi			\
 test-annotate/test1.abi			\
diff --git a/tests/data/test-read-ctf/test-const-array.abi b/tests/data/test-read-ctf/test-const-array.abi
new file mode 100644
index 00000000..bd60b098
--- /dev/null
+++ b/tests/data/test-read-ctf/test-const-array.abi
@@ -0,0 +1,14 @@
+<abi-corpus version='2.1' path='data/test-read-ctf/test-const-array.o'>
+  <elf-variable-symbols>
+    <elf-symbol name='a' size='32' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+  </elf-variable-symbols>
+  <abi-instr address-size='64' language='LANG_C'>
+    <type-decl name='char' size-in-bits='8' id='type-id-1'/>
+    <array-type-def dimensions='1' type-id='type-id-2' size-in-bits='256' id='type-id-3'>
+      <subrange length='32' type-id='type-id-4' id='type-id-5'/>
+    </array-type-def>
+    <type-decl name='unsigned long int' size-in-bits='64' id='type-id-4'/>
+    <qualified-type-def type-id='type-id-1' const='yes' id='type-id-2'/>
+    <var-decl name='a' type-id='type-id-3' mangled-name='a' visibility='default' elf-symbol-id='a'/>
+  </abi-instr>
+</abi-corpus>
diff --git a/tests/data/test-read-ctf/test-const-array.c b/tests/data/test-read-ctf/test-const-array.c
new file mode 100644
index 00000000..4ffecf87
--- /dev/null
+++ b/tests/data/test-read-ctf/test-const-array.c
@@ -0,0 +1,2 @@
+/* gcc -gctf -c test-const-array.c -o test-const-array.o */
+const char a[32];
diff --git a/tests/data/test-read-ctf/test-const-array.o b/tests/data/test-read-ctf/test-const-array.o
new file mode 100644
index 0000000000000000000000000000000000000000..b50e2fe5a7f7c86b3220c08782ee1d72035cc4d3
GIT binary patch
literal 1416
zcmbtU&2H2%5T3NlQhpA!LRAG4GACL=oNPFi1B$4i91sU2?sc+eH)__YY*#cVu1LM`
zNPQEY0bT%R5__|?Rz1K-<N5o(vB&Z5x3?eOZA1}xiQqL1k<S8r-x&P7b91;4_u%)>
z(X#SrfWQcS(k-VC(4L^4qTWK?b4tG3Z%h{Pn`lTfipQM`W9pOlKCL^n(8dI2s7^B4
z6Uw>JP&aDE&XuWVN={j&EC@*E%|a$cTeL}|MOn+l=rXCRLKM|ZRCTQ6*ThO=O)?c$
zbZ)Grn8v03(nn>ZjE#lXqUELE_6B==bo7etAAkIOz$Pzw%2Sr5SvHxZhiqS7oJn0R
zq_Uzu=zGxl87kcil5oc8s+$R8lE}q$H#Z5@NAT-Uh<S*K?XG)YVIDwJ$Jd7w0Nk_-
zD+;t$_jc|*D=#c?x+dU-F~ISPfa|8ul+A+X0<Ib>IZhHAE6P(B&(t|T(+y5<TYrH6
zvyYKZGrRY)Y?d#*>rQxoPq9on|M)yQ_fB9#{xhu88~s=Q3*?49Y0I1zl<-4_*a>1N
zxBM{@2_<(x)<we-#)nuUXgWYT@5J$+2YHw7I8=Ir>T|2m%XP^$*iV?djT)Z+hvSFm
zqPt!df`weFUpfAJ2fpiYo=^WnBzI85Z%6qp*L0VH6$!`M#@agm6UUb;TyjJHU(J<n
AdH?_b

literal 0
HcmV?d00001

diff --git a/tests/test-read-ctf.cc b/tests/test-read-ctf.cc
index a0b399fc..f2d529e2 100644
--- a/tests/test-read-ctf.cc
+++ b/tests/test-read-ctf.cc
@@ -362,7 +362,16 @@ static InOutSpec in_out_specs[] =
     "data/test-read-ctf/test-bitfield-enum.abi",
     "output/test-read-ctf/test-bitfield-enum.abi",
     "--ctf",
-   },
+  },
+  {
+    "data/test-read-ctf/test-const-array.o",
+    "",
+    "",
+    SEQUENCE_TYPE_ID_STYLE,
+    "data/test-read-ctf/test-const-array.abi",
+    "output/test-read-ctf/test-const-array.abi",
+    "--ctf",
+  },
   // This should be the last entry.
   {NULL, NULL, NULL, SEQUENCE_TYPE_ID_STYLE, NULL, NULL, NULL}
 };
-- 
2.38.1


-- 
		Dodji