[PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes

[PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes

[Jozef Lawrynowicz]

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

There have been some ICEs in the past for msp430-elf with the large
memory model (20-bit pointers), caused by SET_{DECL,TYPE}_ALIGN being called
with an argument which resolves to 20 i.e. POINTER_SIZE,
or the default value of TARGET_VTABLE_ENTRY_ALIGN (which is POINTER_SIZE).

The attached patch adds an assertion that SET_{DECL,TYPE}_ALIGN is called with
a value which is a power of 2, or 0, for targets which support a partial int
mode. This should catch issues in the future with non-power of 2
alignments being set, which could propagate into serious problems later in the
compilation process.

If the filtering to only perform this check for targets supporting a partial
int mode is unnecessary, I can remove that so CHECK_POW2_OR_ZEROP always
expands to check_pow2_or_zerop.

Successfully bootstrapped and regtested on x86_64-pc-linux-gnu and
msp430-elf/-mlarge.

Ok for trunk, or does this have to wait for GCC10 Stage 1?

[-- Attachment #2: 0001-Check-requested-alignment-in-SET_-DECL-TYPE-_ALIGN-i.patch --]
[-- Type: text/x-patch, Size: 2689 bytes --]

From b44723988dfb0bb9e8c647dd86aeba762ebdf84d Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Tue, 18 Dec 2018 11:14:35 +0000
Subject: [PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is
 pow2_or_zerop before aligning on targets with partial int modes

2018-12-30  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	gcc/ChangeLog:

	* tree.h (check_pow2_or_zerop): New.
	(CHECK_POW2_OR_ZEROP): New.
	(SET_TYPE_ALIGN): Call CHECK_POW2_OR_ZEROP before setting alignment.
	(SET_DECL_ALIGN): Call CHECK_POW2_OR_ZEROP before setting alignment.
---
 gcc/tree.h | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/gcc/tree.h b/gcc/tree.h
index ed37e54..e1188b7 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -74,6 +74,27 @@ as_internal_fn (combined_fn code)
   return internal_fn (int (code) - int (END_BUILTINS));
 }
 
+/* Historically there have been attempts to call SET_DECL/TYPE_ALIGN with
+   POINTER_SIZE as the alignment.  Alignment is expected to always be a power
+   of 2, so aligning to POINTER_SIZE on targets that use a partial integer
+   mode for pointers will cause problems.
+   So for targets that support a partial integer mode, check the requested
+   alignment is 0 or a power of 2 before aligning.  */
+#if defined(HAVE_PQImode) || defined(HAVE_PHImode) \
+  || defined(HAVE_PSImode) || defined(HAVE_PDImode)
+inline HOST_WIDE_INT
+check_pow2_or_zerop (HOST_WIDE_INT x)
+{
+  gcc_assert (pow2_or_zerop (x));
+  return x;
+}
+
+#define CHECK_POW2_OR_ZEROP(X) check_pow2_or_zerop (X)
+
+#else /* !defined(HAVE_P{Q,H,S,D}Imode) */
+#define CHECK_POW2_OR_ZEROP(X) X
+#endif
+
 /* Macros for initializing `tree_contains_struct'.  */
 #define MARK_TS_BASE(C)					\
   (tree_contains_struct[C][TS_BASE] = true)
@@ -1993,7 +2014,7 @@ extern machine_mode vector_type_mode (const_tree);
 
 /* Specify that TYPE_ALIGN(NODE) is X.  */
 #define SET_TYPE_ALIGN(NODE, X) \
-    (TYPE_CHECK (NODE)->type_common.align = ffs_hwi (X))
+    (TYPE_CHECK (NODE)->type_common.align = ffs_hwi (CHECK_POW2_OR_ZEROP (X)))
 
 /* 1 if the alignment for this type was requested by "aligned" attribute,
    0 if it is the default for this type.  */
@@ -2444,7 +2465,8 @@ extern machine_mode vector_type_mode (const_tree);
      ? ((unsigned)1) << ((NODE)->decl_common.align - 1) : 0)
 /* Specify that DECL_ALIGN(NODE) is X.  */
 #define SET_DECL_ALIGN(NODE, X) \
-    (DECL_COMMON_CHECK (NODE)->decl_common.align = ffs_hwi (X))
+    (DECL_COMMON_CHECK (NODE)->decl_common.align \
+     = ffs_hwi (CHECK_POW2_OR_ZEROP (X)))
 
 /* The minimum alignment necessary for the datum, in bits, without
    warning.  */
-- 
2.7.4

Re: [PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes

[Richard Biener]

On Sun, Dec 30, 2018 at 12:51 PM Jozef Lawrynowicz
<jozef.l@mittosystems.com> wrote:
>
> There have been some ICEs in the past for msp430-elf with the large
> memory model (20-bit pointers), caused by SET_{DECL,TYPE}_ALIGN being called
> with an argument which resolves to 20 i.e. POINTER_SIZE,
> or the default value of TARGET_VTABLE_ENTRY_ALIGN (which is POINTER_SIZE).
>
> The attached patch adds an assertion that SET_{DECL,TYPE}_ALIGN is called with
> a value which is a power of 2, or 0, for targets which support a partial int
> mode. This should catch issues in the future with non-power of 2
> alignments being set, which could propagate into serious problems later in the
> compilation process.
>
> If the filtering to only perform this check for targets supporting a partial
> int mode is unnecessary, I can remove that so CHECK_POW2_OR_ZEROP always
> expands to check_pow2_or_zerop.
>
> Successfully bootstrapped and regtested on x86_64-pc-linux-gnu and
> msp430-elf/-mlarge.
>
> Ok for trunk, or does this have to wait for GCC10 Stage 1?

I think the use of ffs_hwi suggests that the current behavior was intended
(or kept exactly because of calls with bougs values).  If that's not wanted
why not simply use exact_log2 instead of ffs_hwi in the SET_ macros?

Btw, I'd simply make SET_{TYPE,DECL}_ALIGN inline functions and
then gcc_checking_assert that exact_log2 didn't return -1.

Richard.

Re: [PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes

[Jeff Law]

On 12/30/18 4:51 AM, Jozef Lawrynowicz wrote:
> There have been some ICEs in the past for msp430-elf with the large
> memory model (20-bit pointers), caused by SET_{DECL,TYPE}_ALIGN being called
> with an argument which resolves to 20 i.e. POINTER_SIZE,
> or the default value of TARGET_VTABLE_ENTRY_ALIGN (which is POINTER_SIZE).
> 
> The attached patch adds an assertion that SET_{DECL,TYPE}_ALIGN is called with
> a value which is a power of 2, or 0, for targets which support a partial int
> mode. This should catch issues in the future with non-power of 2
> alignments being set, which could propagate into serious problems later in the
> compilation process.
> 
> If the filtering to only perform this check for targets supporting a partial
> int mode is unnecessary, I can remove that so CHECK_POW2_OR_ZEROP always
> expands to check_pow2_or_zerop.
> 
> Successfully bootstrapped and regtested on x86_64-pc-linux-gnu and
> msp430-elf/-mlarge.
> 
> Ok for trunk, or does this have to wait for GCC10 Stage 1?
> 
Let's defer to gcc-10.

I'm not sure it's worth trying to conditionalize this on the partial
integer modes.  Why not just do it all the time?  I'd be surprised if
it's that expensive.

jeff

Re: [PATCH] Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes

[Jozef Lawrynowicz]

On Fri, 11 Jan 2019 13:00:22 -0700
Jeff Law <law@redhat.com> wrote:

> On 12/30/18 4:51 AM, Jozef Lawrynowicz wrote:
> > 
> > The attached patch adds an assertion that SET_{DECL,TYPE}_ALIGN is called with
> > a value which is a power of 2, or 0, for targets which support a partial int
> > mode. This should catch issues in the future with non-power of 2
> > alignments being set, which could propagate into serious problems later in the
> > compilation process.
> > 
> Let's defer to gcc-10.
> 
> I'm not sure it's worth trying to conditionalize this on the partial
> integer modes.  Why not just do it all the time?  I'd be surprised if
> it's that expensive.

Yes in hindsight it was overkill to conditionalize the checks.

I'll redo and submit this again with yours and Richard's suggestions for
GCC-10.

Thanks,
Jozef