From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [IPv6:2001:4b98:dc4:8::230]) by sourceware.org (Postfix) with ESMTPS id 6B1643858D37 for ; Wed, 30 Nov 2022 09:33:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6B1643858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by mail.gandi.net (Postfix) with ESMTPSA id C626B24000B; Wed, 30 Nov 2022 09:33:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1669800794; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6JcBHxsGpFSbW37x8/ReYe9jfnKR5jogaBp2dYisQeM=; b=Knv8yIOEwF1I2pehspp1apqsFQR7XzlySjdOy6sW5cqrL6sQVVwReizd55q4gkvCk2ateP qLo/qI+GEYkOSflPUgEoEkMAIp6tXi5WjLtCNn3Hl7XqW3mIDJ302eMZVJQt3uCVJxWOz6 JpZXnM5x4FGUAF3RWjEJL4uBiS4U1yvqhzoqcP/Qq3MDFTFQUpJUjN9Xv6pUhqF+aFXrwc q/5iFup7MoIywVYe+Wngs3jO+EHViON2Puerr1ozUfcK3TbYhJ67Ud62Yo6rzsEogHktyK vyYaHkhLCyG2xEOWOtyNCbLrWxShmHxrvVb0ajS2/+p6VHnuluYrW+6H9LjF0w== Received: by localhost (Postfix, from userid 1000) id 1D81F581C59; Wed, 30 Nov 2022 10:33:12 +0100 (CET) From: Dodji Seketeli To: "Guillermo E. Martinez via Libabigail" Cc: "Guillermo E. Martinez" Subject: Re: [PATCH 3/5] ctf-front-end: Strip qualification from a qualified array type Organization: Me, myself and I References: <20221117034305.184864-1-guillermo.e.martinez@oracle.com> <20221117034305.184864-4-guillermo.e.martinez@oracle.com> X-Operating-System: Fedora 38 X-URL: http://www.seketeli.net/~dodji Date: Wed, 30 Nov 2022 10:33:12 +0100 In-Reply-To: <20221117034305.184864-4-guillermo.e.martinez@oracle.com> (Guillermo E. Martinez via Libabigail's message of "Wed, 16 Nov 2022 21:43:03 -0600") Message-ID: <87h6ygokh3.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hello Guillermo, "Guillermo E. Martinez via Libabigail" a =C3=A9crit: 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]; > > > > > 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 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 $ 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 =3D t; > + type_base_sptr u =3D 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 =3D 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 =3D array->get_element_type(); > + > + if (qualified_type_def_sptr qualified =3D is_qualified_type(elemen= t_type)) > + { > + qualified_type_def::CV quals =3D qualified->get_cv_quals(); > + quals |=3D 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 =3D 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" 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 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 $ 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 Signed-off-by: Dodji Seketeli --- 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; } =20 +/// 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 =3D t; + type_base_sptr u =3D 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 =3D 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 =3D array->get_element_type(); + + if (qualified_type_def_sptr qualified =3D is_qualified_type(element_= type)) + { + qualified_type_def::CV quals =3D qualified->get_cv_quals(); + quals |=3D 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 =3D 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 =3D 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 =3D maybe_strip_qualification(is_qualified_type(res= ult)); + if (!d) + d =3D get_type_declaration(result); + ABG_ASSERT(d); + + add_decl_to_scope(d, tunit->get_global_scope()); + result =3D is_type(d); rdr->add_type(ctf_dictionary, ctf_type, result); } =20 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/tes= t-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 @@ + + + + + + + + + + + + + + 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..b50e2fe5a7f7c86b3220c08782e= e1d72035cc4d3 GIT binary patch literal 1416 zcmbtU&2H2%5T3NlQhpA!LRAG4GACL=3DoNPFi1B$4i91sU2?sc+eH)__YY*#cVu1LM` zNPQEY0bT%R5__|?Rz1K- z(X#SrfWQcS(k-VC(4L^4qTWK?b4tG3Z%h{Pn`lTfipQM`W9pOlKCL^n(8dI2s7^B4 z6Uw>JP&aDE&XuWVN=3D{j&EC@*E%|a$cTeL}|MOn+l=3DrXCRLKM|ZRCTQ6*ThO=3DO)?c$ zbZ)Grn8v03(nn>ZjE#lXqUELE_6B=3D=3Dbo7etAAkIOz$Pzw%2Sr5SvHxZhiqS7oJn0R zq_Uzu=3DzGxl87kcil5oc8s+$R8lE}q$H#Z5@NAT-UhIZhHAE6P(B&(t|T(+y5rQxoPq9on|M)yQ_fB9#{xhu88~s=3DQ3*?49Y0I1zl<-4_*a>1N zxBM{@2_<(x)T|2m%XP^$*iV?djT)Z+hvSFm zqPt!df`weFUpfAJ2fpiYo=3D^WnBzI85Z%6qp*L0VH6$!`M#@agm6UUb;TyjJHU(J