| 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 | expand |
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.
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
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
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
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?