* [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
@ 2017-06-29 17:30 H.J. Lu
2017-06-29 17:55 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-29 17:30 UTC (permalink / raw)
To: GNU C Library
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>.
---
sysdeps/generic/malloc-alignment.h | 24 ++++++++++++++++++++++++
sysdeps/generic/malloc-machine.h | 1 +
sysdeps/i386/malloc-alignment.h | 24 ++++++++++++++++++++++++
3 files changed, 49 insertions(+)
create mode 100644 sysdeps/generic/malloc-alignment.h
create mode 100644 sysdeps/i386/malloc-alignment.h
diff --git a/sysdeps/generic/malloc-alignment.h b/sysdeps/generic/malloc-alignment.h
new file mode 100644
index 0000000..193e827
--- /dev/null
+++ b/sysdeps/generic/malloc-alignment.h
@@ -0,0 +1,24 @@
+/* Define MALLOC_ALIGNMENT for malloc. Generic version.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _GENERIC_MALLOC_ALIGNMENT_H
+#define _GENERIC_MALLOC_ALIGNMENT_H
+
+/* Use the default MALLOC_ALIGNMENT. */
+
+#endif /* !defined(_GENERIC_MALLOC_ALIGNMENT_H) */
diff --git a/sysdeps/generic/malloc-machine.h b/sysdeps/generic/malloc-machine.h
index 21aa9fc..4491b90 100644
--- a/sysdeps/generic/malloc-machine.h
+++ b/sysdeps/generic/malloc-machine.h
@@ -21,6 +21,7 @@
#define _GENERIC_MALLOC_MACHINE_H
#include <atomic.h>
+#include <malloc-alignment.h>
#ifndef atomic_full_barrier
# define atomic_full_barrier() __asm ("" ::: "memory")
diff --git a/sysdeps/i386/malloc-alignment.h b/sysdeps/i386/malloc-alignment.h
new file mode 100644
index 0000000..f72f7a8
--- /dev/null
+++ b/sysdeps/i386/malloc-alignment.h
@@ -0,0 +1,24 @@
+/* Define MALLOC_ALIGNMENT for malloc. i386 version.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _I386_MALLOC_ALIGNMENT_H
+#define _I386_MALLOC_ALIGNMENT_H
+
+#define MALLOC_ALIGNMENT 16
+
+#endif /* !defined(_I386_MALLOC_ALIGNMENT_H) */
--
2.9.4
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 17:30 [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120] H.J. Lu
@ 2017-06-29 17:55 ` Carlos O'Donell
2017-06-29 18:07 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-29 17:55 UTC (permalink / raw)
To: H.J. Lu, GNU C Library
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.
OK with that change.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 17:55 ` Carlos O'Donell
@ 2017-06-29 18:07 ` H.J. Lu
2017-06-29 18:11 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-29 18:07 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
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.
> OK with that change.
>
> --
> Cheers,
> Carlos.
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 18:07 ` H.J. Lu
@ 2017-06-29 18:11 ` Carlos O'Donell
2017-06-29 18:43 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-29 18:11 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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?
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 18:11 ` Carlos O'Donell
@ 2017-06-29 18:43 ` H.J. Lu
2017-06-29 20:05 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-29 18:43 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
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.
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 18:43 ` H.J. Lu
@ 2017-06-29 20:05 ` Carlos O'Donell
2017-06-29 20:10 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-29 20:05 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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?
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 20:05 ` Carlos O'Donell
@ 2017-06-29 20:10 ` H.J. Lu
2017-06-30 11:54 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-29 20:10 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
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.
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-29 20:10 ` H.J. Lu
@ 2017-06-30 11:54 ` H.J. Lu
2017-06-30 13:05 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-30 11:54 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 11:54 ` H.J. Lu
@ 2017-06-30 13:05 ` Carlos O'Donell
2017-06-30 13:35 ` H.J. Lu
2017-06-30 14:31 ` Joseph Myers
0 siblings, 2 replies; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-30 13:05 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
On 06/30/2017 07:54 AM, H.J. Lu wrote:
> On Thu, Jun 29, 2017 at 1:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> Are you suggesting 3 i386 header files to define MALLOC_ALIGNMENT?
>>
>> 1. generic i386.
>> 2. nptl i386.
>> 3. hurd i386.
>>
>> That is very weird.
I hope my explanation below makes the difference clearer.
> I built and checked them for i686 with GCC 7. The biggest challenge is to
> support GCC 7, not the header file itself.
We have an existing malloc-machine.h for this purpose.
The design question is:
(a) Should we add a new file malloc-alignment.h to specify a new parameter.
- Adds new file malloc-alignment.h.
- 2 new files.
(b) Should we use existing malloc-machine.h to include the new parameter?
- Extends existing malloc-machine.h to new sysdep dirs.
- 3 new files.
You are arguing for (a) and I argue for (b).
Use 1 less file but require a new header.
Use 1 more file but use existing headers.
I argue that it's simpler and easier for developers if we avoid additional headers
and use existing headers.
I'm open hear a comment from someone else.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 13:05 ` Carlos O'Donell
@ 2017-06-30 13:35 ` H.J. Lu
2017-06-30 13:52 ` Carlos O'Donell
2017-06-30 14:31 ` Joseph Myers
1 sibling, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-30 13:35 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
On Fri, Jun 30, 2017 at 6:05 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 06/30/2017 07:54 AM, H.J. Lu wrote:
>> On Thu, Jun 29, 2017 at 1:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> Are you suggesting 3 i386 header files to define MALLOC_ALIGNMENT?
>>>
>>> 1. generic i386.
>>> 2. nptl i386.
>>> 3. hurd i386.
>>>
>>> That is very weird.
>
> I hope my explanation below makes the difference clearer.
>
>> I built and checked them for i686 with GCC 7. The biggest challenge is to
>> support GCC 7, not the header file itself.
>
> We have an existing malloc-machine.h for this purpose.
>
> The design question is:
>
> (a) Should we add a new file malloc-alignment.h to specify a new parameter.
> - Adds new file malloc-alignment.h.
> - 2 new files.
And define MALLOC_ALIGNMENT for i386 in one place.
>
> (b) Should we use existing malloc-machine.h to include the new parameter?
> - Extends existing malloc-machine.h to new sysdep dirs.
> - 3 new files.
And define MALLOC_ALIGNMENT for i386 in 3 difference places.
If we need to support a new OS or threading library, we need to
define MALLOC_ALIGNMENT for i386 in another place.
> You are arguing for (a) and I argue for (b).
>
> Use 1 less file but require a new header.
> Use 1 more file but use existing headers.
>
> I argue that it's simpler and easier for developers if we avoid additional headers
> and use existing headers.
>
Because your suggestion defines MALLOC_ALIGNMENT for i386 in
more than one place, I argue it is more complex and more error
prone than a single MALLOC_ALIGNMENT definition for i386. With
my approach, it is fixed once for all.
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 13:35 ` H.J. Lu
@ 2017-06-30 13:52 ` Carlos O'Donell
0 siblings, 0 replies; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-30 13:52 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
On 06/30/2017 09:35 AM, H.J. Lu wrote:
> On Fri, Jun 30, 2017 at 6:05 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 06/30/2017 07:54 AM, H.J. Lu wrote:
>>> On Thu, Jun 29, 2017 at 1:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> Are you suggesting 3 i386 header files to define MALLOC_ALIGNMENT?
>>>>
>>>> 1. generic i386.
>>>> 2. nptl i386.
>>>> 3. hurd i386.
>>>>
>>>> That is very weird.
>>
>> I hope my explanation below makes the difference clearer.
>>
>>> I built and checked them for i686 with GCC 7. The biggest challenge is to
>>> support GCC 7, not the header file itself.
>>
>> We have an existing malloc-machine.h for this purpose.
>>
>> The design question is:
>>
>> (a) Should we add a new file malloc-alignment.h to specify a new parameter.
>> - Adds new file malloc-alignment.h.
>> - 2 new files.
>
> And define MALLOC_ALIGNMENT for i386 in one place.
>
>>
>> (b) Should we use existing malloc-machine.h to include the new parameter?
>> - Extends existing malloc-machine.h to new sysdep dirs.
>> - 3 new files.
>
> And define MALLOC_ALIGNMENT for i386 in 3 difference places.
> If we need to support a new OS or threading library, we need to
> define MALLOC_ALIGNMENT for i386 in another place.
>
>> You are arguing for (a) and I argue for (b).
>>
>> Use 1 less file but require a new header.
>> Use 1 more file but use existing headers.
>>
>> I argue that it's simpler and easier for developers if we avoid additional headers
>> and use existing headers.
>>
>
> Because your suggestion defines MALLOC_ALIGNMENT for i386 in
> more than one place, I argue it is more complex and more error
> prone than a single MALLOC_ALIGNMENT definition for i386. With
> my approach, it is fixed once for all.
Can't they all equally just include 'sysdeps/i386/malloc-machine.h'?
/* Include the base i386 definitions. */
#include <sysdeps/i386/malloc-machine.h>
So you have one definition in one place?
In sysdeps/nptl/malloc-machine.h we already have:
#include <sysdeps/generic/malloc-machine.h>
So we already use a layered approach to avoid multiple definitions.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 13:05 ` Carlos O'Donell
2017-06-30 13:35 ` H.J. Lu
@ 2017-06-30 14:31 ` Joseph Myers
2017-06-30 14:48 ` H.J. Lu
1 sibling, 1 reply; 14+ messages in thread
From: Joseph Myers @ 2017-06-30 14:31 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: H.J. Lu, GNU C Library
On Fri, 30 Jun 2017, Carlos O'Donell wrote:
> (a) Should we add a new file malloc-alignment.h to specify a new parameter.
> - Adds new file malloc-alignment.h.
> - 2 new files.
My preference is to add headers like this and so reduce the need for
#ifndef (that is, have a generic malloc-alignment that includes the
definition that's currently guarded with #ifndef).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 14:31 ` Joseph Myers
@ 2017-06-30 14:48 ` H.J. Lu
2017-06-30 16:04 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2017-06-30 14:48 UTC (permalink / raw)
To: Joseph Myers; +Cc: Carlos O'Donell, GNU C Library
[-- Attachment #1: Type: text/plain, Size: 544 bytes --]
On Fri, Jun 30, 2017 at 7:31 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Fri, 30 Jun 2017, Carlos O'Donell wrote:
>
>> (a) Should we add a new file malloc-alignment.h to specify a new parameter.
>> - Adds new file malloc-alignment.h.
>> - 2 new files.
>
> My preference is to add headers like this and so reduce the need for
> #ifndef (that is, have a generic malloc-alignment that includes the
> definition that's currently guarded with #ifndef).
>
I like this idea. Here is the patch. OK for master?
Thanks.
--
H.J.
[-- Attachment #2: 0001-i386-Increase-MALLOC_ALIGNMENT-to-16-BZ-21120.patch --]
[-- Type: text/x-patch, Size: 5472 bytes --]
From 1c13144a6252b1edc40c4900c6ea683aea6fc691 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 29 Jun 2017 10:26:04 -0700
Subject: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
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 moves the MALLOC_ALIGNMENT definition to <malloc-alignment.h>
and increases the malloc alignment to 16 for i386.
[BZ #21120]
* malloc/malloc-internal.h (MALLOC_ALIGNMENT): Moved to ...
* sysdeps/generic/malloc-alignment.h: Here. New file.
* sysdeps/i386/malloc-alignment.h: Likewise.
* sysdeps/generic/malloc-machine.h: Include <malloc-alignment.h>.
---
malloc/malloc-internal.h | 10 ----------
sysdeps/generic/malloc-alignment.h | 31 +++++++++++++++++++++++++++++++
sysdeps/generic/malloc-machine.h | 1 +
sysdeps/i386/malloc-alignment.h | 24 ++++++++++++++++++++++++
4 files changed, 56 insertions(+), 10 deletions(-)
create mode 100644 sysdeps/generic/malloc-alignment.h
create mode 100644 sysdeps/i386/malloc-alignment.h
diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
index dbd801a..6a62717 100644
--- a/malloc/malloc-internal.h
+++ b/malloc/malloc-internal.h
@@ -58,16 +58,6 @@
/* The corresponding word size. */
#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
-/* MALLOC_ALIGNMENT is the minimum alignment for malloc'ed chunks. It
- must be a power of two at least 2 * SIZE_SZ, even on machines for
- which smaller alignments would suffice. It may be defined as larger
- than this though. Note however that code and data structures are
- optimized for the case of 8-byte alignment. */
-#ifndef MALLOC_ALIGNMENT
-# define MALLOC_ALIGNMENT (2 * SIZE_SZ < __alignof__ (long double) \
- ? __alignof__ (long double) : 2 * SIZE_SZ)
-#endif
-
/* The corresponding bit mask value. */
#define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1)
diff --git a/sysdeps/generic/malloc-alignment.h b/sysdeps/generic/malloc-alignment.h
new file mode 100644
index 0000000..efd03fa
--- /dev/null
+++ b/sysdeps/generic/malloc-alignment.h
@@ -0,0 +1,31 @@
+/* Define MALLOC_ALIGNMENT for malloc. Generic version.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _GENERIC_MALLOC_ALIGNMENT_H
+#define _GENERIC_MALLOC_ALIGNMENT_H
+
+/* MALLOC_ALIGNMENT is the minimum alignment for malloc'ed chunks. It
+ must be a power of two at least 2 * SIZE_SZ, even on machines for
+ which smaller alignments would suffice. It may be defined as larger
+ than this though. Note however that code and data structures are
+ optimized for the case of 8-byte alignment. */
+#define MALLOC_ALIGNMENT (2 * SIZE_SZ < __alignof__ (long double) \
+ ? __alignof__ (long double) : 2 * SIZE_SZ)
+
+
+#endif /* !defined(_GENERIC_MALLOC_ALIGNMENT_H) */
diff --git a/sysdeps/generic/malloc-machine.h b/sysdeps/generic/malloc-machine.h
index 21aa9fc..4491b90 100644
--- a/sysdeps/generic/malloc-machine.h
+++ b/sysdeps/generic/malloc-machine.h
@@ -21,6 +21,7 @@
#define _GENERIC_MALLOC_MACHINE_H
#include <atomic.h>
+#include <malloc-alignment.h>
#ifndef atomic_full_barrier
# define atomic_full_barrier() __asm ("" ::: "memory")
diff --git a/sysdeps/i386/malloc-alignment.h b/sysdeps/i386/malloc-alignment.h
new file mode 100644
index 0000000..f72f7a8
--- /dev/null
+++ b/sysdeps/i386/malloc-alignment.h
@@ -0,0 +1,24 @@
+/* Define MALLOC_ALIGNMENT for malloc. i386 version.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _I386_MALLOC_ALIGNMENT_H
+#define _I386_MALLOC_ALIGNMENT_H
+
+#define MALLOC_ALIGNMENT 16
+
+#endif /* !defined(_I386_MALLOC_ALIGNMENT_H) */
--
2.9.4
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120]
2017-06-30 14:48 ` H.J. Lu
@ 2017-06-30 16:04 ` Carlos O'Donell
0 siblings, 0 replies; 14+ messages in thread
From: Carlos O'Donell @ 2017-06-30 16:04 UTC (permalink / raw)
To: H.J. Lu, Joseph Myers; +Cc: GNU C Library
On 06/30/2017 10:48 AM, H.J. Lu wrote:
> On Fri, Jun 30, 2017 at 7:31 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Fri, 30 Jun 2017, Carlos O'Donell wrote:
>>
>>> (a) Should we add a new file malloc-alignment.h to specify a new parameter.
>>> - Adds new file malloc-alignment.h.
>>> - 2 new files.
>>
>> My preference is to add headers like this and so reduce the need for
>> #ifndef (that is, have a generic malloc-alignment that includes the
>> definition that's currently guarded with #ifndef).
>>
>
> I like this idea. Here is the patch. OK for master?
Agreed, that's a better solution than my suggested (b).
LGTM.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-06-30 16:04 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 17:30 [PATCH] i386: Increase MALLOC_ALIGNMENT to 16 [BZ #21120] H.J. Lu
2017-06-29 17:55 ` Carlos O'Donell
2017-06-29 18:07 ` H.J. Lu
2017-06-29 18:11 ` Carlos O'Donell
2017-06-29 18:43 ` H.J. Lu
2017-06-29 20:05 ` Carlos O'Donell
2017-06-29 20:10 ` H.J. Lu
2017-06-30 11:54 ` H.J. Lu
2017-06-30 13:05 ` Carlos O'Donell
2017-06-30 13:35 ` H.J. Lu
2017-06-30 13:52 ` Carlos O'Donell
2017-06-30 14:31 ` Joseph Myers
2017-06-30 14:48 ` H.J. Lu
2017-06-30 16:04 ` Carlos O'Donell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).