[PATCH] MIPS: Ensure that lo_sums do not contain an unaligned symbol
Matthew Fortune
Matthew.Fortune@imgtec.com
Thu May 5 09:43:00 GMT 2016
Hi Andrew,
Thanks for working on this it is a painful area. There's a bit more to do
but this is cleaning up some sneaky bugs. Can you create a GCC bugzilla
entry if you haven't already as we should record where these bugs exist and
when they are fixed?
See my comments but I think that you are fixing more variants of this bug
than your summary states so we need to capture the detail on what code is
affected by these issues.
Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
> different offsets. Lets show this with an example C program.
>
> struct
> {
> short s;
> unsigned long long l;
> } h;
>
> void foo (void)
> {
> h.l = 0;
> }
>
> When this is compiled for MIPS it produces the following assembly:
>
> lui $2,%hi(h+8)
> sw $0,%lo(h+8)($2)
> jr $31
> sw $0,%lo(h+12)($2)
This looks like a stale example h+8 implies 8 bytes of data preceding 'l'.
>diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
>index 6cdda3b..f07e433 100644
>--- a/gcc/config/mips/mips.c
>+++ b/gcc/config/mips/mips.c
>@@ -2354,6 +2354,38 @@ mips_valid_lo_sum_p (enum mips_symbol_type symbol_type, machine_mode mode)
> return true;
> }
>
>+/* Return true if X in LO_SUM (REG, X) is a valid. */
is a valid...
I would however merge this code into mips_valid_lo_sum_p as the new function
name is fairly confusing. Both call sites for this function have the
symbol_type available which mips_valid_lo_sum_p requires and also have the
mode, reg, x so just add those to mips_valid_lo_sum_p.
>+
>+bool
>+mips_valid_lo_sum_lo_part_p (machine_mode mode, rtx reg, rtx x)
>+{
>+ rtx symbol = NULL;
three space indent.
>+
>+ if (mips_abi != ABI_32)
>+ return true;
I don't think this is limited to o32. I was thinking this was just about
splitting a multi-word unaligned access but actually the test cases in this
patch show that it is also about accessing unaligned elements in a structure
using the same 'hi' part for multiple 'lo' parts with differing offsets.
In the end I think there is no word size or abi specific issues here; it is
quite general.
>+ if (mode == BLKmode)
>+ return true;
Why is this special? Does the core GCC code ensure that lo_sum on a BLKmode
cannot have a constant offset greater than alignment?
>+ if (reg && REG_P (reg) && REGNO (reg) == GLOBAL_POINTER_REGNUM)
>+ return true;
I don't think reg need be an optional argument it is available at both
call sites. A comment to say why offsets from the global pointer are
not affected would also be useful.
>+
>+ if (GET_CODE (x) == CONST
>+ && GET_CODE (XEXP (x, 0)) == PLUS
>+ && GET_CODE (XEXP (XEXP (x, 0), 0)) == SYMBOL_REF)
>+ symbol = XEXP (XEXP (x, 0), 0);
>+ else if (GET_CODE (x) == SYMBOL_REF)
>+ symbol = x;
>+
>+ if (symbol
>+ && SYMBOL_REF_DECL (symbol)
>+ && (GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT) >
>+ (DECL_ALIGN_UNIT (SYMBOL_REF_DECL (symbol))))
This needs another bracket to cover the multiline '>' condition.
>+ return false;
>+
>+ return true;
>+}
>+
> /* Return true if X is a valid address for machine mode MODE. If it is,
> fill in INFO appropriately. STRICT_P is true if REG_OK_STRICT is in
> effect. */
>@@ -2394,7 +2426,8 @@ mips_classify_address (struct mips_address_info *info, rtx x,
> info->symbol_type
> = mips_classify_symbolic_expression (info->offset, SYMBOL_CONTEXT_MEM);
> return (mips_valid_base_register_p (info->reg, mode, strict_p)
>- && mips_valid_lo_sum_p (info->symbol_type, mode));
>+ && mips_valid_lo_sum_p (info->symbol_type, mode)
>+ && mips_valid_lo_sum_lo_part_p (mode, info->reg, info->offset));
As above this can become:
&& mips_valid_lo_sum_p (info->symbol_type, mode, info-reg,
info->offset)
>
> case CONST_INT:
> /* Small-integer addresses don't occur very often, but they
>@@ -3143,6 +3176,8 @@ mips_split_symbol (rtx temp, rtx addr, machine_mode mode, rtx *low_out)
> high = gen_rtx_HIGH (Pmode, copy_rtx (addr));
> high = mips_force_temporary (temp, high);
> *low_out = gen_rtx_LO_SUM (Pmode, high, addr);
>+ if (!mips_valid_lo_sum_lo_part_p (mode, NULL, addr))
tab indent. This can also become:
if (!mips_valid_lo_sum_p (symbol_type, mode, high, addr))
>+ *low_out = mips_force_temporary (temp, *low_out);
> break;
> }
> return true;
General comment on the tests... I don't think any of the '-mfp??' related tests
are actually doing anything with the FPU. All the load/store operations are
happening in GP registers so the FP mode is irrelevant. This makes some
tests redundant.
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c
>new file mode 100644
>index 0000000..dc81fd9
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c
>@@ -0,0 +1,14 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
Is the -mcode-readable=no so that these checks work for MIPS16?
>+/* { dg-final { scan-assembler-not "%lo\\(h\\+6\\)" } } */
>+
>+struct __attribute__((packed))
>+{
>+ short s;
>+ unsigned long long l;
indent 2 space.
>+} h;
>+
>+void foo (void)
void
foo (void)
throughout the tests.
>+{
>+ h.l = 0;
indent 2 space.
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c
>new file mode 100644
>index 0000000..da80b67
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c
>@@ -0,0 +1,19 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfpxx isa=2" } */
>+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */
>+/* { dg-final { scan-assembler "%lo\\(f\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */
This should be checking that there is no %lo(a_d+4) anywhere in the code.
>+
>+double d;
>+double __attribute__((aligned(1))) a_d;
>+float f;
>+float __attribute__((aligned(1))) a_f;
>+
>+void foo (void)
>+{
>+ d = 0;
>+ f = 0;
>+ a_d = 0;
>+ a_f = 0;
indent 2 space.
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c
>new file mode 100644
>index 0000000..b2679f9
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c
>@@ -0,0 +1,17 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */
This appears to be another form of the bug but is not mentioned in the
summary. Current GCC generates the %lo that must not appear in this
test.
>+
>+struct __attribute__((packed))
>+{
>+ char c;
>+ short s;
>+ int i;
indent 2 space
>+} h;
>+
>+void foo (void)
>+{
>+ h.c = 0;
>+ h.s = 0;
>+ h.i = 0;
indent 2 space
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c
>new file mode 100644
>index 0000000..1b9a0b1
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c
>@@ -0,0 +1,13 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */
>+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */
>+
>+unsigned long long l;
>+unsigned long long __attribute__((aligned(1))) a_l;
>+
>+void foo (void)
>+{
>+ l = 0;
>+ a_l = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c
>new file mode 100644
>index 0000000..a01c1d8
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c
>@@ -0,0 +1,15 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */
>+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */
Why not check 'd' and 'a_d' as well?
>+
>+long long l;
>+double d;
>+
>+long long __attribute__((aligned(1))) a_l;
>+double __attribute__((aligned(1))) a_d;
>+
>+void foo (void)
>+{
>+ d = (double) l;
>+ a_d = (double) a_l;
indent 2 space.
>+}
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c
>new file mode 100644
>index 0000000..496c078
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c
>@@ -0,0 +1,16 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */
>+
>+struct __attribute__((packed))
>+{
>+ short s;
>+ double d;
>+ float f;
indent 2 space... throughout, I won't mark each one from here on.
>+} h;
>+
>+void foo (void)
>+{
>+ h.d = 0;
>+ h.f = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c
>new file mode 100644
>index 0000000..828c328
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c
>@@ -0,0 +1,20 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */
>+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */
>+/* { dg-final { scan-assembler-not "%lo\\(a_d\\+4\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */
Why is this even generating SW? GCC should know these are unaligned and
not use SW. That is a separate issue though.
Did these %lo parts without an offset have to fall into the same category
as %lo with an offset due to implementation? Given there is no offset
then the %hi/%lo cannot mismatch. Simply forcing all unaligned address
calculations to a register might be the most elegant solution but if so
then it needs commenting on in the mips_lo_sum_valid_p function or
someone will try to optimise this later and break something else.
>+
>+double d;
>+double __attribute__((aligned(1))) a_d;
>+
>+float f;
>+float __attribute__((aligned(1))) a_f;
>+
>+void foo (void)
>+{
>+ d = 0;
>+ f = 0;
>+ a_d = 0;
>+ a_f = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c
>new file mode 100644
>index 0000000..2eb8f9a
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c
>@@ -0,0 +1,25 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */
>+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(c\\)" } } */
>+/* { dg-final { scan-assembler "\tsh\t\\\$\[0-9\],%lo\\(s\\)" } } */
>+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(i\\)" } } */
>+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(a_c\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsh\t\\\$\[0-9\],%lo\\(a_s\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_i\\)" } } */
>+
>+char __attribute__((aligned(1))) a_c;
>+short __attribute__((aligned(1))) a_s;
>+int __attribute__((aligned(1))) a_i;
>+char c;
>+short s;
>+int i;
>+
>+void foo (void)
>+{
>+ c = 0;
>+ s = 0;
>+ i = 0;
>+ a_c = 0;
>+ a_s = 0;
>+ a_i = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c
>new file mode 100644
>index 0000000..8f348fb
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c
>@@ -0,0 +1,16 @@
>+/* { dg-options "-mcode-readable=no -mabi=32 -mfp32 isa=1" } */
>+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */
>+
>+struct __attribute__((packed))
>+{
>+ short s;
>+ double d;
>+ float f;
>+} h;
>+
>+void foo (void)
>+{
>+ h.d = 0;
>+ h.f = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c
>new file mode 100644
>index 0000000..9070b53
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c
>@@ -0,0 +1,20 @@
>+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfp32 isa=1" } */
>+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */
>+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */
>+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */
>+
>+double d;
>+double __attribute__((aligned(1))) a_d;
>+
>+float f;
>+float __attribute__((aligned(1))) a_f;
>+
>+void foo (void)
>+{
>+ d = 0;
>+ f = 0;
>+ a_d = 0;
>+ a_f = 0;
>+}
>+
>diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c
>new file mode 100644
>index 0000000..ee11366
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c
>@@ -0,0 +1,16 @@
>+/* { dg-options "-mcode-readable=no -mabi=32 -mfpxx isa=2" } */
>+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */
>+
>+struct __attribute__((packed))
>+{
>+ short s;
>+ double d;
>+ float f;
>+} h;
>+
>+void foo (void)
>+{
>+ h.d = 0;
>+ h.f = 0;
>+}
>+
>
>
More information about the Gcc-patches
mailing list