Re: [PATCH] Revert commit 05a910f7

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Mar 8, 2016 at 4:24 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> 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...

Exactly.  Has anyone reported any issues against it?

>> 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
>

<string.h> has

#if defined __GNUC__ && __GNUC__ >= 2
# if defined __OPTIMIZE__ && !defined __OPTIMIZE_SIZE__ \
     && !defined __NO_INLINE__ && !defined __cplusplus
/* When using GNU CC we provide some optimized versions of selected
   functions from this header.  There are two kinds of optimizations:

   - machine-dependent optimizations, most probably using inline
     assembler code; these might be quite expensive since the code
     size can increase significantly.
     These optimizations are not used unless the symbol
__USE_STRING_INLINES
     is defined before including this header.

   - machine-independent optimizations which do not increase the
     code size significantly and which optimize mainly situations
     where one or more arguments are compile-time constants.
     These optimizations are used always when the compiler is
     taught to optimize.

   One can inhibit all optimizations by defining __NO_STRING_INLINES.  */

/* Get the machine-dependent optimizations (if any).  */
#  include <bits/string.h>

/* These are generic optimizations which do not add too much inline code.  */
#  include <bits/string2.h>
# endif

# if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function
/* Functions with security checks.  */
#  include <bits/string3.h>
# endif
#endif

Why do we want to remove  <bits/string2.h>?  Shouldn't inline mempcpy
be there?


-- 
H.J.