[Bug default/30048] New: incorrect qualifiers within compound types

[Bug default/30048] New: incorrect qualifiers within compound types

[gprocida at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30048

            Bug ID: 30048
           Summary: incorrect qualifiers within compound types
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

This appears to be some kind of equality or canonicalisation issue.

The types of the f06 and f07 members should be the same in the two ABIs
extracted. Here I've compiled with GCC (and checked the DWARF independently
anyway).

==> small.c <==
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;

==> smaller.c <==
struct S {
  int (*f06)(int* restrict const);
  int (*f07)(int* const restrict);
};

struct S s;

$ abidw --no-show-locs --no-comp-dir-path --no-corpus-path small.o
<abi-corpus version='2.1' architecture='elf-amd-x86_64'>
  <elf-variable-symbols>
    <elf-symbol name='s' size='56' type='object-type' binding='global-binding'
visibility='default-visibility' is-defined='yes'/>
  </elf-variable-symbols>
  <abi-instr address-size='64' path='/tmp/small.c' language='LANG_C11'>
    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
    <class-decl name='S' size-in-bits='448' is-struct='yes'
visibility='default' id='type-id-2'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='f01' type-id='type-id-3' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='64'>
        <var-decl name='f02' type-id='type-id-4' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='128'>
        <var-decl name='f03' type-id='type-id-5' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='192'>
        <var-decl name='f04' type-id='type-id-6' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='256'>
        <var-decl name='f05' type-id='type-id-7' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='320'>
        <var-decl name='f06' type-id='type-id-7' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='384'>
        <var-decl name='f07' type-id='type-id-7' visibility='default'/>
      </data-member>
    </class-decl>
    <qualified-type-def type-id='type-id-1' const='yes' id='type-id-8'/>
    <pointer-type-def type-id='type-id-8' size-in-bits='64' id='type-id-9'/>
    <qualified-type-def type-id='type-id-9' restrict='yes' id='type-id-10'/>
    <pointer-type-def type-id='type-id-11' size-in-bits='64' id='type-id-7'/>
    <pointer-type-def type-id='type-id-12' size-in-bits='64' id='type-id-4'/>
    <pointer-type-def type-id='type-id-13' size-in-bits='64' id='type-id-3'/>
    <pointer-type-def type-id='type-id-14' size-in-bits='64' id='type-id-5'/>
    <pointer-type-def type-id='type-id-15' size-in-bits='64' id='type-id-6'/>
    <pointer-type-def type-id='type-id-1' size-in-bits='64' id='type-id-16'/>
    <qualified-type-def type-id='type-id-16' const='yes' id='type-id-17'/>
    <qualified-type-def type-id='type-id-16' restrict='yes' id='type-id-18'/>
    <var-decl name='s' type-id='type-id-2' mangled-name='s'
visibility='default' elf-symbol-id='s'/>
    <function-type size-in-bits='64' id='type-id-11'>
      <parameter type-id='type-id-10'/>
      <return type-id='type-id-1'/>
    </function-type>
    <function-type size-in-bits='64' id='type-id-12'>
      <parameter type-id='type-id-9'/>
      <return type-id='type-id-1'/>
    </function-type>
    <function-type size-in-bits='64' id='type-id-13'>
      <parameter type-id='type-id-1'/>
      <return type-id='type-id-1'/>
    </function-type>
    <function-type size-in-bits='64' id='type-id-14'>
      <parameter type-id='type-id-17'/>
      <return type-id='type-id-1'/>
    </function-type>
    <function-type size-in-bits='64' id='type-id-15'>
      <parameter type-id='type-id-18'/>
      <return type-id='type-id-1'/>
    </function-type>
  </abi-instr>
</abi-corpus>

abidw --no-show-locs --no-comp-dir-path --no-corpus-path smaller.o
<abi-corpus version='2.1' architecture='elf-amd-x86_64'>
  <elf-variable-symbols>
    <elf-symbol name='s' size='16' type='object-type' binding='global-binding'
visibility='default-visibility' is-defined='yes'/>
  </elf-variable-symbols>
  <abi-instr address-size='64' path='/tmp/smaller.c' language='LANG_C11'>
    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
    <class-decl name='S' size-in-bits='128' is-struct='yes'
visibility='default' id='type-id-2'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='f06' type-id='type-id-3' visibility='default'/>
      </data-member>
      <data-member access='public' layout-offset-in-bits='64'>
        <var-decl name='f07' type-id='type-id-3' visibility='default'/>
      </data-member>
    </class-decl>
    <pointer-type-def type-id='type-id-4' size-in-bits='64' id='type-id-3'/>
    <pointer-type-def type-id='type-id-1' size-in-bits='64' id='type-id-5'/>
    <qualified-type-def type-id='type-id-5' const='yes' id='type-id-6'/>
    <qualified-type-def type-id='type-id-6' restrict='yes' id='type-id-7'/>
    <var-decl name='s' type-id='type-id-2' mangled-name='s'
visibility='default' elf-symbol-id='s'/>
    <function-type size-in-bits='64' id='type-id-4'>
      <parameter type-id='type-id-7'/>
      <return type-id='type-id-1'/>
    </function-type>
  </abi-instr>
</abi-corpus>

abidiff reports (whether directly on the .o or the .xml):

        type of 'int (restrict int* const)* f06' changed:
          in pointed to type 'function type int (restrict int* const)':
            parameter 1 of type 'restrict int* const' changed:
              'restrict int* const' changed to 'const int* restrict'

but fails to say the same thing about f07.

abidiff --leaf-changes-only doesn't mention the type diffs at all.

This might be up to 3 different issues (or there could be fewer root causes):

- DWARF reader / canonicalisation getting some types wrong or confused
- abidiff should also report that f07's type has changed, even if it has
changed in exactly the same way
- abidiff --leaf-changes-only should report that f06 and f07 types have
changed, as well as their offsets

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/30048] incorrect qualifiers within compound types

[gprocida at google dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[woodard at redhat dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[gprocida at google dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[gprocida at google dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[dodji at redhat dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[dodji at redhat dot com]

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.

[Bug default/30048] incorrect qualifiers within compound types

[gprocida at google dot com]

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.

Re: [Bug default/30048] incorrect qualifiers within compound types

[Dodji Seketeli]

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

[Bug default/30048] incorrect qualifiers within compound types

[dodji at seketeli dot org]

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.

[Bug default/30048] incorrect qualifiers within compound types

[dodji at redhat dot com]

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.