Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes
diff mbox series

Message ID 20181230115148.720ac4aa@jozef-Aspire-VN7-793G
State New
Headers show
Series
  • Check requested alignment in SET_{DECL,TYPE}_ALIGN is pow2_or_zerop before aligning on targets with partial int modes
Related show

Commit Message

Jozef Lawrynowicz Dec. 30, 2018, 11:51 a.m. UTC
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?

Comments

Richard Biener Jan. 2, 2019, 9:02 a.m. UTC | #1
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.
Jeff Law Jan. 11, 2019, 8 p.m. UTC | #2
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
Jozef Lawrynowicz Jan. 16, 2019, 10:37 a.m. UTC | #3
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

Patch
diff mbox series

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