[RFC] PR30048 - wrong pretty representation of qualified pointers
Commit Message
Hello,
This patch is a candidate fix for the problem reported at
https://sourceware.org/bugzilla/show_bug.cgi?id=30048 and I'd be glad to
your feedback.
A qualified type is textually represented as the following:
<qualifier> <underlying-type>
were "qualifier" is the qualifier carried by the qualified type and
"underlying-type" is the type being qualified.
In this case, the qualifier prefixes the textual representation of the
qualified type.
This is true if the underlying type of the qualified type is a
non-const, non-reference type. For instance:
const int;
But when the underlying type is a pointer, then the qualified type is
represented as:
int* const;
In that later case, the qualifier comes /after/ the textual
representation of the underlying type.
Now suppose the underlying type is itself a qualified type. In that
case, for a non-const underlying type, we'd have, e.g:
const volatile int;
where the qualifier precedes the qualified type (which is itself a
qualified type) /IF/ the ultimate underlying type (a.k.a the leaf
underlying type) is itself a non-const, non-reference type.
But if the ultimate underlying type is a pointer, a qualified type
with an underlying qualified type would be textually represented as,
e.g:
int* const volatile;
In other words, if the leaf type is a pointer, the qualifier suffixes
the textual representation the underlying qualified type.
Libabigail is failing to apply this later rule.
As the type name is used as a key to cache IR nodes of DIEs (among
other things), getting it wrong can lead to a bumpy ride down the
road.
Fixed thus.
* src/abg-dwarf-reader.cc
(die_is_pointer_array_or_reference_type): Rename
die_is_pointer_or_reference_type into this. This new name
reflects more what the function does as it tests if a DIE is for
pointer, an array or a reference.
(pointer_or_qual_die_of_anonymous_class_type): Adjust to use the
newly (and better) named die_is_pointer_array_or_reference_type.
(die_is_pointer_or_reference_type): Make this really test if a DIE
is for a pointer or a reference. Now the name matches what the
function does.
(die_peel_qualified): Define new function.
(die_qualified_type_name): When a qualified name Q has another
qualified name as its underlying type, it's important to know if
the leaf type is a pointer type or not to know how to construct
the name of Q. This change now peels the potential qualifiers
from the underlying type of the qualified type to see if the leaf
type is a pointer or not.
* src/abg-ir.cc (get_name_of_qualified_type): Likewise. Also, the
name of array types doesn't follow the same rule as for pointers
and references.
* tests/data/test-abidiff-exit/PR30048-test-report-0.txt: Add new
reference test output.
* tests/data/test-abidiff-exit/PR30048-test-v{0,1}.c: Add source
code of binary input data.
* tests/data/test-abidiff-exit/PR30048-test-v{0,1}.o: Add binary
input data.
* tests/data/Makefile.am: Add the new test material above to
source distribution.
* tests/test-abidiff-exit.cc (in_out_specs): Add the input
binaries to this test harness.
* tests/data/test-abidiff-exit/qualifier-typedef-array-report-1.txt:
Adjust.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
OK to apply to mainline?
---
src/abg-dwarf-reader.cc | 65 ++++++++++++++++--
src/abg-ir.cc | 5 +-
tests/data/Makefile.am | 5 ++
.../PR30048-test-report-0.txt | 19 +++++
.../data/test-abidiff-exit/PR30048-test-v0.c | 12 ++++
.../data/test-abidiff-exit/PR30048-test-v0.o | Bin 0 -> 2784 bytes
.../data/test-abidiff-exit/PR30048-test-v1.c | 6 ++
.../data/test-abidiff-exit/PR30048-test-v1.o | Bin 0 -> 2608 bytes
.../qualifier-typedef-array-report-1.txt | 40 +++++------
tests/test-abidiff-exit.cc | 11 +++
10 files changed, 134 insertions(+), 29 deletions(-)
create mode 100644 tests/data/test-abidiff-exit/PR30048-test-report-0.txt
create mode 100644 tests/data/test-abidiff-exit/PR30048-test-v0.c
create mode 100644 tests/data/test-abidiff-exit/PR30048-test-v0.o
create mode 100644 tests/data/test-abidiff-exit/PR30048-test-v1.c
create mode 100644 tests/data/test-abidiff-exit/PR30048-test-v1.o
new file mode 100644
index 00000000..21499acc
index d45c9c6c..8172c568 100644
@@ -424,6 +424,9 @@ pointer_or_qual_die_of_anonymous_class_type(const Dwarf_Die* die);
static bool
die_is_reference_type(const Dwarf_Die* die);
+static bool
+die_is_pointer_array_or_reference_type(const Dwarf_Die* die);
+
static bool
die_is_pointer_or_reference_type(const Dwarf_Die* die);
@@ -570,6 +573,9 @@ die_function_signature(const reader& rdr,
static bool
die_peel_qual_ptr(Dwarf_Die *die, Dwarf_Die& peeled_die);
+static bool
+die_peel_qualified(Dwarf_Die *die, Dwarf_Die& peeled_die);
+
static bool
die_function_type_is_method_type(const reader& rdr,
const Dwarf_Die *die,
@@ -6912,7 +6918,7 @@ die_is_pointer_type(const Dwarf_Die* die)
static bool
pointer_or_qual_die_of_anonymous_class_type(const Dwarf_Die* die)
{
- if (!die_is_pointer_or_reference_type(die)
+ if (!die_is_pointer_array_or_reference_type(die)
&& !die_is_qualified_type(die))
return false;
@@ -6970,11 +6976,20 @@ die_is_array_type(const Dwarf_Die* die)
///
/// @return true iff @p die represents a pointer or reference type.
static bool
-die_is_pointer_or_reference_type(const Dwarf_Die* die)
+die_is_pointer_array_or_reference_type(const Dwarf_Die* die)
{return (die_is_pointer_type(die)
|| die_is_reference_type(die)
|| die_is_array_type(die));}
+/// Test if a DIE represents a pointer or a reference type.
+///
+/// @param die the die to consider.
+///
+/// @return true iff @p die represents a pointer or reference type.
+static bool
+die_is_pointer_or_reference_type(const Dwarf_Die* die)
+{return (die_is_pointer_type(die) || die_is_reference_type(die));}
+
/// Test if a DIE represents a pointer, a reference or a typedef type.
///
/// @param die the die to consider.
@@ -6983,7 +6998,7 @@ die_is_pointer_or_reference_type(const Dwarf_Die* die)
/// typedef type.
static bool
die_is_pointer_reference_or_typedef_type(const Dwarf_Die* die)
-{return (die_is_pointer_or_reference_type(die)
+{return (die_is_pointer_array_or_reference_type(die)
|| dwarf_tag(const_cast<Dwarf_Die*>(die)) == DW_TAG_typedef);}
/// Test if a DIE represents a class type.
@@ -7225,6 +7240,38 @@ die_peel_qual_ptr(Dwarf_Die *die, Dwarf_Die& peeled_die)
return true;
}
+/// Return the leaf object under a qualified type DIE.
+///
+/// @param die the DIE of the type to consider.
+///
+/// @param peeled_die out parameter. Set to the DIE of the leaf
+/// object iff the function actually peeled anything.
+///
+/// @return true upon successful completion.
+static bool
+die_peel_qualified(Dwarf_Die *die, Dwarf_Die& peeled_die)
+{
+ if (!die)
+ return false;
+
+ memcpy(&peeled_die, die, sizeof(peeled_die));
+
+ int tag = dwarf_tag(&peeled_die);
+
+ bool result = false;
+ while (tag == DW_TAG_const_type
+ || tag == DW_TAG_volatile_type
+ || tag == DW_TAG_restrict_type)
+ {
+ if (!die_die_attribute(&peeled_die, DW_AT_type, peeled_die))
+ break;
+ tag = dwarf_tag(&peeled_die);
+ result = true;
+ }
+
+ return result;
+}
+
/// Return the leaf object under a typedef type DIE.
///
/// @param die the DIE of the type to consider.
@@ -9096,9 +9143,15 @@ die_qualified_type_name(const reader& rdr,
repr.clear();
else
{
- if (has_underlying_type_die
- && die_is_pointer_or_reference_type(&underlying_type_die))
- repr = underlying_type_repr + " " + repr;
+ if (has_underlying_type_die)
+ {
+ Dwarf_Die peeled;
+ die_peel_qualified(&underlying_type_die, peeled);
+ if (die_is_pointer_or_reference_type(&peeled))
+ repr = underlying_type_repr + " " + repr;
+ else
+ repr += " " + underlying_type_repr;
+ }
else
repr += " " + underlying_type_repr;
}
@@ -8740,9 +8740,8 @@ get_name_of_qualified_type(const type_base_sptr& underlying_type,
if (!quals_repr.empty())
{
- if (is_pointer_type(underlying_type)
- || is_reference_type(underlying_type)
- || is_array_type(underlying_type))
+ if (is_pointer_type(peel_qualified_type(underlying_type))
+ || is_reference_type(peel_qualified_type(underlying_type)))
{
name += " ";
name += quals_repr;
@@ -248,6 +248,11 @@ test-abidiff-exit/btf/test0-v0.c \
test-abidiff-exit/btf/test0-v0.o \
test-abidiff-exit/btf/test0-v1.c \
test-abidiff-exit/btf/test0-v1.o \
+test-abidiff-exit/PR30048-test-report-0.txt \
+test-abidiff-exit/PR30048-test-v0.c \
+test-abidiff-exit/PR30048-test-v1.c \
+test-abidiff-exit/PR30048-test-v0.o \
+test-abidiff-exit/PR30048-test-v1.o \
\
test-diff-dwarf/test0-v0.cc \
test-diff-dwarf/test0-v0.o \
new file mode 100644
@@ -0,0 +1,19 @@
+Functions changes summary: 0 Removed, 0 Changed, 0 Added function
+Variables changes summary: 0 Removed, 1 Changed, 0 Added variable
+
+1 Changed variable:
+
+ [C] 'S s' was changed at PR30048-test-v1.c:6:1:
+ size of symbol changed from 56 to 16
+ type of variable changed:
+ type size changed from 448 to 128 (in bits)
+ 5 data member deletions:
+ 'int (int)* f01', at offset 0 (in bits) at PR30048-test-v0.c:2:1
+ 'int (const int*)* f02', at offset 64 (in bits) at PR30048-test-v0.c:3:1
+ 'int (int* const)* f03', at offset 128 (in bits) at PR30048-test-v0.c:4:1
+ 'int (int* restrict)* f04', at offset 192 (in bits) at PR30048-test-v0.c:5:1
+ 'int (const int* restrict)* f05', at offset 256 (in bits) at PR30048-test-v0.c:6:1
+ 2 data member changes:
+ 'int (int* restrict const)* f06' offset changed from 320 to 0 (in bits) (by -320 bits)
+ 'int (int* restrict const)* f07' offset changed from 384 to 64 (in bits) (by -320 bits)
+
new file mode 100644
@@ -0,0 +1,12 @@
+struct S {
+ int (*f01)(int);
+ int (*f02)(const int*);
+ int (*f03)(int* const);
+ int (*f04)(int* restrict);
+ int (*f05)(const int* restrict);
+ int (*f06)(int* restrict const);
+ int (*f07)(int* const restrict);
+};
+
+struct S s;
+
new file mode 100644
GIT binary patch
literal 2784
zcmbtU&2Jk;6o2F0b=F?eCN+hY0@)x+(+aOYY7!HaVks(Z1VU92=diZd&VoN8dsAG2
z#EnY@LI{a#4~PpFgoL>Ez=aDJE?oEr5SN}g!26A7ob6<h5Ikw$`@P@$nAw@P`_c6`
zUdstVS_ECAV^6b0e*PG@By3TM&d?Q3c1XCOmY|-2(&D~FO(ER(+0XT=HOhUC2*WyA
z-!gI;UM2fihARvk<ov<#9K)9=kA3(IH)-i{h8qmG$OWvD^)ct+v7xA7JMkzX2adpM
zGX6CK8;Zmytnz^3OI9-aBLn*<RwPj7z|TWDKZE<8mCVDAY)~YA(;T*De<Qhvn#0EI
zUnJ)+EKW&y#b?<sSRkc{)8wtYdnG=DWwPAub@7V#@_J#f)bx0@v*ObEy^`Rl?GTcj
z7o^E=Nwc&nE}klR*4l-&l_&Y^O2TqIZ`<P*9*PRjljmLYEWV3k1Knk0uk`Bq+&NzT
zX?}8Y7xJ<sN~=0M4!LxNtq|hRQ_C>?CU{u}Yk-s!Yq`_&HgM_2n{WHOwJpC^59&e9
zuUG5!TCHC5H*QB=|7Mt|K)G?*FAtNWQMA*KM$shhup3S~2RpOoW_h#Wm-}?<_SI^&
z(JUv?G%4Sy1|6y#jE7OBJMO+4SNd^M8N}_d9rwd{Q0c|9Xj(z)w9*ZeP{J}tyK%2q
zj%IQ4KdVpf<|IQmc6Yb^4L*a*|Kb4TdD*OeejI+dj&DRf0H;MLUmmt5Gc4jZJ^U*p
zL>5P~#Nux%ODrm6(cFipGD9qS>;%j6N2^?+rK!QmQ=1~BXRyflxQb|5)q%$uX*r_+
z`1D&mVemTdsT7|+jf?7FVdVcx<>{X2pBis^F4YG)x0L^)!GEg!n!(>!J_Xae!z668
zO(t`DpivUd5(+TI6tt&P3MSDY47ySKsJ|bNdSi}B9Jbq&=*}bIa1xIC(W51UcogXp
zUP43A`_k`>hr@_}m%(V9L;+7>d76Zs_mtls9R-Jz@nJMc?&{6|cYA?Lan|~Lb*GjF
zhE><IVAmOeOV0>`EvO7J>o0K*X6>Kq7jvq5`O0d8`!stZ)`DUQjG*BX<^=QC;wLRL
zHI$zza)m7@{axUX4cXzx><NCP$PeM9#LUIKnjF%9SELUoC1&pH9Lw6jt3FP>PwXGx
zlezycb7oJbsF;H$+MJt(HgoqmlC{3C*1xGLu|ED{=K6=sp=Xt!w6xUvc)zsh^|k$g
zgIV?WRQ-49VwDR@*WXjvTz^LmWCh77(RJE>!7+_fX*4hgS_RGAub9WZsO>IV9IASA
zU;6xXBKn!Nh}~E958$w@@{@J{xRb1{zJp6;IXgECZSEg^v+57j{r|A2oCCA|Z!wtY
AYXATM
literal 0
HcmV?d00001
@@ -0,0 +1,6 @@
+struct S {
+ int (*f06)(int* restrict const);
+ int (*f07)(int* const restrict);
+};
+
+struct S s;
new file mode 100644
GIT binary patch
literal 2608
zcmbtW-EI>{6h5<SCw8`h1Pdqynhk3Csk-Z*MoAhJuoM-Ds7gdtREdjpZI7L8;~#l9
zQBW)K0F@9(eSlu{0jl1V8$>R72p$5ixhUsb&)CakE1~vCGv}P|{LP*@WBK7%U*E8V
zAWed<(L~cI(I7vO>xo#W5}lx*S<aIv0J5>2$EGRwEzJp0sIxBGm1VLdEG({&4Y5G^
zJIt`Ae&CI!bNR3rlLe%3kGJA|-mE%rGZBfYnU7g2CWTnwRI_9k))vK8=gY<7W@*)7
z=i}ro7B)+Ko_X=<v65pioL!hZO_VDMyWlu$4m&%7?fH}9(wWT?#Hl<XV)_$uoNJEF
zpHsM)AjOr0%`Bl#61iEre#ZKcYoFs~;Sl^8SyW>%&$iE)3nAVdn*~F2d9kpN^w|;%
z*6~rAB+1QhzH>LKD{i&s*ZitmtJG@MYOU&CzAIbqtsqtgZ|REbb>qFBT<^%99EMF+
zgJE-L{a|(3TV8U#4sG52qEcB}^<o*t-hS0@QhBG}mE~5y^<!A>gmHN{Yy^$46NJ0v
zc6cD8GF+o_D~N+c^w`=8+ig!Cgz^83K6;v?2;JP+SaUCP53amN1JJJ=FP9g_lf(Ez
z#33+ELg`7bHJHI+&(Z6DGE6+z>5LN(o2Dj8O0ub#(k7x!Jf0b3(+5XT9!8oUy{N=C
zr#+4~ErLmp;0()?=Rq{9;(((^nocMIc={_&8MyZQD21oHv9ECSZ(7M<Iw$g{rkgG(
zA6<7#>7N_;Bc)dj{CB03A3f;CL4$cb9Pu43#quB~A45!jBZ|l$%H6<k$;Mu1JM6Xl
zY)j;z(HP48cg(>s=yl|~A-iEu>JToWMZnvM-t2d~l7F@QUO$#TPoWpZLGvf2cY1sN
zVAvnXVf;WJ{=f6H9Ex7+{_0G1Eg0OK`$e_`OV0>G>ew>KjK9P_n6-b4AJ3`EW$L(3
zGbdc@*q9!U*myU6F(;V6`q*P6yif;vtZ>wYpMDqk!3M4KGIIidj*aP;5gYHO?-pB3
z49-^-=t4=6>3f@PS?B*=Jt}=poFDJWJb#NZGbclIUxCX|Qe^sWvn{LsK-IscVo@Kz
zVrKmTW5`+Im8K`E5WX)>y1wT4zmOIGQaSvU4pzQ^bo{SMHtRpK;6+p(vGK0sH2;k)
zS{^&G@Jz+0G03LxAB^E%O!q?tnCH^{LoN{VnX$lMDew>q(>$-N`^TM_Zx8-|D)1zf
V6q)Bo-mLhi>i+*u#X>aW-vSvc)T#gg
literal 0
HcmV?d00001
@@ -47,39 +47,39 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
underlying type 'typedef A' at qualifier-typedef-array-v0.c:1:1 changed:
entity changed from 'typedef A' to compatible type 'void*[7]'
type of 'C v_c' changed:
- entity changed from 'typedef C' to compatible type 'const volatile void*[7]'
+ entity changed from 'typedef C' to compatible type 'void* const volatile[7]'
array element type 'void* const' changed:
- 'void* const' changed to 'const volatile void*'
- type name changed from 'void* const[7]' to 'const volatile void*[7]'
+ 'void* const' changed to 'void* const volatile'
+ type name changed from 'void* const[7]' to 'void* const volatile[7]'
type size hasn't changed
type of 'C r_c' changed:
- entity changed from 'typedef C' to compatible type 'restrict const void*[7]'
+ entity changed from 'typedef C' to compatible type 'void* restrict const[7]'
array element type 'void* const' changed:
- 'void* const' changed to 'restrict const void*'
- type name changed from 'void* const[7]' to 'restrict const void*[7]'
+ 'void* const' changed to 'void* restrict const'
+ type name changed from 'void* const[7]' to 'void* restrict const[7]'
type size hasn't changed
type of 'D v_d' changed:
- entity changed from 'typedef D' to compatible type 'const volatile void*[7]'
+ entity changed from 'typedef D' to compatible type 'void* const volatile[7]'
array element type 'void* const' changed:
- 'void* const' changed to 'const volatile void*'
- type name changed from 'void* const[7]' to 'const volatile void*[7]'
+ 'void* const' changed to 'void* const volatile'
+ type name changed from 'void* const[7]' to 'void* const volatile[7]'
type size hasn't changed
type of 'D r_d' changed:
- entity changed from 'typedef D' to compatible type 'restrict const void*[7]'
+ entity changed from 'typedef D' to compatible type 'void* restrict const[7]'
array element type 'void* const' changed:
- 'void* const' changed to 'restrict const void*'
- type name changed from 'void* const[7]' to 'restrict const void*[7]'
+ 'void* const' changed to 'void* restrict const'
+ type name changed from 'void* const[7]' to 'void* restrict const[7]'
type size hasn't changed
type of 'E r_e' changed:
- entity changed from 'typedef E' to compatible type 'restrict const volatile void*[7]'
- array element type 'const volatile void*' changed:
- 'const volatile void*' changed to 'restrict const volatile void*'
- type name changed from 'const volatile void*[7]' to 'restrict const volatile void*[7]'
+ entity changed from 'typedef E' to compatible type 'void* restrict const volatile[7]'
+ array element type 'void* const volatile' changed:
+ 'void* const volatile' changed to 'void* restrict const volatile'
+ type name changed from 'void* const volatile[7]' to 'void* restrict const volatile[7]'
type size hasn't changed
type of 'F r_f' changed:
- entity changed from 'typedef F' to compatible type 'restrict const volatile void*[7]'
- array element type 'const volatile void*' changed:
- 'const volatile void*' changed to 'restrict const volatile void*'
- type name changed from 'const volatile void*[7]' to 'restrict const volatile void*[7]'
+ entity changed from 'typedef F' to compatible type 'void* restrict const volatile[7]'
+ array element type 'void* const volatile' changed:
+ 'void* const volatile' changed to 'void* restrict const volatile'
+ type name changed from 'void* const volatile[7]' to 'void* restrict const volatile[7]'
type size hasn't changed
@@ -471,6 +471,17 @@ InOutSpec in_out_specs[] =
"data/test-abidiff-exit/test-rhbz2114909-report-1.txt",
"output/test-abidiff-exit/test-rhbz2114909-report-1.txt"
},
+ {
+ "data/test-abidiff-exit/PR30048-test-v0.o",
+ "data/test-abidiff-exit/PR30048-test-v1.o",
+ "",
+ "",
+ "",
+ "--no-default-suppression",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+ "data/test-abidiff-exit/PR30048-test-report-0.txt",
+ "output/test-abidiff-exit/PR30048-test-report-0.txt"
+ },
#ifdef WITH_BTF
{
"data/test-abidiff-exit/btf/test0-v0.o",