Re: [PATCH] Revert commit 05a910f7

[Wilco Dijkstra <Wilco dot Dijkstra at arm dot com>]

H.J. Lu <hjl.tools@gmail.com> wrote:
On Mon, Mar 7, 2016 at 4:13 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Mar 7, 2016 at 3:20 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>> H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> +/* Don't inline mempcpy into memcpy as x86 has an optimized mempcpy.  */
>>>> +# define _HAVE_STRING_ARCH_mempcpy 1
>>>> +
>>>>  /* Copy N bytes of SRC to DEST.  */
>>>>  # define _HAVE_STRING_ARCH_memcpy 1
>>>>  # define memcpy(dest, src, n) \
>>>> --
>>>> 2.5.0
>>>>
>>>
>>> It doesn't work since  <bits/string.h> is included only if
>>>
>>> #if defined __GNUC__ && __GNUC__ >= 2
>>> # if defined __OPTIMIZE__ && !defined __OPTIMIZE_SIZE__ \
>>>      && !defined __NO_INLINE__ && !defined __cplusplus
>>>
>>> is true.
>>>
>>
>>> I believe commit 05a910f7 was wrong.  At minimum,
>>> mempcpy shouldn't be inlined in 2 different header files.
>>
>> There is nothing wrong with that commit. I already posted patches that remove most
>> of the redundant stuff from bits/string.h, including the duplicate mempcpy defines.
>> I don't understand how defining _HAVE_STRING_ARCH_mempcpy doesn't work for you
>> either, unless you use non-standard options or a very ancient compiler.
>
> I can make it to work.
>
> If we don't want to wait before the other mempcpy inline is removed first,
> we can put the new mempcpy inline in a new header file and x86 won't
> include it until the other mempcpy inline is removed.  It is very odd
> to have mempcpy inlined in 2 different places.

Actually it is inlined in 3 different places as x86/bits/string.h also defines it. 

If _HAVE_STRING_ARCH_mempcpy doesn't work in all circumstances, it is due
to the weird condition for including bits/string.h. That's on my list to get rid of, but
if you want something asap then you could move this from <string.h>

#if defined __GNUC__ && __GNUC__ >= 2
# if defined __OPTIMIZE__ && !defined __OPTIMIZE_SIZE__ \
      && !defined __NO_INLINE__ && !defined __cplusplus

into x86/bits/string.h and sparc/bits/string.h, and add your _HAVE_STRING_ARCH_mempcpy
define before it.

>> The proper solution is to get rid of the bits/string.h mess altogether rather than
>> conditionally including it. With my outstanding patches we're there most of the way.
>>
>> Wilco
>>
>
> The remove patch should have gone in first before adding another one.

That wasn't feasible at the time due to the complex mess in string2.h. After 5 
cleanup patches it has now become possible to remove it altogether. A few
more and we can get rid of string2.h altogether! It may also be worth checking 
whether any of the inlines in i386/bits/string.h are still relevant today as many
use rep movsb variants that stopped being useful 2 decades ago...

> Another thing, I don't think inline with _HAVE_STRING_ARCH_mempcpy
> checking should be in <string.h>.  It belongs to a different header file.

Why? String.h contains lots of inlines.

Wilco