* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
@ 2023-01-25 9:57 ` gprocida at google dot com
2023-01-27 1:10 ` woodard at redhat dot com
` (7 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: gprocida at google dot com @ 2023-01-25 9:57 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #1 from gprocida at google dot com ---
The examples here include "useless" qualifiers on parameter types and removing
them consistently may make some of the issues disappear.
It may be possible to construct examples where this isn't the case, but I
haven't tried.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
2023-01-25 9:57 ` [Bug default/30048] " gprocida at google dot com
@ 2023-01-27 1:10 ` woodard at redhat dot com
2023-01-27 10:05 ` gprocida at google dot com
` (6 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: woodard at redhat dot com @ 2023-01-27 1:10 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
Ben Woodard <woodard at redhat dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |woodard at redhat dot com
--- Comment #2 from Ben Woodard <woodard at redhat dot com> ---
I would call this one of many DWARF idiom issues. At its base libabigail uses
string comparisons in many cases rather than abstractions of the semantic
information. This can cause problems like this when the expression in the DWARF
is derived the source code and two literally different strings represent
semantically identical items.
I have filed several of these but we haven't gotten to looking at them yet.
Here are some examples:
https://sourceware.org/bugzilla/show_bug.cgi?id=28641
https://sourceware.org/bugzilla/show_bug.cgi?id=28664
https://sourceware.org/bugzilla/show_bug.cgi?id=28675
I find a lot of things like this when trying to test compatibility between
things which are built with different compilers.
Something like:
abidiff lib_built_with_gcc.so same_lib_built_with_clang.so
The last time that I talked to Dodji about these he suggested either fix the
string comparison or putting in special case code that handles these cases
after the string comparison fails.
The tricky thing is really knowing what are actually ABI artifacts and what are
not. The order of restrict in relation to const doesn't matter but is the fact
that restrict included in the declaration actually an ABI artifact? It wouldn't
affect the calling convention but the codegen could be different if restrict
were not included in the decl. So we don't want to just ignore restrict. What
if a library added 'restrict' to an exported function's parameter. Previous
users of the library may not have been expecting that requirement. Is that
actually an ABI change?
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
2023-01-25 9:57 ` [Bug default/30048] " gprocida at google dot com
2023-01-27 1:10 ` woodard at redhat dot com
@ 2023-01-27 10:05 ` gprocida at google dot com
2023-01-27 10:10 ` gprocida at google dot com
` (5 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: gprocida at google dot com @ 2023-01-27 10:05 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #3 from gprocida at google dot com ---
Function prototypes that only differ in restrict are compatible according to
the standard.
This means that foo.c containing a declaration of f with restrict and foo.h
containing one without restrict is legal.
The type exposed to users may or may not contain restrict ("not", in the case
above). Tooling such as abidw could warn about this.
However, the C language itself makes it clear that qualifiers on function
parameter types only have an effect on the implementation, not the interface.
I'm in the camp that we should just discard them all. I don't think patches for
Linux that try to drop or add const in various places where this discrepancy
already exists would be well received by kernel maintainers.
Dropping them means dropping restrict almost everywhere. I suppose restrict
could appear in standalone function types... but that is even less useful.
For the STG tool, I'm in fact proposing dropping qualifiers on function
parameter types and return types and dropping restrict unconditionally.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (2 preceding siblings ...)
2023-01-27 10:05 ` gprocida at google dot com
@ 2023-01-27 10:10 ` gprocida at google dot com
2023-02-03 0:11 ` dodji at redhat dot com
` (4 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: gprocida at google dot com @ 2023-01-27 10:10 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #4 from gprocida at google dot com ---
We recently patched libabigail locally so that it doesn't transform const void
to void (which misrepresents the type of memcpy, for example).
This seemed like an obvious self-contained fix... but, it seems it may have
exposed other issues relating to qualifiers. There may be some overlap with
that and issue here and the ones you linked to.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (3 preceding siblings ...)
2023-01-27 10:10 ` gprocida at google dot com
@ 2023-02-03 0:11 ` dodji at redhat dot com
2023-02-07 17:37 ` dodji at redhat dot com
` (3 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: dodji at redhat dot com @ 2023-02-03 0:11 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
dodji at redhat dot com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |ASSIGNED
Last reconfirmed| |2023-02-03
Ever confirmed|0 |1
--- Comment #5 from dodji at redhat dot com ---
I could reproduce the issue and I am looking into it. Thank you for reporting
it.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (4 preceding siblings ...)
2023-02-03 0:11 ` dodji at redhat dot com
@ 2023-02-07 17:37 ` dodji at redhat dot com
2023-02-08 13:08 ` gprocida at google dot com
` (2 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: dodji at redhat dot com @ 2023-02-07 17:37 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #6 from dodji at redhat dot com ---
I have sent a candidate patch for issue to the mailing list at
https://inbox.sourceware.org/libabigail/871qn1nzsm.fsf@redhat.com/.
I'd be glad to have your feedback about it.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (5 preceding siblings ...)
2023-02-07 17:37 ` dodji at redhat dot com
@ 2023-02-08 13:08 ` gprocida at google dot com
2023-02-10 12:50 ` Dodji Seketeli
2023-02-10 12:50 ` dodji at seketeli dot org
2023-02-10 12:50 ` dodji at redhat dot com
8 siblings, 1 reply; 11+ messages in thread
From: gprocida at google dot com @ 2023-02-08 13:08 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #7 from gprocida at google dot com ---
Hi Dodji.
I don't feel qualified to review the code change and I haven't had the chance
to patch and test it (but I'm sure you have!).
However, I can offer some test case suggestions.
Firstly, printing out C types is a hard problem! I worked on an implementation
that seems to have mostly survived over a year or so of use:
https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.h
https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.cc
The important code here is Name, which consolidates precedence handling, and
the Describe function object, which works recursively on the type graph. I
documented the original design approach here:
https://android.googlesource.com/platform/external/stg/+/refs/heads/master/doc/NAMES.md
I think the main takeaway is that qualifiers can be "tacked on" without
disturbing the rest of the code too much but some contextual awareness is
needed to make sure the qualifiers end up on the correct side with appropriate
whitespace. It would have been significantly easier if the world (or C itself)
just had "int const" and not "const int".
In terms of testing... this is my torture test for naming and diffs. The
comments are (or at least were at one point) valid input for cdecl(1).
Please feel free to use or adapt this for libabigail. If you need copyright /
licence attribution it's: Google Inc. 2020, LLVM licence (Apache + LLVM
exception), same as libabigail.
struct amusement {
// declare A as array 7 of int
int A[7];
// declare B as pointer to int
int *B;
// declare C as pointer to function (void) returning int
int (*C)(void );
// declare D as array 7 of array 7 of int
int D[7][7];
// declare E as array 7 of pointer to int
int *E[7];
// declare F as array 7 of pointer to function (void) returning int
int (*F[7])(void );
// declare G as pointer to array 7 of int
int (*G)[7];
// declare H as pointer to pointer to int
int **H;
// declare I as pointer to function (void) returning int
int (*I)(void );
// declare J as pointer to function (void) returning pointer to array 7 of
int
int (*(*J)(void ))[7];
// declare K as pointer to function (void) returning pointer to int
int *(*K)(void );
// declare L as pointer to function (void) returning pointer to function
(void) returning int
int (*(*L)(void ))(void );
// declare a as array 7 of volatile int
volatile int a[7];
// declare b as const pointer to volatile int
volatile int * const b;
// declare c as const pointer to function (void) returning int
int (* const c)(void );
// declare d as array 7 of array 7 of volatile int
volatile int d[7][7];
// declare e as array 7 of const pointer to volatile int
volatile int * const e[7];
// declare f as array 7 of const pointer to function (void) returning int
int (* const f[7])(void );
// declare g as const pointer to array 7 of volatile int
volatile int (* const g)[7];
// declare h as const pointer to const pointer to volatile int
volatile int * const * const h;
// declare i as const pointer to function (void) returning int
int (* const i)(void );
// declare j as const pointer to function (void) returning pointer to array 7
of volatile int
volatile int (*(* const j)(void ))[7];
// declare k as const pointer to function (void) returning pointer to
volatile int
volatile int *(* const k)(void );
// declare l as const pointer to function (void) returning pointer to
function (void) returning int
int (*(* const l)(void ))(void );
};
struct amusement * fun(void) { return 0; }
// declare M as function (void) returning int
int M(void ) { return 0; }
// declare N as function (void) returning pointer to array 7 of int
int (*N(void ))[7] { return 0; }
// declare O as function (void) returning pointer to int
int *O(void ) { return 0; }
// declare P as function (void) returning pointer to function (void) returning
int
int (*P(void ))(void ) { return 0; }
// declare m as function (void) returning int
int m(void ) { return 0; }
// declare n as function (void) returning pointer to array 7 of volatile int
volatile int (*n(void ))[7] { return 0; }
// declare o as function (void) returning pointer to volatile int
volatile int *o(void ) { return 0; }
// declare p as function (void) returning pointer to function (void) returning
int
int (*p(void ))(void ) { return 0; }
This can be abidiff'd against:
struct amusement {
int A;
int B;
int C;
int D;
int E;
int F;
int G;
int H;
int I;
int J;
int K;
int L;
int a;
int b;
int c;
int d;
int e;
int f;
int g;
int h;
int i;
int j;
int k;
int l;
};
struct amusement * fun() { return 0; }
int M() { return 0; }
int N() { return 0; }
int O() { return 0; }
int P() { return 0; }
int m() { return 0; }
int n() { return 0; }
int o() { return 0; }
int p() { return 0; }
Regards,
Giuliano.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Bug default/30048] incorrect qualifiers within compound types
2023-02-08 13:08 ` gprocida at google dot com
@ 2023-02-10 12:50 ` Dodji Seketeli
0 siblings, 0 replies; 11+ messages in thread
From: Dodji Seketeli @ 2023-02-10 12:50 UTC (permalink / raw)
To: gprocida at google dot com via Libabigail; +Cc: gprocida at google dot com
Hello Giuliano,
gprocida at google dot com via Libabigail <libabigail@sourceware.org> a
écrit:
> I don't feel qualified to review the code change
OK, no problem, I understand.
> and I haven't had the chance to patch and test it (but I'm sure you
> have!).
No problem. And yes, I have tested it against the reproducer of this
problem report.
> However, I can offer some test case suggestions.
>
> Firstly, printing out C types is a hard problem!
Heh. Yeah, I know. Libabigail's implementation is a bit different
from the standard C or C++ one, even if it's very close. There are
things that are simplified so that it looks easier to understand. Also,
by default, it could be look not too much "alien" for compilers of other
languages that might have produced the type info we are looking at.
> I worked on an implementation
> that seems to have mostly survived over a year or so of use:
>
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.h
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.cc
>
> The important code here is Name, which consolidates precedence handling, and
> the Describe function object, which works recursively on the type graph. I
> documented the original design approach here:
>
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/doc/NAMES.md
>
> I think the main takeaway is that qualifiers can be "tacked on" without
> disturbing the rest of the code too much but some contextual awareness is
> needed to make sure the qualifiers end up on the correct side with appropriate
> whitespace. It would have been significantly easier if the world (or C itself)
> just had "int const" and not "const int".
Thanks!
> In terms of testing... this is my torture test for naming and diffs. The
> comments are (or at least were at one point) valid input for cdecl(1).
>
> Please feel free to use or adapt this for libabigail. If you need copyright /
> licence attribution it's: Google Inc. 2020, LLVM licence (Apache + LLVM
> exception), same as libabigail.
[...]
Thanks! I have added this test to the libabigail test suite. After
(hopefully carefully) starring at the report of abidiff on it, it looks
like the output is what I would expect, with the patch I proposed.
I thus went ahead and applied the patch to the master branch at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=03d8b52fc7a510edaf3182a3600cc54c0a180fe7.
Thanks!
--
Dodji
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (6 preceding siblings ...)
2023-02-08 13:08 ` gprocida at google dot com
@ 2023-02-10 12:50 ` dodji at seketeli dot org
2023-02-10 12:50 ` dodji at redhat dot com
8 siblings, 0 replies; 11+ messages in thread
From: dodji at seketeli dot org @ 2023-02-10 12:50 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
--- Comment #8 from dodji at seketeli dot org ---
Hello Giuliano,
gprocida at google dot com via Libabigail <libabigail@sourceware.org> a
écrit:
> I don't feel qualified to review the code change
OK, no problem, I understand.
> and I haven't had the chance to patch and test it (but I'm sure you
> have!).
No problem. And yes, I have tested it against the reproducer of this
problem report.
> However, I can offer some test case suggestions.
>
> Firstly, printing out C types is a hard problem!
Heh. Yeah, I know. Libabigail's implementation is a bit different
from the standard C or C++ one, even if it's very close. There are
things that are simplified so that it looks easier to understand. Also,
by default, it could be look not too much "alien" for compilers of other
languages that might have produced the type info we are looking at.
> I worked on an implementation
> that seems to have mostly survived over a year or so of use:
>
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.h
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/naming.cc
>
> The important code here is Name, which consolidates precedence handling, and
> the Describe function object, which works recursively on the type graph. I
> documented the original design approach here:
>
> https://android.googlesource.com/platform/external/stg/+/refs/heads/master/doc/NAMES.md
>
> I think the main takeaway is that qualifiers can be "tacked on" without
> disturbing the rest of the code too much but some contextual awareness is
> needed to make sure the qualifiers end up on the correct side with appropriate
> whitespace. It would have been significantly easier if the world (or C itself)
> just had "int const" and not "const int".
Thanks!
> In terms of testing... this is my torture test for naming and diffs. The
> comments are (or at least were at one point) valid input for cdecl(1).
>
> Please feel free to use or adapt this for libabigail. If you need copyright /
> licence attribution it's: Google Inc. 2020, LLVM licence (Apache + LLVM
> exception), same as libabigail.
[...]
Thanks! I have added this test to the libabigail test suite. After
(hopefully carefully) starring at the report of abidiff on it, it looks
like the output is what I would expect, with the patch I proposed.
I thus went ahead and applied the patch to the master branch at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=03d8b52fc7a510edaf3182a3600cc54c0a180fe7.
Thanks!
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread* [Bug default/30048] incorrect qualifiers within compound types
2023-01-25 9:40 [Bug default/30048] New: incorrect qualifiers within compound types gprocida at google dot com
` (7 preceding siblings ...)
2023-02-10 12:50 ` dodji at seketeli dot org
@ 2023-02-10 12:50 ` dodji at redhat dot com
8 siblings, 0 replies; 11+ messages in thread
From: dodji at redhat dot com @ 2023-02-10 12:50 UTC (permalink / raw)
To: libabigail
https://sourceware.org/bugzilla/show_bug.cgi?id=30048
dodji at redhat dot com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution|--- |FIXED
--- Comment #9 from dodji at redhat dot com ---
Closing the bug.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply [flat|nested] 11+ messages in thread