Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]

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

On Thu, Jun 29, 2017 at 1:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jun 29, 2017 at 1:03 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 06/29/2017 02:43 PM, H.J. Lu wrote:
>>> On Thu, Jun 29, 2017 at 11:11 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>> On 06/29/2017 02:06 PM, H.J. Lu wrote:
>>>>> On Thu, Jun 29, 2017 at 10:55 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>> On 06/29/2017 01:30 PM, H.J. Lu wrote:
>>>>>>> GCC 7 changed the definition of max_align_t on i386:
>>>>>>>
>>>>>>> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9b5c49ef97e63cc63f1ffa13baf771368105ebe2
>>>>>>>
>>>>>>> As a result, glibc malloc no longer returns memory blocks which are as
>>>>>>> aligned as max_align_t requires.
>>>>>>>
>>>>>>> This causes malloc/tst-malloc-thread-fail to fail with an error like this
>>>>>>> one:
>>>>>>>
>>>>>>> error: allocation function 0, size 144 not aligned to 16
>>>>>>>
>>>>>>> This patch increases the malloc alignment to 16 for i386.
>>>>>>>
>>>>>>> Tested on i386 with GCC 7 and on x86-64.  OK for master?
>>>>>>>
>>>>>>> H.J.
>>>>>>> ---
>>>>>>>       [BZ #21120]
>>>>>>>       * sysdeps/generic/malloc-alignment.h: New file.
>>>>>>>       * sysdeps/i386/malloc-alignment.h: Likewise.
>>>>>>>       * sysdeps/generic/malloc-machine.h: Include <malloc-alignment.h>.
>>>>>>
>>>>>> Please use malloc-machine.h which was the previous header that provided
>>>>>> machine-dependent malloc definitions. That way we remain consistent across
>>>>>> releases and make it easier to backport such changes without adding a new
>>>>>> header.
>>>>>
>>>>> It won't work too well for Hurd since we have
>>>>>
>>>>> ./sysdeps/generic/malloc-machine.h
>>>>> ./sysdeps/mach/hurd/malloc-machine.h
>>>>> ./sysdeps/nptl/malloc-machine.h
>>>>>
>>>>> What will Hurd/i386 get?   malloc-alignment.h handles it automatically.
>>>>
>>>> If your patch made Hurd/i386 use MALLOC_ALIGNMENT of 16 then a new patch
>>>> using malloc-machine.h would set MALLOC_ALIGNMENT to 16 in
>>>> systeps/mach/hurd/malloc-machine.h?
>>>>
>>>
>>> This assumes that mach/hurd == i386.  Also I don't like define
>>> MALLOC_ALIGNMENT to 16 for i386 in 2 different places.
>>> Since
>>>
>>> ./sysdeps/generic/malloc-machine.h
>>> ./sysdeps/mach/hurd/malloc-machine.h
>>> ./sysdeps/nptl/malloc-machine.h
>>>
>>> malloc-machine.h is not pure processor specific. It is also OS
>>> specific.  I prefer to define MALLOC_ALIGNMENT to 16 for i386
>>> in a processor specific header file.
>>
>> Add a sysdeps/mach/hurd/i386, which is an os/machine directory.
>>
>> Similarly for i386?
>>
>
> Are you suggesting 3 i386 header files to define MALLOC_ALIGNMENT?
>
> 1. generic i386.
> 2. nptl i386.
> 3. hurd i386.
>
> That is very weird.

malloc-machine.h is an internal header and was never installed.  Adding
another internal header makes no differences for backport in this case.
FWIW, I backported my malloc-alignment.h patch to glibc 2.25:

https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/hjl/pr21120/2.25

and 2.24:

https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/hjl/pr21120/2.24

I built and checked them for i686 with GCC 7.  The biggest challenge is to
support GCC 7, not the header file itself.

-- 
H.J.