On Mon, Feb 11, 2019 at 5:35 PM Wilco Dijkstra <wilco.dijks...@arm.com> wrote:
>
> The GCC optimizer can generate symbols with non-zero offset from simple
> if-statements. Bit zero is used for the Arm/Thumb state bit, so relocations
> with offsets fail if it changes bit zero and the relocation forces bit zero
> to true. The fix is to disable offsets on function pointer symbols.
>
> ARMv5te bootstrap OK, regression tests pass. OK for commit?
Interesting bug. armv5te-linux bootstrap ? Can you share your --target
and --with-arch flags ?
>
> ChangeLog:
> 2019-02-06 Wilco Dijkstra <wdijk...@arm.com>
>
> gcc/
> PR target/89222
> * config/arm/arm.md (movsi): Use arm_cannot_force_const_mem
> to decide when to split off an offset from a symbol.
> * config/arm/arm.c (arm_cannot_force_const_mem): Disallow offsets
> in function symbols.
> * config/arm/arm-protos.h (arm_cannot_force_const_mem): Add.
>
> testsuite/
> PR target/89222
> * gcc.target/arm/pr89222.c: Add new test.
>
> --
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index
> 79ede0db174fcce87abe8b4d18893550d4c7e2f6..0bedbe5110853617ecf7456bbaa56b1405fb65dd
> 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -184,6 +184,7 @@ extern void arm_init_cumulative_args (CUMULATIVE_ARGS *,
> tree, rtx, tree);
> extern bool arm_pad_reg_upward (machine_mode, tree, int);
> #endif
> extern int arm_apply_result_size (void);
> +extern bool arm_cannot_force_const_mem (machine_mode, rtx);
>
> #endif /* RTX_CODE */
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index
> c4c9b4a667100d81d918196713e40b01ee232ee2..ccd4211045066d8edb89dd4c23d554517639f8f6
> 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -178,7 +178,6 @@ static void arm_internal_label (FILE *, const char *,
> unsigned long);
> static void arm_output_mi_thunk (FILE *, tree, HOST_WIDE_INT, HOST_WIDE_INT,
> tree);
> static bool arm_have_conditional_execution (void);
> -static bool arm_cannot_force_const_mem (machine_mode, rtx);
> static bool arm_legitimate_constant_p (machine_mode, rtx);
> static bool arm_rtx_costs (rtx, machine_mode, int, int, int *, bool);
> static int arm_address_cost (rtx, machine_mode, addr_space_t, bool);
> @@ -8936,15 +8935,20 @@ arm_legitimate_constant_p (machine_mode mode, rtx x)
>
> /* Implement TARGET_CANNOT_FORCE_CONST_MEM. */
>
> -static bool
Let's keep this static ...
> +bool
> arm_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
> {
> rtx base, offset;
> + split_const (x, &base, &offset);
>
> - if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P)
> + if (GET_CODE (base) == SYMBOL_REF)
Isn't there a SYMBOL_REF_P predicate for this ?
> {
> - split_const (x, &base, &offset);
> - if (GET_CODE (base) == SYMBOL_REF
> + /* Function symbols cannot have an offset due to the Thumb bit. */
> + if ((SYMBOL_REF_FLAGS (base) & SYMBOL_FLAG_FUNCTION)
> + && INTVAL (offset) != 0)
> + return true;
> +
Can we look to allow anything that is a power of 2 as an offset i.e.
anything with bit 0 set to 0 ? Could you please file an enhancement
request on binutils for both gold and ld to catch the linker warning
case ? I suspect we are looking for addends which have the lower bit
set and function symbols ?
> + if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P
> && !offset_within_block_p (base, INTVAL (offset)))
> return true;
> }
this looks ok.
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index
> aa759624f8f617576773aa75fd6239d6e06e8a13..00fccd964a86dd814f15e4a1fdf5b47173a3ee3f
> 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -5981,17 +5981,13 @@ (define_expand "movsi"
> }
> }
>
> - if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P)
> + if (arm_cannot_force_const_mem (SImode, operands[1]))
Firstly (targetm.cannot_force_const_mem (...)) please instead of
arm_cannot_force_const_mem , then that can remain static. Let's look
to use the targetm interface instead of direct calls here. We weren't
hitting this path for non-vxworks code , however now we do so if
arm_tls_referenced_p is true at the end of arm_cannot_force_const_mem
which means that we could well have a TLS address getting spat out or
am I mis-reading something ?
This is my main concern with this patch ..
> {
> split_const (operands[1], &base, &offset);
> - if (GET_CODE (base) == SYMBOL_REF
> - && !offset_within_block_p (base, INTVAL (offset)))
> - {
> - tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
> - emit_move_insn (tmp, base);
> - emit_insn (gen_addsi3 (operands[0], tmp, offset));
> - DONE;
> - }
> + tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
> + emit_move_insn (tmp, base);
> + emit_insn (gen_addsi3 (operands[0], tmp, offset));
> + DONE;
Can Olivier or someone who cares about vxworks also give this a quick
sanity run for the alternate code path once we resolve some of the
review questions here ? Don't we also need to worry about
-mslow-flash-data where we get rid of literal pools and have movw /
movt instructions ?
regards
Ramana
> }
>
> /* Recognize the case where operand[1] is a reference to thread-local
> diff --git a/gcc/testsuite/gcc.target/arm/pr89222.c
> b/gcc/testsuite/gcc.target/arm/pr89222.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..d26d7df17544db8426331e67b9a36d749ec6c6d1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr89222.c
> @@ -0,0 +1,32 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +void g (void);
> +
> +void f1 (int x)
> +{
> + if (x != (int) g + 3)
> + return;
> + g();
> +}
> +
> +void (*a2)(void);
> +
> +void f2 (void)
> +{
> + a2 = &g + 3;
> +}
> +
> +typedef void (*__sighandler_t)(int);
> +void handler (int);
> +
> +void f3 (int x)
> +{
> + __sighandler_t h = &handler;
> + if (h != (__sighandler_t) 2 && h != (__sighandler_t) 1)
> + h (x);
> +}
> +
> +/* { dg-final { scan-assembler-times {add(?:s)?\tr[0-9]+, r[0-9]+, #3} 2 } }
> */
> +/* { dg-final { scan-assembler-not {.word\tg\+3} } } */
> +/* { dg-final { scan-assembler-not {.word\thandler-1} } } */
>
