BZ #19590: Fixed build of shared objects that use libmvec.so functions

  Hi,

here the fix for BZ #19590.



--
WBR,
Andrew
  

On 02/10/2016 12:36 PM, Andrew Senkevich wrote:

>  #define ALIAS_IMPL(alias, target) \
>  ENTRY (alias); \
> -       call target; \
> +       call target@PLT; \
>         ret; \
>  END (alias)

This needs a comment why this cannot be a tail call (JMP).

Out of curiosity, why is it not possible to avoid this indirection?  Why
is this logic required in libmvec_nonshared.a?

Florian
  

On Wed, Feb 10, 2016 at 3:36 AM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> Hi,
>
> here the fix for BZ #19590.
>
>
> diff --git a/ChangeLog b/ChangeLog
> index 11c3156..6ebcefb 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2016-02-10  Andrew Senkevich  <andrew.senkevich@intel.com>
> +           Carlos O'Donell  <carlos@redhat.com>
> +
> +       [BZ #19590]
> +       * sysdeps/x86_64/fpu/svml_finite_alias.S (ALIAS_IMPL): Use PLT.
> +
>  2016-02-04  Rajalakshmi Srinivasaraghavan  <raji@linux.vnet.ibm.com>
>
>         * sysdeps/powerpc/fpu/libm-test-ulps: Regenerated.
> diff --git a/sysdeps/x86_64/fpu/svml_finite_alias.S
> b/sysdeps/x86_64/fpu/svml_finite_alias.S
> index 0062fe4..0da3b64 100644
> --- a/sysdeps/x86_64/fpu/svml_finite_alias.S
> +++ b/sysdeps/x86_64/fpu/svml_finite_alias.S
> @@ -23,7 +23,7 @@
>
>  #define ALIAS_IMPL(alias, target) \
>  ENTRY (alias); \
> -       call target; \
> +       call target@PLT; \
>         ret; \
>  END (alias)
>
>

Need a testcase,   You can use assembly code in testcase to make it
compiler independent.
  

2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>
>>  #define ALIAS_IMPL(alias, target) \
>>  ENTRY (alias); \
>> -       call target; \
>> +       call target@PLT; \
>>         ret; \
>>  END (alias)
>
> This needs a comment why this cannot be a tail call (JMP).

Yes, it should be jmp.

> Out of curiosity, why is it not possible to avoid this indirection?  Why
> is this logic required in libmvec_nonshared.a?

It is workaround to exclude unnecessary symbol aliases in libmvec.so
(BZ #19058).


--
WBR,
Andrew
  

On 02/10/2016 01:58 PM, Andrew Senkevich wrote:
> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>
>>>  #define ALIAS_IMPL(alias, target) \
>>>  ENTRY (alias); \
>>> -       call target; \
>>> +       call target@PLT; \
>>>         ret; \
>>>  END (alias)
>>
>> This needs a comment why this cannot be a tail call (JMP).
> 
> Yes, it should be jmp.

Fine, then you can drop the RET (which was present in Carlos' patch).

>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>> is this logic required in libmvec_nonshared.a?
> 
> It is workaround to exclude unnecessary symbol aliases in libmvec.so
> (BZ #19058).

Ugh.  This is a GCC bug, or a lack of a GCC feature.  It needs fixing in
GCC.  Apparently, this hasn't happened for GCC 6.  Is there lack of
agreement that this is a compiler issue?

At least libmvec_shared.a doesn't have an ABI impact.  But this approach
is very odd.

Florian
  

On Wed, Feb 10, 2016 at 4:58 AM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>
>>>  #define ALIAS_IMPL(alias, target) \
>>>  ENTRY (alias); \
>>> -       call target; \
>>> +       call target@PLT; \
>>>         ret; \
>>>  END (alias)
>>
>> This needs a comment why this cannot be a tail call (JMP).
>
> Yes, it should be jmp.
>
>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>> is this logic required in libmvec_nonshared.a?
>
> It is workaround to exclude unnecessary symbol aliases in libmvec.so
> (BZ #19058).
>

Can you fold  libmvec_nonshared.a into libmvec,so?
  

2016-02-10 16:15 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Wed, Feb 10, 2016 at 4:58 AM, Andrew Senkevich
> <andrew.n.senkevich@gmail.com> wrote:
>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>
>>>>  #define ALIAS_IMPL(alias, target) \
>>>>  ENTRY (alias); \
>>>> -       call target; \
>>>> +       call target@PLT; \
>>>>         ret; \
>>>>  END (alias)
>>>
>>> This needs a comment why this cannot be a tail call (JMP).
>>
>> Yes, it should be jmp.
>>
>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>> is this logic required in libmvec_nonshared.a?
>>
>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>> (BZ #19058).
>>
>
> Can you fold  libmvec_nonshared.a into libmvec,so?

It is possible but I am not sure it is Ok with point that unnecessary
ABI extension should be limited..


--
WBR,
Andrew
  

On Wed, Feb 10, 2016 at 8:07 AM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> 2016-02-10 16:15 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Wed, Feb 10, 2016 at 4:58 AM, Andrew Senkevich
>> <andrew.n.senkevich@gmail.com> wrote:
>>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>>
>>>>>  #define ALIAS_IMPL(alias, target) \
>>>>>  ENTRY (alias); \
>>>>> -       call target; \
>>>>> +       call target@PLT; \
>>>>>         ret; \
>>>>>  END (alias)
>>>>
>>>> This needs a comment why this cannot be a tail call (JMP).
>>>
>>> Yes, it should be jmp.
>>>
>>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>>> is this logic required in libmvec_nonshared.a?
>>>
>>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>>> (BZ #19058).
>>>
>>
>> Can you fold  libmvec_nonshared.a into libmvec,so?
>
> It is possible but I am not sure it is Ok with point that unnecessary
> ABI extension should be limited..
>

Should the workaround be in GCC?  Otherwise, it will be used
even when compiler works fine.
  

2016-02-10 16:07 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
> On 02/10/2016 01:58 PM, Andrew Senkevich wrote:
>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>
>>>>  #define ALIAS_IMPL(alias, target) \
>>>>  ENTRY (alias); \
>>>> -       call target; \
>>>> +       call target@PLT; \
>>>>         ret; \
>>>>  END (alias)
>>>
>>> This needs a comment why this cannot be a tail call (JMP).
>>
>> Yes, it should be jmp.
>
> Fine, then you can drop the RET (which was present in Carlos' patch).
>
>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>> is this logic required in libmvec_nonshared.a?
>>
>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>> (BZ #19058).
>
> Ugh.  This is a GCC bug, or a lack of a GCC feature.  It needs fixing in
> GCC.  Apparently, this hasn't happened for GCC 6.  Is there lack of
> agreement that this is a compiler issue?

There are thread about it at
https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html. I will refresh it
soon.


--
WBR,
Andrew
  

On Wed, Feb 10, 2016 at 8:10 AM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> 2016-02-10 16:07 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>> On 02/10/2016 01:58 PM, Andrew Senkevich wrote:
>>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>>
>>>>>  #define ALIAS_IMPL(alias, target) \
>>>>>  ENTRY (alias); \
>>>>> -       call target; \
>>>>> +       call target@PLT; \
>>>>>         ret; \
>>>>>  END (alias)
>>>>
>>>> This needs a comment why this cannot be a tail call (JMP).
>>>
>>> Yes, it should be jmp.
>>
>> Fine, then you can drop the RET (which was present in Carlos' patch).
>>
>>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>>> is this logic required in libmvec_nonshared.a?
>>>
>>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>>> (BZ #19058).
>>
>> Ugh.  This is a GCC bug, or a lack of a GCC feature.  It needs fixing in
>> GCC.  Apparently, this hasn't happened for GCC 6.  Is there lack of
>> agreement that this is a compiler issue?
>
> There are thread about it at
> https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html. I will refresh it
> soon.

I don't think we should put a GCC bug workaround in glibc. It should
be fixed or worked around it in GCC.
  

2016-02-10 19:13 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Wed, Feb 10, 2016 at 8:10 AM, Andrew Senkevich
> <andrew.n.senkevich@gmail.com> wrote:
>> 2016-02-10 16:07 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>> On 02/10/2016 01:58 PM, Andrew Senkevich wrote:
>>>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>>>
>>>>>>  #define ALIAS_IMPL(alias, target) \
>>>>>>  ENTRY (alias); \
>>>>>> -       call target; \
>>>>>> +       call target@PLT; \
>>>>>>         ret; \
>>>>>>  END (alias)
>>>>>
>>>>> This needs a comment why this cannot be a tail call (JMP).
>>>>
>>>> Yes, it should be jmp.
>>>
>>> Fine, then you can drop the RET (which was present in Carlos' patch).
>>>
>>>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>>>> is this logic required in libmvec_nonshared.a?
>>>>
>>>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>>>> (BZ #19058).
>>>
>>> Ugh.  This is a GCC bug, or a lack of a GCC feature.  It needs fixing in
>>> GCC.  Apparently, this hasn't happened for GCC 6.  Is there lack of
>>> agreement that this is a compiler issue?
>>
>> There are thread about it at
>> https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html. I will refresh it
>> soon.
>
> I don't think we should put a GCC bug workaround in glibc. It should
> be fixed or worked around it in GCC.

Since GCC 4.9 vector function name is formed based on asm declaration
name, to support that GCC versions we need to have such workaround.


--
WBR,
Andrew
  

On Wed, Feb 10, 2016 at 8:21 AM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> 2016-02-10 19:13 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Wed, Feb 10, 2016 at 8:10 AM, Andrew Senkevich
>> <andrew.n.senkevich@gmail.com> wrote:
>>> 2016-02-10 16:07 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>>> On 02/10/2016 01:58 PM, Andrew Senkevich wrote:
>>>>> 2016-02-10 15:08 GMT+03:00 Florian Weimer <fweimer@redhat.com>:
>>>>>> On 02/10/2016 12:36 PM, Andrew Senkevich wrote:
>>>>>>
>>>>>>>  #define ALIAS_IMPL(alias, target) \
>>>>>>>  ENTRY (alias); \
>>>>>>> -       call target; \
>>>>>>> +       call target@PLT; \
>>>>>>>         ret; \
>>>>>>>  END (alias)
>>>>>>
>>>>>> This needs a comment why this cannot be a tail call (JMP).
>>>>>
>>>>> Yes, it should be jmp.
>>>>
>>>> Fine, then you can drop the RET (which was present in Carlos' patch).
>>>>
>>>>>> Out of curiosity, why is it not possible to avoid this indirection?  Why
>>>>>> is this logic required in libmvec_nonshared.a?
>>>>>
>>>>> It is workaround to exclude unnecessary symbol aliases in libmvec.so
>>>>> (BZ #19058).
>>>>
>>>> Ugh.  This is a GCC bug, or a lack of a GCC feature.  It needs fixing in
>>>> GCC.  Apparently, this hasn't happened for GCC 6.  Is there lack of
>>>> agreement that this is a compiler issue?
>>>
>>> There are thread about it at
>>> https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html. I will refresh it
>>> soon.
>>
>> I don't think we should put a GCC bug workaround in glibc. It should
>> be fixed or worked around it in GCC.
>
> Since GCC 4.9 vector function name is formed based on asm declaration
> name, to support that GCC versions we need to have such workaround.
>

So it is the part of ABI, isn't it?
  

On Wed, 10 Feb 2016, H.J. Lu wrote:

> >> I don't think we should put a GCC bug workaround in glibc. It should
> >> be fixed or worked around it in GCC.
> >
> > Since GCC 4.9 vector function name is formed based on asm declaration
> > name, to support that GCC versions we need to have such workaround.
> >
> 
> So it is the part of ABI, isn't it?

It is deliberately only part of the static library ABI, not part of the 
shared library ABI, because the shared libraries should not need more than 
one internal-namespace exported name for the same interface (and it's a 
compiler limitation that the other name exists at all).
  

On 02/10/2016 05:33 PM, Joseph Myers wrote:
> On Wed, 10 Feb 2016, H.J. Lu wrote:
> 
>>>> I don't think we should put a GCC bug workaround in glibc. It should
>>>> be fixed or worked around it in GCC.
>>>
>>> Since GCC 4.9 vector function name is formed based on asm declaration
>>> name, to support that GCC versions we need to have such workaround.
>>>
>>
>> So it is the part of ABI, isn't it?
> 
> It is deliberately only part of the static library ABI, not part of the 
> shared library ABI, because the shared libraries should not need more than 
> one internal-namespace exported name for the same interface (and it's a 
> compiler limitation that the other name exists at all).

I'm still surprised that we ended up in this situation. :-/

Can those distributions who want to support unfixed system compilers
carry this workaround as a local fix?

Florian
  

On Wed, 10 Feb 2016, Florian Weimer wrote:

> > It is deliberately only part of the static library ABI, not part of the 
> > shared library ABI, because the shared libraries should not need more than 
> > one internal-namespace exported name for the same interface (and it's a 
> > compiler limitation that the other name exists at all).
> 
> I'm still surprised that we ended up in this situation. :-/

My original request to work on a GCC-based solution rather than exporting 
the extra names from libmvec.so was *not* meant to mean "post one message 
to the gcc list and expect a solution to emerge and be implemented".  It 
was meant to mean leading the full process from discussion through 
consensus building and implementation.  (And originally we envisaged that 
the asm-based workaround in the header would suffice, before it turned out 
that didn't work with LTO.)
  

On Wed, Feb 10, 2016 at 8:33 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 10 Feb 2016, H.J. Lu wrote:
>
>> >> I don't think we should put a GCC bug workaround in glibc. It should
>> >> be fixed or worked around it in GCC.
>> >
>> > Since GCC 4.9 vector function name is formed based on asm declaration
>> > name, to support that GCC versions we need to have such workaround.
>> >
>>
>> So it is the part of ABI, isn't it?
>
> It is deliberately only part of the static library ABI, not part of the
> shared library ABI, because the shared libraries should not need more than
> one internal-namespace exported name for the same interface (and it's a
> compiler limitation that the other name exists at all).
>

Let me try to understand the problem:

1. GCC should generate an alias, foo, which points to bar defined
in libmvec.so.
2. But the alias foo may be removed by GCC sometimes (LTO?).
3. As a workaround, we put an alias foo, in libmvec_nonshared.a which
points to bar defined in libmvec.so.

Am I correct?
  

On Wed, 10 Feb 2016, H.J. Lu wrote:

> > It is deliberately only part of the static library ABI, not part of the
> > shared library ABI, because the shared libraries should not need more than
> > one internal-namespace exported name for the same interface (and it's a
> > compiler limitation that the other name exists at all).
> >
> 
> Let me try to understand the problem:
> 
> 1. GCC should generate an alias, foo, which points to bar defined
> in libmvec.so.
> 2. But the alias foo may be removed by GCC sometimes (LTO?).
> 3. As a workaround, we put an alias foo, in libmvec_nonshared.a which
> points to bar defined in libmvec.so.
> 
> Am I correct?

Ideally the alias should not exist at all.

The issue is: function foo has two variants for scalar use (foo and 
__foo_finite), where calls to foo get remapped to __foo_finite by 
bits/math-finite.h under certain conditions.  For a given vector type, it 
has, logically, one variant (e.g. _ZGVbN2v_foo).  But because GCC 
determines the names to use when calling vector functions based on the 
assembler name of the scalar function, sometimes it generates calls to 
_ZGVbN2v___foo_finite instead.

Initially, this was worked around by using top-level asm statements in 
bits/math-vector.h to set "_ZGVbN2v___foo_finite = _ZGVbN2v_foo".  That 
meant that the calls still resulted in references to _ZGVbN2v_foo instead 
of _ZGVbN2v___foo_finite in the .o files.  But remapping like that proved 
not to solve the problem for LTO, so the wrapper _ZGVbN2v___foo_finite was 
needed in libmvec_nonshared.a.

The sort of proper solution I envisage is a new attribute that can be used 
in bits/math-vector.h, e.g. __attribute__ ((__vector_asm_name__ ("foo"))), 
which would mean that the given string is used as the basis for forming 
names of vector function variants, instead of using the 
DECL_ASSEMBLER_NAME.  That way, glibc headers could determine the name of 
the scalar version to use (specified with asm) and the name to use as the 
basis for vector versions' names (specified with that attribute) 
independently.
  

On Wed, Feb 10, 2016 at 9:38 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 10 Feb 2016, H.J. Lu wrote:
>
>> > It is deliberately only part of the static library ABI, not part of the
>> > shared library ABI, because the shared libraries should not need more than
>> > one internal-namespace exported name for the same interface (and it's a
>> > compiler limitation that the other name exists at all).
>> >
>>
>> Let me try to understand the problem:
>>
>> 1. GCC should generate an alias, foo, which points to bar defined
>> in libmvec.so.
>> 2. But the alias foo may be removed by GCC sometimes (LTO?).
>> 3. As a workaround, we put an alias foo, in libmvec_nonshared.a which
>> points to bar defined in libmvec.so.
>>
>> Am I correct?
>
> Ideally the alias should not exist at all.
>
> The issue is: function foo has two variants for scalar use (foo and
> __foo_finite), where calls to foo get remapped to __foo_finite by
> bits/math-finite.h under certain conditions.  For a given vector type, it
> has, logically, one variant (e.g. _ZGVbN2v_foo).  But because GCC
> determines the names to use when calling vector functions based on the
> assembler name of the scalar function, sometimes it generates calls to
> _ZGVbN2v___foo_finite instead.
>
> Initially, this was worked around by using top-level asm statements in
> bits/math-vector.h to set "_ZGVbN2v___foo_finite = _ZGVbN2v_foo".  That
> meant that the calls still resulted in references to _ZGVbN2v_foo instead
> of _ZGVbN2v___foo_finite in the .o files.  But remapping like that proved
> not to solve the problem for LTO, so the wrapper _ZGVbN2v___foo_finite was
> needed in libmvec_nonshared.a.
>
> The sort of proper solution I envisage is a new attribute that can be used
> in bits/math-vector.h, e.g. __attribute__ ((__vector_asm_name__ ("foo"))),
> which would mean that the given string is used as the basis for forming
> names of vector function variants, instead of using the
> DECL_ASSEMBLER_NAME.  That way, glibc headers could determine the name of
> the scalar version to use (specified with asm) and the name to use as the
> basis for vector versions' names (specified with that attribute)
> independently.

Since the proper fix involves both GCC and glibc, do we have bugs
open for GCC and glibc so that it won't get lost?
  

On Wed, 10 Feb 2016, H.J. Lu wrote:

> Since the proper fix involves both GCC and glibc, do we have bugs
> open for GCC and glibc so that it won't get lost?

I'm not aware of such bugs.
  

2016-02-10 15:45 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Wed, Feb 10, 2016 at 3:36 AM, Andrew Senkevich
> <andrew.n.senkevich@gmail.com> wrote:
>> Hi,
>>
>> here the fix for BZ #19590.
>>
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 11c3156..6ebcefb 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2016-02-10  Andrew Senkevich  <andrew.senkevich@intel.com>
>> +           Carlos O'Donell  <carlos@redhat.com>
>> +
>> +       [BZ #19590]
>> +       * sysdeps/x86_64/fpu/svml_finite_alias.S (ALIAS_IMPL): Use PLT.
>> +
>>  2016-02-04  Rajalakshmi Srinivasaraghavan  <raji@linux.vnet.ibm.com>
>>
>>         * sysdeps/powerpc/fpu/libm-test-ulps: Regenerated.
>> diff --git a/sysdeps/x86_64/fpu/svml_finite_alias.S
>> b/sysdeps/x86_64/fpu/svml_finite_alias.S
>> index 0062fe4..0da3b64 100644
>> --- a/sysdeps/x86_64/fpu/svml_finite_alias.S
>> +++ b/sysdeps/x86_64/fpu/svml_finite_alias.S
>> @@ -23,7 +23,7 @@
>>
>>  #define ALIAS_IMPL(alias, target) \
>>  ENTRY (alias); \
>> -       call target; \
>> +       call target@PLT; \
>>         ret; \
>>  END (alias)
>>
>>
>
> Need a testcase,   You can use assembly code in testcase to make it
> compiler independent.

If we need runtime tests with calls to finite aliases it looks better
to adopt build of existing libmvec tests. We can call finite aliases
in *-wrappers.c and build shared library from them and link with it
test binaries. Is this approach looks OK?



--
WBR,
Andrew
  

On Thu, 11 Feb 2016, Andrew Senkevich wrote:

> If we need runtime tests with calls to finite aliases it looks better
> to adopt build of existing libmvec tests. We can call finite aliases
> in *-wrappers.c and build shared library from them and link with it
> test binaries. Is this approach looks OK?

I don't think the finite aliases should be considered part of the API to 
test; it's an implementation detail of the header that they may get 
referenced in certain circumstances.  The relevant thing to test is 
whether building a program that directly calls the scalar functions, with 
options such that the calls get vectorized, works (including with variant 
options for e.g. LTO).