[PATCH] Implement new hook for max_align_t_align

[PATCH] Implement new hook for max_align_t_align

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

Various pthread types on hppa-linux require an alignment of 16 bytes for ABI reasons.  This is larger
than the current implementations of max_align_t and max_align_t_align.  As a result, the patch to
implement C++17 over-aligned new  causes warnings on hppa.  The thread here discusses the problem:
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01593.html

The attached patch implements a new hook to allow overriding the default implementation of max_align_t_align
in the PA backend.  It changes the max_align_t alignment to 16 bytes on 32-bit hppa-linux.  I added an additional
comment to BIGGEST_ALIGNMENT in pa.h to document the alignment choices for long double on hpux.  I also
defined MALLOC_ABI_ALIGNMENT so that it reflects the behaviour of the glibc and hpux malloc implementations.

Tested on hppa2.0w-hp-hpux11.11, hppa64-hp-hpux11.11 and hppa-unknown-linux-gnu with no observed
regressions.

Okay for trunk?

Dave
--
John David Anglin	dave.anglin@bell.net


[-- Attachment #2: ldcw-align.d.txt --]
[-- Type: text/plain, Size: 10431 bytes --]

2016-10-08  John David Anglin  <danglin@gcc.gnu.org>

gcc/c-family/
	* c-common.c (max_align_t_align): Move to targhooks.c.
	* c-common.h (max_align_t_align): Delete.
gcc/
	* target.def (max_align_t_align): New target hook.
	* targhooks.c (default_max_align_t_align): New.
	* targhooks.h (default_max_align_t_align): Declare.
	* config/pa/pa.h (BIGGEST_ALIGNMENT): Adjust comment.
	(MALLOC_ABI_ALIGNMENT): Define.
	* config/pa/pa.c (pa_max_align_t_align): New.
	(TARGET_MAX_ALIGN_T_ALIGN): Define.
	* doc/tm.texi.in (TARGET_MAX_ALIGN_T_ALIGN): Add documentation hook.
	* doc/tm.texi: Update.
gcc/cp/
	* decl.c (cxx_init_decl_processing): Use max_align_t_align target hook.
	* init.c (build_new_1): Likewise.

Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 240688)
+++ c-family/c-common.c	(working copy)
@@ -12899,22 +12899,6 @@
   return stv_nothing;
 }
 
-/* Return the alignment of std::max_align_t.
-
-   [support.types.layout] The type max_align_t is a POD type whose alignment
-   requirement is at least as great as that of every scalar type, and whose
-   alignment requirement is supported in every context.  */
-
-unsigned
-max_align_t_align ()
-{
-  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
-				TYPE_ALIGN (long_double_type_node));
-  if (float128_type_node != NULL_TREE)
-    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
-  return max_align;
-}
-
 /* Return true iff ALIGN is an integral constant that is a fundamental
    alignment, as defined by [basic.align] in the c++-11
    specifications.
@@ -12928,7 +12912,7 @@
 bool
 cxx_fundamental_alignment_p (unsigned align)
 {
-  return (align <= max_align_t_align ());
+  return (align <= targetm.max_align_t_align ());
 }
 
 /* Return true if T is a pointer to a zero-sized aggregate.  */
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 240688)
+++ c-family/c-common.h	(working copy)
@@ -864,7 +864,6 @@
 extern bool keyword_is_storage_class_specifier (enum rid);
 extern bool keyword_is_type_qualifier (enum rid);
 extern bool keyword_is_decl_specifier (enum rid);
-extern unsigned max_align_t_align (void);
 extern bool cxx_fundamental_alignment_p (unsigned);
 extern bool pointer_to_zero_sized_aggr_p (tree);
 extern bool diagnose_mismatched_attributes (tree, tree);
Index: config/pa/pa.c
===================================================================
--- config/pa/pa.c	(revision 240688)
+++ config/pa/pa.c	(working copy)
@@ -194,6 +194,7 @@
 static bool pa_legitimate_constant_p (machine_mode, rtx);
 static unsigned int pa_section_type_flags (tree, const char *, int);
 static bool pa_legitimate_address_p (machine_mode, rtx, bool);
+static unsigned int pa_max_align_t_align (void);
 
 /* The following extra sections are only used for SOM.  */
 static GTY(()) section *som_readonly_data_section;
@@ -400,6 +401,9 @@
 #undef TARGET_LRA_P
 #define TARGET_LRA_P hook_bool_void_false
 
+#undef TARGET_MAX_ALIGN_T_ALIGN
+#define TARGET_MAX_ALIGN_T_ALIGN pa_max_align_t_align
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 /* Parse the -mfixed-range= option string.  */
@@ -10719,4 +10723,16 @@
   return NULL_RTX;
 }
 
+/* The maximimum alignment in bits for the POD type std:max_align_t.
+   This is set to 128 on 32-bit non HP-UX systems to suppress warnings
+   about new with extended alignment.  This arises because various POSIX
+   types such as pthread_mutex_t have for historical reasons 128-bit
+   alignment but the default alignment of std:max_align_t is 64 bits.  */
+
+static unsigned int
+pa_max_align_t_align (void)
+{
+  return TARGET_HPUX && !TARGET_64BIT ? 64 : 128;
+}
+
 #include "gt-pa.h"
Index: config/pa/pa.h
===================================================================
--- config/pa/pa.h	(revision 240688)
+++ config/pa/pa.h	(working copy)
@@ -292,9 +292,21 @@
 /* A bit-field declared as `int' forces `int' alignment for the struct.  */
 #define PCC_BITFIELD_TYPE_MATTERS 1
 
-/* No data type wants to be aligned rounder than this.  */
+/* No data type wants to be aligned rounder than this.  The long double
+   type has 16-byte alignment on the 64-bit target even though it was never
+   implemented in hardware.  The software implementation only needs 8-byte
+   alignment.  This is to match the HP compilers.  */
 #define BIGGEST_ALIGNMENT (2 * BITS_PER_WORD)
 
+/* Alignment, in bits, a C conformant malloc implementation has to provide.
+   The HP-UX malloc implementation provides a default alignment of 8 bytes.
+   This can be increased with mallopt.  The glibc implementation also provides
+   8-byte alignment.  Note that this isn't enough for various POSIX types such
+   as pthread_mutex_t.  However, since we no longer need the 16-byte alignment
+   for atomic operations, we ignore the nominal alignment specified for these
+   types.  The same is true for long double on 64-bit HP-UX.  */
+#define MALLOC_ABI_ALIGNMENT (64)
+
 /* Get around hp-ux assembler bug, and make strcpy of constants fast.  */
 #define CONSTANT_ALIGNMENT(EXP, ALIGN)		\
   (TREE_CODE (EXP) == STRING_CST		\
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 240688)
+++ cp/decl.c	(working copy)
@@ -4141,7 +4141,7 @@
   if (aligned_new_threshold == -1)
     aligned_new_threshold = (cxx_dialect >= cxx1z) ? 1 : 0;
   if (aligned_new_threshold == 1)
-    aligned_new_threshold = max_align_t_align () / BITS_PER_UNIT;
+    aligned_new_threshold = targetm.max_align_t_align () / BITS_PER_UNIT;
 
   {
     tree newattrs, extvisattr;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 240688)
+++ cp/init.c	(working copy)
@@ -3017,7 +3017,7 @@
   gcc_assert (alloc_fn != NULL_TREE);
 
   if (warn_aligned_new
-      && TYPE_ALIGN (elt_type) > max_align_t_align ()
+      && TYPE_ALIGN (elt_type) > targetm.max_align_t_align ()
       && (warn_aligned_new > 1
 	  || CP_DECL_CONTEXT (alloc_fn) == global_namespace)
       && !aligned_allocation_fn_p (alloc_fn))
Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 240688)
+++ doc/tm.texi	(working copy)
@@ -11686,6 +11686,10 @@
 ISO C11 requires atomic compound assignments that may raise floating-point exceptions to raise exceptions corresponding to the arithmetic operation whose result was successfully stored in a compare-and-exchange sequence.  This requires code equivalent to calls to @code{feholdexcept}, @code{feclearexcept} and @code{feupdateenv} to be generated at appropriate points in the compare-and-exchange sequence.  This hook should set @code{*@var{hold}} to an expression equivalent to the call to @code{feholdexcept}, @code{*@var{clear}} to an expression equivalent to the call to @code{feclearexcept} and @code{*@var{update}} to an expression equivalent to the call to @code{feupdateenv}.  The three expressions are @code{NULL_TREE} on entry to the hook and may be left as @code{NULL_TREE} if no code is required in a particular place.  The default implementation leaves all three expressions as @code{NULL_TREE}.  The @code{__atomic_feraiseexcept} function from @code{libatomic} may be of use as part of the code generated in @code{*@var{update}}.
 @end deftypefn
 
+@deftypefn {Target Hook} {unsigned int} TARGET_MAX_ALIGN_T_ALIGN (void)
+If defined, this function returns the alignment in bits for std:max_align_t. The default is suitable for most targets.  
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_RECORD_OFFLOAD_SYMBOL (tree)
 Used when offloaded functions are seen in the compilation unit and no named
 sections are available.  It is called once for each symbol that must be
Index: doc/tm.texi.in
===================================================================
--- doc/tm.texi.in	(revision 240688)
+++ doc/tm.texi.in	(working copy)
@@ -8224,6 +8224,8 @@
 
 @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 
+@hook TARGET_MAX_ALIGN_T_ALIGN
+
 @hook TARGET_RECORD_OFFLOAD_SYMBOL
 
 @hook TARGET_OFFLOAD_OPTIONS
Index: target.def
===================================================================
--- target.def	(revision 240688)
+++ target.def	(working copy)
@@ -5894,6 +5894,15 @@
  void, (tree *hold, tree *clear, tree *update),
  default_atomic_assign_expand_fenv)
 
+/* Return an unsigned int representing the alignment (in bits) of
+   std:max_align_t.  This allows the default alignment to be overridden.  */
+DEFHOOK
+(max_align_t_align,
+"If defined, this function returns the alignment in bits for std:max_align_t.\
+ The default is suitable for most targets.  ",
+ unsigned int, (void),
+ default_max_align_t_align)
+
 /* Leave the boolean fields at the end.  */
 
 /* True if we can create zeroed data by switching to a BSS section
Index: targhooks.c
===================================================================
--- targhooks.c	(revision 240688)
+++ targhooks.c	(working copy)
@@ -1907,6 +1907,23 @@
 {
 }
 
+/* Default implementation of TARGET_MAX_ALIGN_T_ALIGN to return alignment
+   in bits of std::max_align_t.
+
+   [support.types.layout] The type max_align_t is a POD type whose alignment
+   requirement is at least as great as that of every scalar type, and whose
+   alignment requirement is supported in every context.  */
+
+unsigned int
+default_max_align_t_align (void)
+{
+  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
+                                TYPE_ALIGN (long_double_type_node));
+  if (float128_type_node != NULL_TREE)
+    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
+  return max_align;
+}
+
 #ifndef PAD_VARARGS_DOWN
 #define PAD_VARARGS_DOWN BYTES_BIG_ENDIAN
 #endif
Index: targhooks.h
===================================================================
--- targhooks.h	(revision 240688)
+++ targhooks.h	(working copy)
@@ -262,5 +262,6 @@
 				       optimization_type);
 
 extern unsigned int default_max_noce_ifcvt_seq_cost (edge);
+extern unsigned int default_max_align_t_align (void);
 
 #endif /* GCC_TARGHOOKS_H */

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2017-02-25, at 5:32 PM, Gerald Pfeifer wrote:

> On Sat, 25 Feb 2017, John David Anglin wrote:
>>> Sooo, why not deprecate 32-bit HP-UX with GCC 7?
>> GCC 7 is in reasonable shape on 32-bit HP-UX.  I'll do it after it is 
>> released.
> 
> If you deprecate it with GCC 7, it'd be only removed with GCC 8.
> 
> On the other hand, if you deprecate it past GCC 7, removal would 
> happen with GCC 9 (by default).


Exactly.  I'm happy with that and don't want to require --enable-obsolete to build GCC 7.

Dave
--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Gerald Pfeifer]

On Sat, 25 Feb 2017, John David Anglin wrote:
>> Sooo, why not deprecate 32-bit HP-UX with GCC 7?
> GCC 7 is in reasonable shape on 32-bit HP-UX.  I'll do it after it is 
> released.

If you deprecate it with GCC 7, it'd be only removed with GCC 8.

On the other hand, if you deprecate it past GCC 7, removal would 
happen with GCC 9 (by default).

Gerald

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2017-02-25, at 12:25 PM, Gerald Pfeifer wrote:

> On Wed, 12 Oct 2016, John David Anglin wrote:
>>> It's really hard to make an argument to do anything other than deprecate
>>> that platform.  Given John's ongoing involvement it's much harder to
>>> deprecate 64bit linux (and consequently 64bit hpux).
>> I believe that deprecating the 32-bit HP-UX platform makes sense. There 
>> is no HP involvement in hppa, ia64 or alpha open source that I am aware 
>> of, so deprecating gcc platforms with HP systems is reasonable.
>> 
>> My primary focus is open source and one less platform will free some time.
> 
> Sooo, why not deprecate 32-bit HP-UX with GCC 7?

GCC 7 is in reasonable shape on 32-bit HP-UX.  I'll do it after it is released.

Dave
--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Gerald Pfeifer]

On Wed, 12 Oct 2016, John David Anglin wrote:
>> It's really hard to make an argument to do anything other than deprecate
>> that platform.  Given John's ongoing involvement it's much harder to
>> deprecate 64bit linux (and consequently 64bit hpux).
> I believe that deprecating the 32-bit HP-UX platform makes sense. There 
> is no HP involvement in hppa, ia64 or alpha open source that I am aware 
> of, so deprecating gcc platforms with HP systems is reasonable.
> 
> My primary focus is open source and one less platform will free some time.

Sooo, why not deprecate 32-bit HP-UX with GCC 7?

Now is still a good time doing so, from the release perspective,
and if you as the maintainer (and pretty much only person active
here) are in favor...

Gerald

Re: [PATCH] Implement new hook for max_align_t_align

[Carlos O'Donell]

On 10/12/2016 10:17 AM, John David Anglin wrote:
> On 2016-10-12 9:48 AM, Jason Merrill wrote:
>> On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
>>>> dropping the alignment means that the padding before the lock member
>>>> vanishes.  Consequently, we have just created a silent ABI change in
>>>> application code, which is a big no-no.
>>> Sure, it would be an ABI change, but how many users would it affect?
>>>
>>>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>>>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
>>> Or just drop support for a dead arch?
>>>
>>>> instead.  Sure, it is a bit inefficient, but this will only affect PA-RISC
>>>> users.  It does not even cause work for PA-RISC porters. Conversely, if we
>>>> work on this to come up with a different fix, many more people will be
>>>> affected (because they don't get all the nice things we could work on
>>>> instead), and we may need to maintain a special GCC kludge for the
>>>> alternative solution, impacting GCC developers in particular.
>>> But sure, bumping malloc alignment is probably easiest.  And people who want
>>> performance have better options than to stay on 32-bit PA-RISC anyway.
>> Or we could do nothing and tell people to ignore the harmless warning.
> The warning is an issue because of -Werror.  However, it appears easy to suppress it in the PA
> backend.  I have a patch that I'm testing.
> 
> We are discussing offline regarding the glibc issue.  It easy to bump the alignment of malloc
> but I take Jakub's point and maybe we should break the ABI.  Debian unstable churns
> quickly, and I think we would be better off being consistent with the current max_align_t
> and 8-byte aligned malloc.

I am against breaking the ABI.

I would rather see us bump malloc alignment up to 16-bytes.

The last time I changed this alignment it _immediately_ broken libstdc++ boostrap
because it's using exactly the kind of embedded pthread_mutext_t we're talking about
breaking.

So in that case the debian builds in unstable broke right away, and I had to revert
the change. We'd have to BinNMU a bunch of things to get this working again.

Again I think our two options are, in my order of preference:

- Disable the warning via a PA backend change.
- Bump malloc alignment.

I am sensitive to the first change being something that carries with it extra
maintenance burden, so I'm happy to see the second solution chosen if that's what
everyone wants (Florian's suggestion).

-- 
Cheers,
Carlos.

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-12 2:01 PM, Florian Weimer wrote:
> On 10/12/2016 06:14 PM, Jeff Law wrote:
>
>> If the issue is strictly limited to 32 bit hpux, then do we really care?
>>  Can we just deprecate that platform and thus make the issue go away?
>
> Are we talking about HP-XX or Linux on PA-RISC?
The patch was intended to address a Linux issue on PA-RISC.  On HP-UX, 
the current
version of max_align_t is fine.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Florian Weimer]

On 10/12/2016 06:14 PM, Jeff Law wrote:

> If the issue is strictly limited to 32 bit hpux, then do we really care?
>  Can we just deprecate that platform and thus make the issue go away?

Are we talking about HP-XX or Linux on PA-RISC?

Thanks,
Florian

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-12 12:14 PM, Jeff Law wrote:
> On 10/12/2016 02:02 AM, Jakub Jelinek wrote:
>> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
>>> dropping the alignment means that the padding before the lock member
>>> vanishes.  Consequently, we have just created a silent ABI change in
>>> application code, which is a big no-no.
>>
>> Sure, it would be an ABI change, but how many users would it affect?
>
>>
>>> Since this is PA-RISC, which is essentially dead (neither HPE nor 
>>> Debian
>>> ship it anymore), I stand by my suggestion to bump the fundamental 
>>> alignment
>>
>> Or just drop support for a dead arch?
>>
>>> instead.  Sure, it is a bit inefficient, but this will only affect 
>>> PA-RISC
>>> users.  It does not even cause work for PA-RISC porters. Conversely, 
>>> if we
>>> work on this to come up with a different fix, many more people will be
>>> affected (because they don't get all the nice things we could work on
>>> instead), and we may need to maintain a special GCC kludge for the
>>> alternative solution, impacting GCC developers in particular.
>>
>> But sure, bumping malloc alignment is probably easiest.  And people 
>> who want
>> performance have better options than to stay on 32-bit PA-RISC anyway.
> If the issue is strictly limited to 32 bit hpux, then do we really 
> care?  Can we just deprecate that platform and thus make the issue go 
> away?
>
> It's really hard to make an argument to do anything other than 
> deprecate that platform.  Given John's ongoing involvement it's much 
> harder to deprecate 64bit linux (and consequently 64bit hpux).

I believe that deprecating the 32-bit HP-UX platform makes sense. There 
is no HP involvement in hppa,
  ia64 or alpha open source that I am aware of, so deprecating gcc 
platforms with HP systems is reasonable.

My primary focus is open source and one less platform will free some 
time. We still need 64bit hpux for
64bit linux development.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Jeff Law]

On 10/12/2016 02:02 AM, Jakub Jelinek wrote:
> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
>> dropping the alignment means that the padding before the lock member
>> vanishes.  Consequently, we have just created a silent ABI change in
>> application code, which is a big no-no.
>
> Sure, it would be an ABI change, but how many users would it affect?

>
>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
>
> Or just drop support for a dead arch?
>
>> instead.  Sure, it is a bit inefficient, but this will only affect PA-RISC
>> users.  It does not even cause work for PA-RISC porters. Conversely, if we
>> work on this to come up with a different fix, many more people will be
>> affected (because they don't get all the nice things we could work on
>> instead), and we may need to maintain a special GCC kludge for the
>> alternative solution, impacting GCC developers in particular.
>
> But sure, bumping malloc alignment is probably easiest.  And people who want
> performance have better options than to stay on 32-bit PA-RISC anyway.
If the issue is strictly limited to 32 bit hpux, then do we really care? 
  Can we just deprecate that platform and thus make the issue go away?

It's really hard to make an argument to do anything other than deprecate 
that platform.  Given John's ongoing involvement it's much harder to 
deprecate 64bit linux (and consequently 64bit hpux).

Jeff

Re: [PATCH] Implement new hook for max_align_t_align

[Joseph Myers]

On Wed, 12 Oct 2016, Richard Biener wrote:

> I'd say what applies to PA should apply equally well to the pdp11 and
> the alpha port ...
> 
> But usually the question is just whether the port has a maintainer
> and/or whether it is
> a maintainance burden to keep it (say, last user of obsolete feature X).

Last users of obsolete feature "defines TARGET_HAVE_NAMED_SECTIONS to 
false": 32-bit PA HP-UX (64-bit HP-UX is ELF like GNU/Linux, so OK), 
pdp11, pre-ELF OpenBSD ports.  (I'm not aware of that obsolete feature 
particularly causing problems, however.  But the implication for pdp11 
would be that if the port were to stay with that feature being removed, it 
should move to ELF - the e_machine value EM_PDP11 having been allocated by 
Lars Brinkhoff on 30 May 2002, but I don't know if there's an ABI.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-12 9:48 AM, Jason Merrill wrote:
> On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
>>> dropping the alignment means that the padding before the lock member
>>> vanishes.  Consequently, we have just created a silent ABI change in
>>> application code, which is a big no-no.
>> Sure, it would be an ABI change, but how many users would it affect?
>>
>>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
>> Or just drop support for a dead arch?
>>
>>> instead.  Sure, it is a bit inefficient, but this will only affect PA-RISC
>>> users.  It does not even cause work for PA-RISC porters. Conversely, if we
>>> work on this to come up with a different fix, many more people will be
>>> affected (because they don't get all the nice things we could work on
>>> instead), and we may need to maintain a special GCC kludge for the
>>> alternative solution, impacting GCC developers in particular.
>> But sure, bumping malloc alignment is probably easiest.  And people who want
>> performance have better options than to stay on 32-bit PA-RISC anyway.
> Or we could do nothing and tell people to ignore the harmless warning.
The warning is an issue because of -Werror.  However, it appears easy to 
suppress it in the PA
backend.  I have a patch that I'm testing.

We are discussing offline regarding the glibc issue.  It easy to bump 
the alignment of malloc
but I take Jakub's point and maybe we should break the ABI.  Debian 
unstable churns
quickly, and I think we would be better off being consistent with the 
current max_align_t
and 8-byte aligned malloc.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Jason Merrill]

On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
>> dropping the alignment means that the padding before the lock member
>> vanishes.  Consequently, we have just created a silent ABI change in
>> application code, which is a big no-no.
>
> Sure, it would be an ABI change, but how many users would it affect?
>
>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
>
> Or just drop support for a dead arch?
>
>> instead.  Sure, it is a bit inefficient, but this will only affect PA-RISC
>> users.  It does not even cause work for PA-RISC porters. Conversely, if we
>> work on this to come up with a different fix, many more people will be
>> affected (because they don't get all the nice things we could work on
>> instead), and we may need to maintain a special GCC kludge for the
>> alternative solution, impacting GCC developers in particular.
>
> But sure, bumping malloc alignment is probably easiest.  And people who want
> performance have better options than to stay on 32-bit PA-RISC anyway.

Or we could do nothing and tell people to ignore the harmless warning.

Jason

Re: [PATCH] Implement new hook for max_align_t_align

[Bernd Schmidt]

On 10/12/2016 02:43 PM, Richard Biener wrote:
> I'd say what applies to PA should apply equally well to the pdp11 and
> the alpha port ...
>
> But usually the question is just whether the port has a maintainer
> and/or whether it is
> a maintainance burden to keep it (say, last user of obsolete feature X).

Well, we seem to be running into a problem with PA, and pdp11 is a cc0 port.


Bernd

Re: [PATCH] Implement new hook for max_align_t_align

[Richard Biener]

On Wed, Oct 12, 2016 at 2:33 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/12/2016 02:12 PM, John David Anglin wrote:
>>
>> On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote:
>>
>>>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>>>> ship it anymore), I stand by my suggestion to bump the fundamental
>>>> alignment
>>>
>>>
>>> Or just drop support for a dead arch?
>>
>>
>> Hardware is still available on the second hand market.
>
>
> So is the Commodore 64, but is that enough though to keep supporting PA in
> gcc? Anyone who wants to do retrocomputing can still use gcc-6 or earlier
> versions.

I'd say what applies to PA should apply equally well to the pdp11 and
the alpha port ...

But usually the question is just whether the port has a maintainer
and/or whether it is
a maintainance burden to keep it (say, last user of obsolete feature X).

Richard.

>
> Bernd

Re: [PATCH] Implement new hook for max_align_t_align

[Bernd Schmidt]

On 10/12/2016 02:12 PM, John David Anglin wrote:
> On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote:
>
>>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
>>
>> Or just drop support for a dead arch?
>
> Hardware is still available on the second hand market.

So is the Commodore 64, but is that enough though to keep supporting PA 
in gcc? Anyone who wants to do retrocomputing can still use gcc-6 or 
earlier versions.


Bernd

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote:

>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
>> ship it anymore), I stand by my suggestion to bump the fundamental alignment
> 
> Or just drop support for a dead arch?

Hardware is still available on the second hand market.

Linux is available from Debian although not as a release architecture:
https://buildd.debian.org/status/architecture.php?a=hppa&suite=sid
I know from personal use that it is more functional than it has ever been.

It is also still available from gentoo as far as I know.

Dave
--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Jakub Jelinek]

On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote:
> dropping the alignment means that the padding before the lock member
> vanishes.  Consequently, we have just created a silent ABI change in
> application code, which is a big no-no.

Sure, it would be an ABI change, but how many users would it affect?

> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
> ship it anymore), I stand by my suggestion to bump the fundamental alignment

Or just drop support for a dead arch?

> instead.  Sure, it is a bit inefficient, but this will only affect PA-RISC
> users.  It does not even cause work for PA-RISC porters. Conversely, if we
> work on this to come up with a different fix, many more people will be
> affected (because they don't get all the nice things we could work on
> instead), and we may need to maintain a special GCC kludge for the
> alternative solution, impacting GCC developers in particular.

But sure, bumping malloc alignment is probably easiest.  And people who want
performance have better options than to stay on 32-bit PA-RISC anyway.

	Jakub

Re: [PATCH] Implement new hook for max_align_t_align

[Florian Weimer]

On 10/12/2016 09:25 AM, Jakub Jelinek wrote:

> No, you can just drop the aligned attributes for HPUX 32-bit, basically
> introduce a new ABI.  If needed, you could add new symbol versions for
> pthread_mutex_* etc. (though, if the current code doesn't care about the
> alignment, perhaps you could get away without bumping that).

This is not something which can be solved with symbol versioning.  It is 
fairly common to embed mutexes into other objects, like this:

   struct client {
     pthread_mutex_t lock;
     struct client *next;
     size_t attachment_count;
   };

The layout above is fine with the alignment change, but if the 
programmer writes this instead:

   struct client {
     struct client *next;
     pthread_mutex_t lock;
     size_t attachment_count;
   };

dropping the alignment means that the padding before the lock member 
vanishes.  Consequently, we have just created a silent ABI change in 
application code, which is a big no-no.

Since this is PA-RISC, which is essentially dead (neither HPE nor Debian 
ship it anymore), I stand by my suggestion to bump the fundamental 
alignment instead.  Sure, it is a bit inefficient, but this will only 
affect PA-RISC users.  It does not even cause work for PA-RISC porters. 
Conversely, if we work on this to come up with a different fix, many 
more people will be affected (because they don't get all the nice things 
we could work on instead), and we may need to maintain a special GCC 
kludge for the alternative solution, impacting GCC developers in particular.

Thanks,
Florian

Re: [PATCH] Implement new hook for max_align_t_align

[Jakub Jelinek]

On Wed, Oct 12, 2016 at 03:01:51AM -0400, Carlos O'Donell wrote:
> On 10/11/2016 04:04 PM, John David Anglin wrote:
> > On 2016-10-11 2:50 PM, Jason Merrill wrote:
> >> /* Alignment, in bits, a C conformant malloc implementation has to
> >> provide.
> >>     The HP-UX malloc implementation provides a default alignment of 8
> >> bytes.
> >>     This can be increased with mallopt.  The glibc implementation also
> >> provides
> >>     8-byte alignment.  Note that this isn't enough for various POSIX
> >> types such
> >>     as pthread_mutex_t.  However, since we no longer need the 16-byte
> >> alignment
> >>     for atomic operations, we ignore the nominal alignment specified
> >> for these
> >>     types.  The same is true for long double on 64-bit HP-UX.  */
> >>
> >> If PA malloc doesn't actually provide 16-byte alignment, this change
> >> seems problematic; it will mean any type that wants 16-byte alignment
> >> will silently get 8-byte alignment instead.
> > 
> > I agree the situation is something of a mess.  On linux, we could bump the alignment
> > of malloc to 16-bytes.  However, Carlos argued that we don't need to and I think doing
> > so would be detrimental to performance.
> 
> Correct, we do not need a 16-byte alignment at runtime.

The problem with cheating is that gcc will then assume the structure is
properly aligned and optimize based on that (optimize away alignment checks
etc.).
> 
> > The 16-byte alignment was used originally because the ldcw instruction used for atomic
> > operations in linux threads needs 16-byte alignment.  However, the nptl pthread
> > implementation now uses a kernel helper for atomic operations.  It only needs
> > 4-byte alignment.  The largest alignment actually needed is for long double (8 bytes).
> > However, we can't change the 16-byte alignment without affecting the layout of various
> > structures.
> 
> Correct, the structure padding needs to continue to be there to match the original ABI.

No, you can just drop the aligned attributes for HPUX 32-bit, basically
introduce a new ABI.  If needed, you could add new symbol versions for
pthread_mutex_* etc. (though, if the current code doesn't care about the
alignment, perhaps you could get away without bumping that).

> I think the case where a user specifically requests a larger aligment is still always
> bound to fail if they exceed malloc's aligment. So removing the warning just leaves

If users use posix_memalign/memalign/aligned_alloc or the C++17 aligned new,
they should be fine.

	Jakub

Re: [PATCH] Implement new hook for max_align_t_align

[Carlos O'Donell]

On 10/11/2016 04:04 PM, John David Anglin wrote:
> On 2016-10-11 2:50 PM, Jason Merrill wrote:
>> /* Alignment, in bits, a C conformant malloc implementation has to
>> provide.
>>     The HP-UX malloc implementation provides a default alignment of 8
>> bytes.
>>     This can be increased with mallopt.  The glibc implementation also
>> provides
>>     8-byte alignment.  Note that this isn't enough for various POSIX
>> types such
>>     as pthread_mutex_t.  However, since we no longer need the 16-byte
>> alignment
>>     for atomic operations, we ignore the nominal alignment specified
>> for these
>>     types.  The same is true for long double on 64-bit HP-UX.  */
>>
>> If PA malloc doesn't actually provide 16-byte alignment, this change
>> seems problematic; it will mean any type that wants 16-byte alignment
>> will silently get 8-byte alignment instead.
> 
> I agree the situation is something of a mess.  On linux, we could bump the alignment
> of malloc to 16-bytes.  However, Carlos argued that we don't need to and I think doing
> so would be detrimental to performance.

Correct, we do not need a 16-byte alignment at runtime.

> The 16-byte alignment was used originally because the ldcw instruction used for atomic
> operations in linux threads needs 16-byte alignment.  However, the nptl pthread
> implementation now uses a kernel helper for atomic operations.  It only needs
> 4-byte alignment.  The largest alignment actually needed is for long double (8 bytes).
> However, we can't change the 16-byte alignment without affecting the layout of various
> structures.

Correct, the structure padding needs to continue to be there to match the original ABI.

> The same is true for long double on HPUX.  Originally, it was planned to implement it
> in hardware and that would have required 16-byte alignment.  It was only implemented
> in software with an 8-byte alignment requirement.  Somehow, it ended up with 8 and
> 16-byte alignment in the HP 32 and 64-bit compilers, respectively. In both cases, malloc
> has 8-byte alignment.  It is possible to increase the "grain" size of HP malloc to 16 bytes.
> 
> Thus, I don't think the silent reduction to 8-byte alignment matters.  Without the change,
> we are faced with spurious warnings from new.

I suggested disabling the warnings.
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01445.html

Which is what Jason suggests here:
https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00786.html

Though Florian Weimer suggests just bumping malloc to return 16-byte aligned objects and
raising max_align_t, since conceptually that's simple (but will impact performance):
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01446.html

I think the case where a user specifically requests a larger aligment is still always
bound to fail if they exceed malloc's aligment. So removing the warning just leaves
hppa where it is today. No warning. Working with existing malloc alignment. But unable
to warn users of legitimate cases where this might matter?

I still suggest disabling the warning.

-- 
Cheers,
Carlos.

Re: [PATCH] Implement new hook for max_align_t_align

[Jason Merrill]

On Tue, Oct 11, 2016 at 4:54 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 2016-10-11 4:11 PM, Jason Merrill wrote:
>>
>> On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie <dj@redhat.com> wrote:
>>>
>>> Jason Merrill <jason@redhat.com> writes:
>>>>
>>>> If PA malloc doesn't actually provide 16-byte alignment, this change
>>>> seems problematic; it will mean any type that wants 16-byte alignment
>>>> will silently get 8-byte alignment instead.
>>>
>>> Should such cases be calling memalign (or posix_memalign) instead of
>>> malloc?
>>
>> We're talking about this in the context of C++17 aligned new, which
>> uses one of those functions (or C aligned_alloc) under the hood.
>> Currently on PA, allocating one of these types with 'new' in C++14
>> mode gives a warning because the compiler doesn't think the allocation
>> will actually provide the 16-byte alignment that the type wants.  This
>> warning seems to be correct.  This patch would silence that warning by
>> pretending that malloc will provide 16-byte alignment, which is not
>> actually true.
>
> But the check isn't directly about malloc.  It's between the alignment for
> max_align_t and the alignment that the type wants.  Joseph indicated that max_align_t
> should have an alignment as large as the POSIX types (e.g., pthread_mutex_t).  So, I think
> setting it to 16 when pthread_mutex_t wants an alignment of 16 is correct.

The connection to max_align_t is just a GCC implementation detail; the
C++ standard doesn't tie use of aligned new to alignof(max_align_t),
there's a separate macro __STDCPP_DEFAULT_NEW_ALIGNMENT__.  This was
done specifically to allow for the case where malloc for a target
provides more alignment than max_align_t.

But that still doesn't help with this case, where in fact the warning
is correct, you just know it isn't a problem.  So disabling the
warning on PA seems like the way to go.

Jason

Re: [PATCH] Implement new hook for max_align_t_align

[Jakub Jelinek]

On Tue, Oct 11, 2016 at 04:54:56PM -0400, John David Anglin wrote:
> But the check isn't directly about malloc.  It's between the alignment for
> max_align_t
> and the alignment that the type wants.  Joseph indicated that max_align_t
> should have an
> alignment as large as the POSIX types (e.g., pthread_mutex_t).  So, I think
> setting it to 16
> when pthread_mutex_t wants an alignment of 16 is correct.

But precondition of bumping max_align_t to 16 is IMHO that malloc actually
provides that alignment.  If that is not the case, then I think you should
just drop the alignment attribute from pthread_mutex_t etc. and deal with
the ABI change.

	Jakub

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-11 4:11 PM, Jason Merrill wrote:
> On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie <dj@redhat.com> wrote:
>> Jason Merrill <jason@redhat.com> writes:
>>> If PA malloc doesn't actually provide 16-byte alignment, this change
>>> seems problematic; it will mean any type that wants 16-byte alignment
>>> will silently get 8-byte alignment instead.
>> Should such cases be calling memalign (or posix_memalign) instead of
>> malloc?
> We're talking about this in the context of C++17 aligned new, which
> uses one of those functions (or C aligned_alloc) under the hood.
> Currently on PA, allocating one of these types with 'new' in C++14
> mode gives a warning because the compiler doesn't think the allocation
> will actually provide the 16-byte alignment that the type wants.  This
> warning seems to be correct.  This patch would silence that warning by
> pretending that malloc will provide 16-byte alignment, which is not
> actually true.
But the check isn't directly about malloc.  It's between the alignment 
for max_align_t
and the alignment that the type wants.  Joseph indicated that 
max_align_t should have an
alignment as large as the POSIX types (e.g., pthread_mutex_t).  So, I 
think setting it to 16
when pthread_mutex_t wants an alignment of 16 is correct.

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Jason Merrill]

On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie <dj@redhat.com> wrote:
>
> Jason Merrill <jason@redhat.com> writes:
>> If PA malloc doesn't actually provide 16-byte alignment, this change
>> seems problematic; it will mean any type that wants 16-byte alignment
>> will silently get 8-byte alignment instead.
>
> Should such cases be calling memalign (or posix_memalign) instead of
> malloc?

We're talking about this in the context of C++17 aligned new, which
uses one of those functions (or C aligned_alloc) under the hood.
Currently on PA, allocating one of these types with 'new' in C++14
mode gives a warning because the compiler doesn't think the allocation
will actually provide the 16-byte alignment that the type wants.  This
warning seems to be correct.  This patch would silence that warning by
pretending that malloc will provide 16-byte alignment, which is not
actually true.

It seems to me that the warning is correct, but not a problem in this
case, so perhaps turning the warning off by default on PA is the right
solution.

Jason

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-11 2:50 PM, Jason Merrill wrote:
> /* Alignment, in bits, a C conformant malloc implementation has to
> provide.
>     The HP-UX malloc implementation provides a default alignment of 8
> bytes.
>     This can be increased with mallopt.  The glibc implementation also
> provides
>     8-byte alignment.  Note that this isn't enough for various POSIX
> types such
>     as pthread_mutex_t.  However, since we no longer need the 16-byte
> alignment
>     for atomic operations, we ignore the nominal alignment specified
> for these
>     types.  The same is true for long double on 64-bit HP-UX.  */
>
> If PA malloc doesn't actually provide 16-byte alignment, this change
> seems problematic; it will mean any type that wants 16-byte alignment
> will silently get 8-byte alignment instead.

I agree the situation is something of a mess.  On linux, we could bump 
the alignment
of malloc to 16-bytes.  However, Carlos argued that we don't need to and 
I think doing
so would be detrimental to performance.

The 16-byte alignment was used originally because the ldcw instruction 
used for atomic
operations in linux threads needs 16-byte alignment.  However, the nptl 
pthread
implementation now uses a kernel helper for atomic operations.  It only 
needs
4-byte alignment.  The largest alignment actually needed is for long 
double (8 bytes).
However, we can't change the 16-byte alignment without affecting the 
layout of various
structures.

The same is true for long double on HPUX.  Originally, it was planned to 
implement it
in hardware and that would have required 16-byte alignment.  It was only 
implemented
in software with an 8-byte alignment requirement.  Somehow, it ended up 
with 8 and
16-byte alignment in the HP 32 and 64-bit compilers, respectively. In 
both cases, malloc
has 8-byte alignment.  It is possible to increase the "grain" size of HP 
malloc to 16 bytes.

Thus, I don't think the silent reduction to 8-byte alignment matters.  
Without the change,
we are faced with spurious warnings from new.

--
  John David Anglin dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[DJ Delorie]

Jason Merrill <jason@redhat.com> writes:
> If PA malloc doesn't actually provide 16-byte alignment, this change
> seems problematic; it will mean any type that wants 16-byte alignment
> will silently get 8-byte alignment instead.

Should such cases be calling memalign (or posix_memalign) instead of
malloc?

Re: [PATCH] Implement new hook for max_align_t_align

[Jason Merrill]

/* Alignment, in bits, a C conformant malloc implementation has to
provide.
   The HP-UX malloc implementation provides a default alignment of 8
bytes.
   This can be increased with mallopt.  The glibc implementation also
provides
   8-byte alignment.  Note that this isn't enough for various POSIX
types such
   as pthread_mutex_t.  However, since we no longer need the 16-byte
alignment
   for atomic operations, we ignore the nominal alignment specified
for these
   types.  The same is true for long double on 64-bit HP-UX.  */

If PA malloc doesn't actually provide 16-byte alignment, this change
seems problematic; it will mean any type that wants 16-byte alignment
will silently get 8-byte alignment instead.

Jason

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

Attached is an updated version using the new builtin __MAX_ALIGN_T_ALIGN__.  This
simplifies the declaration of max_align_t and ensures it is always the same as max_align_t_align().

Tested on hppa-unknown-linux-gnu.  Okay for trunk?

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: ldcw-align.d-v3.txt --]
[-- Type: text/plain, Size: 10362 bytes --]

2016-10-10  John David Anglin  <danglin@gcc.gnu.org>

gcc/c-family/
	* c-common.c (c_stddef_cpp_builtins): Add __MAX_ALIGN_T_ALIGN__ builtin
	define.
	(max_align_t_align): Move to targhooks.c.
	* c-common.h (max_align_t_align): Delete.
gcc/
	* target.def (max_align_t_align): New target hook.
	* targhooks.c (default_max_align_t_align): New.
	* targhooks.h (default_max_align_t_align): Declare.
	* config/pa/pa.c (pa_max_align_t_align): New.
	(TARGET_MAX_ALIGN_T_ALIGN): Define.
	* ginclude/stddef.h (max_align_t): Use __MAX_ALIGN_T_ALIGN__ builtin.
	* doc/tm.texi.in (TARGET_MAX_ALIGN_T_ALIGN): Add documentation hook.
	* doc/tm.texi: Update.
gcc/cp/
	* decl.c (cxx_init_decl_processing): Use max_align_t_align target hook.
	* init.c (build_new_1): Likewise.

Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 240901)
+++ c-family/c-common.c	(working copy)
@@ -6683,6 +6683,8 @@
     builtin_define_with_value ("__INTPTR_TYPE__", INTPTR_TYPE, 0);
   if (UINTPTR_TYPE)
     builtin_define_with_value ("__UINTPTR_TYPE__", UINTPTR_TYPE, 0);
+  builtin_define_with_int_value ("__MAX_ALIGN_T_ALIGN__",
+				 targetm.max_align_t_align () / BITS_PER_UNIT);
 }
 
 static void
@@ -12925,22 +12927,6 @@
   return stv_nothing;
 }
 
-/* Return the alignment of std::max_align_t.
-
-   [support.types.layout] The type max_align_t is a POD type whose alignment
-   requirement is at least as great as that of every scalar type, and whose
-   alignment requirement is supported in every context.  */
-
-unsigned
-max_align_t_align ()
-{
-  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
-				TYPE_ALIGN (long_double_type_node));
-  if (float128_type_node != NULL_TREE)
-    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
-  return max_align;
-}
-
 /* Return true iff ALIGN is an integral constant that is a fundamental
    alignment, as defined by [basic.align] in the c++-11
    specifications.
@@ -12954,7 +12940,7 @@
 bool
 cxx_fundamental_alignment_p (unsigned align)
 {
-  return (align <= max_align_t_align ());
+  return (align <= targetm.max_align_t_align ());
 }
 
 /* Return true if T is a pointer to a zero-sized aggregate.  */
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 240901)
+++ c-family/c-common.h	(working copy)
@@ -866,7 +866,6 @@
 extern bool keyword_is_storage_class_specifier (enum rid);
 extern bool keyword_is_type_qualifier (enum rid);
 extern bool keyword_is_decl_specifier (enum rid);
-extern unsigned max_align_t_align (void);
 extern bool cxx_fundamental_alignment_p (unsigned);
 extern bool pointer_to_zero_sized_aggr_p (tree);
 extern bool diagnose_mismatched_attributes (tree, tree);
Index: config/pa/pa.c
===================================================================
--- config/pa/pa.c	(revision 240901)
+++ config/pa/pa.c	(working copy)
@@ -194,6 +194,7 @@
 static bool pa_legitimate_constant_p (machine_mode, rtx);
 static unsigned int pa_section_type_flags (tree, const char *, int);
 static bool pa_legitimate_address_p (machine_mode, rtx, bool);
+static unsigned int pa_max_align_t_align (void);
 
 /* The following extra sections are only used for SOM.  */
 static GTY(()) section *som_readonly_data_section;
@@ -400,6 +401,9 @@
 #undef TARGET_LRA_P
 #define TARGET_LRA_P hook_bool_void_false
 
+#undef TARGET_MAX_ALIGN_T_ALIGN
+#define TARGET_MAX_ALIGN_T_ALIGN pa_max_align_t_align
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 /* Parse the -mfixed-range= option string.  */
@@ -10719,4 +10723,16 @@
   return NULL_RTX;
 }
 
+/* The maximimum alignment in bits for the POD type std:max_align_t.
+   This is set to 128 on 32-bit non HP-UX systems to suppress warnings
+   about new with extended alignment.  This arises because various POSIX
+   types such as pthread_mutex_t have for historical reasons 128-bit
+   alignment but the default alignment of std:max_align_t is 64 bits.  */
+
+static unsigned int
+pa_max_align_t_align (void)
+{
+  return TARGET_HPUX && !TARGET_64BIT ? 64 : 128;
+}
+
 #include "gt-pa.h"
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 240901)
+++ cp/decl.c	(working copy)
@@ -4082,7 +4082,7 @@
   if (aligned_new_threshold == -1)
     aligned_new_threshold = (cxx_dialect >= cxx1z) ? 1 : 0;
   if (aligned_new_threshold == 1)
-    aligned_new_threshold = max_align_t_align () / BITS_PER_UNIT;
+    aligned_new_threshold = targetm.max_align_t_align () / BITS_PER_UNIT;
 
   {
     tree newattrs, extvisattr;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 240901)
+++ cp/init.c	(working copy)
@@ -2975,7 +2975,7 @@
   gcc_assert (alloc_fn != NULL_TREE);
 
   if (warn_aligned_new
-      && TYPE_ALIGN (elt_type) > max_align_t_align ()
+      && TYPE_ALIGN (elt_type) > targetm.max_align_t_align ()
       && (warn_aligned_new > 1
 	  || CP_DECL_CONTEXT (alloc_fn) == global_namespace)
       && !aligned_allocation_fn_p (alloc_fn))
Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 240901)
+++ doc/tm.texi	(working copy)
@@ -11680,6 +11680,10 @@
 ISO C11 requires atomic compound assignments that may raise floating-point exceptions to raise exceptions corresponding to the arithmetic operation whose result was successfully stored in a compare-and-exchange sequence.  This requires code equivalent to calls to @code{feholdexcept}, @code{feclearexcept} and @code{feupdateenv} to be generated at appropriate points in the compare-and-exchange sequence.  This hook should set @code{*@var{hold}} to an expression equivalent to the call to @code{feholdexcept}, @code{*@var{clear}} to an expression equivalent to the call to @code{feclearexcept} and @code{*@var{update}} to an expression equivalent to the call to @code{feupdateenv}.  The three expressions are @code{NULL_TREE} on entry to the hook and may be left as @code{NULL_TREE} if no code is required in a particular place.  The default implementation leaves all three expressions as @code{NULL_TREE}.  The @code{__atomic_feraiseexcept} function from @code{libatomic} may be of use as part of the code generated in @code{*@var{update}}.
 @end deftypefn
 
+@deftypefn {Target Hook} {unsigned int} TARGET_MAX_ALIGN_T_ALIGN (void)
+If defined, this function returns the alignment in bits for std:max_align_t. The default is suitable for most targets.  
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_RECORD_OFFLOAD_SYMBOL (tree)
 Used when offloaded functions are seen in the compilation unit and no named
 sections are available.  It is called once for each symbol that must be
Index: doc/tm.texi.in
===================================================================
--- doc/tm.texi.in	(revision 240901)
+++ doc/tm.texi.in	(working copy)
@@ -8218,6 +8218,8 @@
 
 @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 
+@hook TARGET_MAX_ALIGN_T_ALIGN
+
 @hook TARGET_RECORD_OFFLOAD_SYMBOL
 
 @hook TARGET_OFFLOAD_OPTIONS
Index: ginclude/stddef.h
===================================================================
--- ginclude/stddef.h	(revision 240901)
+++ ginclude/stddef.h	(working copy)
@@ -424,16 +424,7 @@
    as great as that of any standard type not using alignment
    specifiers.  */
 typedef struct {
-  long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
-  long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
-  /* _Float128 is defined as a basic type, so max_align_t must be
-     sufficiently aligned for it.  This code must work in C++, so we
-     use __float128 here; that is only available on some
-     architectures, but only on i386 is extra alignment needed for
-     __float128.  */
-#ifdef __i386__
-  __float128 __max_align_f128 __attribute__((__aligned__(__alignof(__float128))));
-#endif
+  char __max_align[0] __attribute__((__aligned__(__MAX_ALIGN_T_ALIGN__)));
 } max_align_t;
 #endif
 #endif /* C11 or C++11.  */
Index: target.def
===================================================================
--- target.def	(revision 240901)
+++ target.def	(working copy)
@@ -5894,6 +5894,15 @@
  void, (tree *hold, tree *clear, tree *update),
  default_atomic_assign_expand_fenv)
 
+/* Return an unsigned int representing the alignment (in bits) of
+   std:max_align_t.  This allows the default alignment to be overridden.  */
+DEFHOOK
+(max_align_t_align,
+"If defined, this function returns the alignment in bits for std:max_align_t.\
+ The default is suitable for most targets.  ",
+ unsigned int, (void),
+ default_max_align_t_align)
+
 /* Leave the boolean fields at the end.  */
 
 /* True if we can create zeroed data by switching to a BSS section
Index: targhooks.c
===================================================================
--- targhooks.c	(revision 240901)
+++ targhooks.c	(working copy)
@@ -1923,6 +1923,23 @@
 {
 }
 
+/* Default implementation of TARGET_MAX_ALIGN_T_ALIGN to return alignment
+   in bits of std::max_align_t.
+
+   [support.types.layout] The type max_align_t is a POD type whose alignment
+   requirement is at least as great as that of every scalar type, and whose
+   alignment requirement is supported in every context.  */
+
+unsigned int
+default_max_align_t_align (void)
+{
+  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
+                                TYPE_ALIGN (long_double_type_node));
+  if (float128_type_node != NULL_TREE)
+    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
+  return max_align;
+}
+
 #ifndef PAD_VARARGS_DOWN
 #define PAD_VARARGS_DOWN BYTES_BIG_ENDIAN
 #endif
Index: targhooks.h
===================================================================
--- targhooks.h	(revision 240901)
+++ targhooks.h	(working copy)
@@ -262,5 +262,6 @@
 				       optimization_type);
 
 extern unsigned int default_max_noce_ifcvt_seq_cost (edge);
+extern unsigned int default_max_align_t_align (void);
 
 #endif /* GCC_TARGHOOKS_H */

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

On 2016-10-09, at 4:34 AM, Bernd Edlinger wrote:

> For instance have:
> 
> typedef struct {
>   char __max_align[0] __attribute__((__aligned__(__MAX_ALIGN_T_ALIGN__)));
> } max_align_t;
> 
> Provided we do:
> 
> builtin_define_with_value ("__MAX_ALIGN_T_ALIGN__",
> targetm.max_align_t_align () / BITS_PER_UNIT);
> 
> Would'nt that guarantee, that __max_align_t and
> max_align_t_align () will always be the same?

Yes, excellent suggestion.  Testing.

--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] Implement new hook for max_align_t_align

[Bernd Edlinger]

On 10/08/16 19:36, John David Anglin wrote:
> On 2016-10-08, at 1:01 PM, Bernd Edlinger wrote:
>
>> I think your callback should also directly control the
>> alignment of max_align_t in stddef.h:
>>
>>
>>    long long __max_align_ll __attribute__((__aligned__(__alignof__(long
>> long))));
>>    long double __max_align_ld
>> __attribute__((__aligned__(__alignof__(long double))));
>>    /* _Float128 is defined as a basic type, so max_align_t must be
>>       sufficiently aligned for it.  This code must work in C++, so we
>>       use __float128 here; that is only available on some
>>       architectures, but only on i386 is extra alignment needed for
>>       __float128.  */
>> #ifdef __i386__
>>    __float128 __max_align_f128
>> __attribute__((__aligned__(__alignof(__float128))));
>> #endif
>> } max_align_t;
>>
>>
>> otherwise these will not match.
>
> Yes, i missed a hunk in the submission.  On hpux, the alignment is determined by the long
> double field.  With glibc, we need 16 byte alignment for max_align_t.
>

This looks still brittle to me.

I mean also the defines __hppa__ and __hpux__ come from
builtin_define calls in the backend, why not define
something with builtin_define_with_int_value,
which can be used as is in max_align_t.

For instance have:

typedef struct {
   char __max_align[0] __attribute__((__aligned__(__MAX_ALIGN_T_ALIGN__)));
} max_align_t;

Provided we do:

builtin_define_with_value ("__MAX_ALIGN_T_ALIGN__",
targetm.max_align_t_align () / BITS_PER_UNIT);

Would'nt that guarantee, that __max_align_t and
max_align_t_align () will always be the same?


Bernd.

Re: [PATCH] Implement new hook for max_align_t_align

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On 2016-10-08, at 1:01 PM, Bernd Edlinger wrote:

> I think your callback should also directly control the
> alignment of max_align_t in stddef.h:
> 
> typedef struct {
>   long long __max_align_ll __attribute__((__aligned__(__alignof__(long 
> long))));
>   long double __max_align_ld 
> __attribute__((__aligned__(__alignof__(long double))));
>   /* _Float128 is defined as a basic type, so max_align_t must be
>      sufficiently aligned for it.  This code must work in C++, so we
>      use __float128 here; that is only available on some
>      architectures, but only on i386 is extra alignment needed for
>      __float128.  */
> #ifdef __i386__
>   __float128 __max_align_f128 
> __attribute__((__aligned__(__alignof(__float128))));
> #endif
> } max_align_t;
> 
> 
> otherwise these will not match.

Yes, i missed a hunk in the submission.  On hpux, the alignment is determined by the long
double field.  With glibc, we need 16 byte alignment for max_align_t.

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: ldcw-align.d-v2.txt --]
[-- Type: text/plain, Size: 11276 bytes --]

2016-10-08  John David Anglin  <danglin@gcc.gnu.org>

gcc/c-family/
	* c-common.c (max_align_t_align): Move to targhooks.c.
	* c-common.h (max_align_t_align): Delete.
gcc/
	* target.def (max_align_t_align): New target hook.
	* targhooks.c (default_max_align_t_align): New.
	* targhooks.h (default_max_align_t_align): Declare.
	* config/pa/pa.h (BIGGEST_ALIGNMENT): Adjust comment.
	(MALLOC_ABI_ALIGNMENT): Define.
	* config/pa/pa.c (pa_max_align_t_align): New.
	(TARGET_MAX_ALIGN_T_ALIGN): Define.
	* ginclude/stddef.h (max_align_t): Align to 16 bytes on non-hpux hppa.
	* doc/tm.texi.in (TARGET_MAX_ALIGN_T_ALIGN): Add documentation hook.
	* doc/tm.texi: Update.
gcc/cp/
	* decl.c (cxx_init_decl_processing): Use max_align_t_align target hook.
	* init.c (build_new_1): Likewise.

Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 240688)
+++ c-family/c-common.c	(working copy)
@@ -12899,22 +12899,6 @@
   return stv_nothing;
 }
 
-/* Return the alignment of std::max_align_t.
-
-   [support.types.layout] The type max_align_t is a POD type whose alignment
-   requirement is at least as great as that of every scalar type, and whose
-   alignment requirement is supported in every context.  */
-
-unsigned
-max_align_t_align ()
-{
-  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
-				TYPE_ALIGN (long_double_type_node));
-  if (float128_type_node != NULL_TREE)
-    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
-  return max_align;
-}
-
 /* Return true iff ALIGN is an integral constant that is a fundamental
    alignment, as defined by [basic.align] in the c++-11
    specifications.
@@ -12928,7 +12912,7 @@
 bool
 cxx_fundamental_alignment_p (unsigned align)
 {
-  return (align <= max_align_t_align ());
+  return (align <= targetm.max_align_t_align ());
 }
 
 /* Return true if T is a pointer to a zero-sized aggregate.  */
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 240688)
+++ c-family/c-common.h	(working copy)
@@ -864,7 +864,6 @@
 extern bool keyword_is_storage_class_specifier (enum rid);
 extern bool keyword_is_type_qualifier (enum rid);
 extern bool keyword_is_decl_specifier (enum rid);
-extern unsigned max_align_t_align (void);
 extern bool cxx_fundamental_alignment_p (unsigned);
 extern bool pointer_to_zero_sized_aggr_p (tree);
 extern bool diagnose_mismatched_attributes (tree, tree);
Index: config/pa/pa.c
===================================================================
--- config/pa/pa.c	(revision 240688)
+++ config/pa/pa.c	(working copy)
@@ -194,6 +194,7 @@
 static bool pa_legitimate_constant_p (machine_mode, rtx);
 static unsigned int pa_section_type_flags (tree, const char *, int);
 static bool pa_legitimate_address_p (machine_mode, rtx, bool);
+static unsigned int pa_max_align_t_align (void);
 
 /* The following extra sections are only used for SOM.  */
 static GTY(()) section *som_readonly_data_section;
@@ -400,6 +401,9 @@
 #undef TARGET_LRA_P
 #define TARGET_LRA_P hook_bool_void_false
 
+#undef TARGET_MAX_ALIGN_T_ALIGN
+#define TARGET_MAX_ALIGN_T_ALIGN pa_max_align_t_align
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 /* Parse the -mfixed-range= option string.  */
@@ -10719,4 +10723,16 @@
   return NULL_RTX;
 }
 
+/* The maximimum alignment in bits for the POD type std:max_align_t.
+   This is set to 128 on 32-bit non HP-UX systems to suppress warnings
+   about new with extended alignment.  This arises because various POSIX
+   types such as pthread_mutex_t have for historical reasons 128-bit
+   alignment but the default alignment of std:max_align_t is 64 bits.  */
+
+static unsigned int
+pa_max_align_t_align (void)
+{
+  return TARGET_HPUX && !TARGET_64BIT ? 64 : 128;
+}
+
 #include "gt-pa.h"
Index: config/pa/pa.h
===================================================================
--- config/pa/pa.h	(revision 240688)
+++ config/pa/pa.h	(working copy)
@@ -292,9 +292,21 @@
 /* A bit-field declared as `int' forces `int' alignment for the struct.  */
 #define PCC_BITFIELD_TYPE_MATTERS 1
 
-/* No data type wants to be aligned rounder than this.  */
+/* No data type wants to be aligned rounder than this.  The long double
+   type has 16-byte alignment on the 64-bit target even though it was never
+   implemented in hardware.  The software implementation only needs 8-byte
+   alignment.  This is to match the HP compilers.  */
 #define BIGGEST_ALIGNMENT (2 * BITS_PER_WORD)
 
+/* Alignment, in bits, a C conformant malloc implementation has to provide.
+   The HP-UX malloc implementation provides a default alignment of 8 bytes.
+   This can be increased with mallopt.  The glibc implementation also provides
+   8-byte alignment.  Note that this isn't enough for various POSIX types such
+   as pthread_mutex_t.  However, since we no longer need the 16-byte alignment
+   for atomic operations, we ignore the nominal alignment specified for these
+   types.  The same is true for long double on 64-bit HP-UX.  */
+#define MALLOC_ABI_ALIGNMENT (64)
+
 /* Get around hp-ux assembler bug, and make strcpy of constants fast.  */
 #define CONSTANT_ALIGNMENT(EXP, ALIGN)		\
   (TREE_CODE (EXP) == STRING_CST		\
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 240688)
+++ cp/decl.c	(working copy)
@@ -4141,7 +4141,7 @@
   if (aligned_new_threshold == -1)
     aligned_new_threshold = (cxx_dialect >= cxx1z) ? 1 : 0;
   if (aligned_new_threshold == 1)
-    aligned_new_threshold = max_align_t_align () / BITS_PER_UNIT;
+    aligned_new_threshold = targetm.max_align_t_align () / BITS_PER_UNIT;
 
   {
     tree newattrs, extvisattr;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 240688)
+++ cp/init.c	(working copy)
@@ -3017,7 +3017,7 @@
   gcc_assert (alloc_fn != NULL_TREE);
 
   if (warn_aligned_new
-      && TYPE_ALIGN (elt_type) > max_align_t_align ()
+      && TYPE_ALIGN (elt_type) > targetm.max_align_t_align ()
       && (warn_aligned_new > 1
 	  || CP_DECL_CONTEXT (alloc_fn) == global_namespace)
       && !aligned_allocation_fn_p (alloc_fn))
Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 240688)
+++ doc/tm.texi	(working copy)
@@ -11686,6 +11686,10 @@
 ISO C11 requires atomic compound assignments that may raise floating-point exceptions to raise exceptions corresponding to the arithmetic operation whose result was successfully stored in a compare-and-exchange sequence.  This requires code equivalent to calls to @code{feholdexcept}, @code{feclearexcept} and @code{feupdateenv} to be generated at appropriate points in the compare-and-exchange sequence.  This hook should set @code{*@var{hold}} to an expression equivalent to the call to @code{feholdexcept}, @code{*@var{clear}} to an expression equivalent to the call to @code{feclearexcept} and @code{*@var{update}} to an expression equivalent to the call to @code{feupdateenv}.  The three expressions are @code{NULL_TREE} on entry to the hook and may be left as @code{NULL_TREE} if no code is required in a particular place.  The default implementation leaves all three expressions as @code{NULL_TREE}.  The @code{__atomic_feraiseexcept} function from @code{libatomic} may be of use as part of the code generated in @code{*@var{update}}.
 @end deftypefn
 
+@deftypefn {Target Hook} {unsigned int} TARGET_MAX_ALIGN_T_ALIGN (void)
+If defined, this function returns the alignment in bits for std:max_align_t. The default is suitable for most targets.  
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_RECORD_OFFLOAD_SYMBOL (tree)
 Used when offloaded functions are seen in the compilation unit and no named
 sections are available.  It is called once for each symbol that must be
Index: doc/tm.texi.in
===================================================================
--- doc/tm.texi.in	(revision 240688)
+++ doc/tm.texi.in	(working copy)
@@ -8224,6 +8224,8 @@
 
 @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 
+@hook TARGET_MAX_ALIGN_T_ALIGN
+
 @hook TARGET_RECORD_OFFLOAD_SYMBOL
 
 @hook TARGET_OFFLOAD_OPTIONS
Index: target.def
===================================================================
--- target.def	(revision 240688)
+++ target.def	(working copy)
@@ -5894,6 +5894,15 @@
  void, (tree *hold, tree *clear, tree *update),
  default_atomic_assign_expand_fenv)
 
+/* Return an unsigned int representing the alignment (in bits) of
+   std:max_align_t.  This allows the default alignment to be overridden.  */
+DEFHOOK
+(max_align_t_align,
+"If defined, this function returns the alignment in bits for std:max_align_t.\
+ The default is suitable for most targets.  ",
+ unsigned int, (void),
+ default_max_align_t_align)
+
 /* Leave the boolean fields at the end.  */
 
 /* True if we can create zeroed data by switching to a BSS section
Index: ginclude/stddef.h
===================================================================
--- ginclude/stddef.h	(revision 240816)
+++ ginclude/stddef.h	(working copy)
@@ -426,6 +426,10 @@
 typedef struct {
   long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
   long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
+#if defined(__hppa__) && !defined(__hpux__)
+  /* Provide special alignment for POSIX types on hppa-linux.  */
+  int __max_align_posix_types __attribute__((__aligned__(16)));
+#endif
   /* _Float128 is defined as a basic type, so max_align_t must be
      sufficiently aligned for it.  This code must work in C++, so we
      use __float128 here; that is only available on some
Index: targhooks.c
===================================================================
--- targhooks.c	(revision 240688)
+++ targhooks.c	(working copy)
@@ -1907,6 +1907,23 @@
 {
 }
 
+/* Default implementation of TARGET_MAX_ALIGN_T_ALIGN to return alignment
+   in bits of std::max_align_t.
+
+   [support.types.layout] The type max_align_t is a POD type whose alignment
+   requirement is at least as great as that of every scalar type, and whose
+   alignment requirement is supported in every context.  */
+
+unsigned int
+default_max_align_t_align (void)
+{
+  unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
+                                TYPE_ALIGN (long_double_type_node));
+  if (float128_type_node != NULL_TREE)
+    max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
+  return max_align;
+}
+
 #ifndef PAD_VARARGS_DOWN
 #define PAD_VARARGS_DOWN BYTES_BIG_ENDIAN
 #endif
Index: targhooks.h
===================================================================
--- targhooks.h	(revision 240688)
+++ targhooks.h	(working copy)
@@ -262,5 +262,6 @@
 				       optimization_type);
 
 extern unsigned int default_max_noce_ifcvt_seq_cost (edge);
+extern unsigned int default_max_align_t_align (void);
 
 #endif /* GCC_TARGHOOKS_H */

Re: [PATCH] Implement new hook for max_align_t_align

[Bernd Edlinger]

bounced, resent...

Hi,

I think your callback should also directly control the
alignment of max_align_t in stddef.h:

typedef struct {
   long long __max_align_ll __attribute__((__aligned__(__alignof__(long 
long))));
   long double __max_align_ld 
__attribute__((__aligned__(__alignof__(long double))));
   /* _Float128 is defined as a basic type, so max_align_t must be
      sufficiently aligned for it.  This code must work in C++, so we
      use __float128 here; that is only available on some
      architectures, but only on i386 is extra alignment needed for
      __float128.  */
#ifdef __i386__
   __float128 __max_align_f128 
__attribute__((__aligned__(__alignof(__float128))));
#endif
} max_align_t;


otherwise these will not match.

I think is __alignof(max_aling_t) == max_align_t_align ()
on your target?


Bernd.

Re: [PATCH] Implement new hook for max_align_t_align

[Bernd Edlinger]

Hi,

I think your callback should also directly control the
alignment of max_align_t in stddef.h:

typedef struct {
   long long __max_align_ll __attribute__((__aligned__(__alignof__(long 
long))));
   long double __max_align_ld 
__attribute__((__aligned__(__alignof__(long double))));
   /* _Float128 is defined as a basic type, so max_align_t must be
      sufficiently aligned for it.  This code must work in C++, so we
      use __float128 here; that is only available on some
      architectures, but only on i386 is extra alignment needed for
      __float128.  */
#ifdef __i386__
   __float128 __max_align_f128 
__attribute__((__aligned__(__alignof(__float128))));
#endif
} max_align_t;


otherwise these will not match.


Bernd.