| Message ID | CADnVucBLd0YBEWsJGFVNTktuGh-72cxXmNH0HnK-+6TytO-s6Q@mail.gmail.com |
|---|---|
| State | Accepted |
| Commit | e75c1617769823ca3372355a26383dbe03a5c327 |
| Headers | show |
On Thu, Apr 3, 2014 at 2:27 PM, Charles Baylis <charles.baylis@linaro.org> wrote: > Hi > > This bug causes the compiler to create a Thumb-2 TBB instruction with > a jump table containing an out of range value in a .byte field: > > whatever.s:148: Error: value of 256 too large for field of 1 bytes at 100 > > This occurs because the jump table is followed with a ".align 1" due > to ASM_OUTPUT_CASE_END, but the 'shorten' phase does not account for > the space taken by this align directive. My first reaction is to wonder why this is this not a bug in the "shorten" phase. > > This patch addresses the issue by removing ASM_OUTPUT_CASE_END from > arm.h, and ensuring that the alignment after an ADDR_DIFF_VEC is > instead inserted by aligning the label following the barrier which > follows it. This is achieved by defining LABEL_ALIGN_AFTER_BARRIER > appropriately. On first glance this feels like a blunt hammer, what's the code size bloat with putting out such an alignment after each barrier that the compiler emits rather than tracking this in ASM_OUTPUT_CASE_END. I'll try and have a look at this again tomorrow morning. regards Ramana > > Bootstrapped/checked on arm-unknown-linux-gnueabihf. > > OK for trunk, and backporting to 4.8? > > > > 2014-04-02 Charles Baylis <charles.baylis@linaro.org> > > PR target/60609 > * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. > (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after > ADDR_DIFF_VEC. > > > 2014-04-02 Charles Baylis <charles.baylis@linaro.org> > > PR target/60609 > * g++.dg/torture/pr60609.C: New test.
On 04/03/14 11:02, Ramana Radhakrishnan wrote: > On Thu, Apr 3, 2014 at 2:27 PM, Charles Baylis > <charles.baylis@linaro.org> wrote: >> Hi >> >> This bug causes the compiler to create a Thumb-2 TBB instruction with >> a jump table containing an out of range value in a .byte field: >> >> whatever.s:148: Error: value of 256 too large for field of 1 bytes at 100 >> >> This occurs because the jump table is followed with a ".align 1" due >> to ASM_OUTPUT_CASE_END, but the 'shorten' phase does not account for >> the space taken by this align directive. > > My first reaction is to wonder why this is this not a bug in the > "shorten" phase. I don't think that code ever expected an alignment directive to be emitted by ASM_OUTPUT_CASE_END :( > >> >> This patch addresses the issue by removing ASM_OUTPUT_CASE_END from >> arm.h, and ensuring that the alignment after an ADDR_DIFF_VEC is >> instead inserted by aligning the label following the barrier which >> follows it. This is achieved by defining LABEL_ALIGN_AFTER_BARRIER >> appropriately. > > On first glance this feels like a blunt hammer, what's the code size > bloat with putting out such an alignment after each barrier that the > compiler emits rather than tracking this in ASM_OUTPUT_CASE_END. I'd tend to agree that emitting an alignment after each barrier would be a blunt hammer in this case. ISTM we really want a new target hook to define the alignment after a the jump table, independent of the other alignment directives. Then we'd have to teach shorten_branches about that. Perhaps the blunt hammer for 4.9 and the new alignemnt-after-jump-table for the next stage1? jeff
>> >> My first reaction is to wonder why this is this not a bug in the >> "shorten" phase. > > I don't think that code ever expected an alignment directive to be emitted > by ASM_OUTPUT_CASE_END :( Fair point and it looks like this support came in when the support for Thumb2 was added eons ago. > > > >> >>> >>> This patch addresses the issue by removing ASM_OUTPUT_CASE_END from >>> arm.h, and ensuring that the alignment after an ADDR_DIFF_VEC is >>> instead inserted by aligning the label following the barrier which >>> follows it. This is achieved by defining LABEL_ALIGN_AFTER_BARRIER >>> appropriately. >> >> >> On first glance this feels like a blunt hammer, what's the code size >> bloat with putting out such an alignment after each barrier that the >> compiler emits rather than tracking this in ASM_OUTPUT_CASE_END. > > I'd tend to agree that emitting an alignment after each barrier would be a > blunt hammer in this case. > > ISTM we really want a new target hook to define the alignment after a the > jump table, independent of the other alignment directives. Then we'd have > to teach shorten_branches about that. > > Perhaps the blunt hammer for 4.9 and the new alignemnt-after-jump-table for > the next stage1? Given the following below - it may not be that blunt at all. I don't know how I missed this yesterday (thanks to Charlie for pointing this out on IRC) :( +#define LABEL_ALIGN_AFTER_BARRIER(LABEL) \ + (GET_CODE (PATTERN (prev_active_insn (LABEL))) == ADDR_DIFF_VEC \ + ? 1 : 0) I think we should keep this on trunk for a week to check if there is no unintended fallout before backporting to 4.8 Additionally the testing has only considered Thumb2 - since we also do jumptable shortening for Thumb1 and given this late change it's worth also testing this on Thumb1 and making sure there are no regressions. Maybe Joey can help there if you aren't set up to do this. Ok if no regressions and modulo RM objections. One minor Changelog nit. 2014-04-02 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after ADDR_DIFF_VEC. s/)/):/g above. regards Ramana > > jeff >
On 4 April 2014 15:50, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > Additionally the testing has only considered Thumb2 - since we also do > jumptable shortening for Thumb1 and given this late change it's worth > also testing this on Thumb1 and making sure there are no regressions. > Maybe Joey can help there if you aren't set up to do this. > > Ok if no regressions and modulo RM objections. I have tested v5t Thumb-1 with no regressions on qemu. > One minor Changelog nit. > > 2014-04-02 Charles Baylis <charles.baylis@linaro.org> > > PR target/60609 > * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. > (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after > ADDR_DIFF_VEC. > > s/)/):/g above. Noted.
This doesn't seem to have shown problems on trunk/4.9. I have bootstrapped and checked the patch on 4.8 arm-unknown-linux-gnueabihf (Thumb-2) on qemu. I have checked the patch on 4.7 arm-unknown-linux-gnueabihf (Thumb-2) on qemu. OK to backport to 4.8 and 4.7? On 7 April 2014 16:02, Charles Baylis <charles.baylis@linaro.org> wrote: > On 4 April 2014 15:50, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >> Additionally the testing has only considered Thumb2 - since we also do >> jumptable shortening for Thumb1 and given this late change it's worth >> also testing this on Thumb1 and making sure there are no regressions. >> Maybe Joey can help there if you aren't set up to do this. >> >> Ok if no regressions and modulo RM objections. > > I have tested v5t Thumb-1 with no regressions on qemu. > >> One minor Changelog nit. >> >> 2014-04-02 Charles Baylis <charles.baylis@linaro.org> >> >> PR target/60609 >> * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. >> (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after >> ADDR_DIFF_VEC. >> >> s/)/):/g above. > > Noted.
On Fri, Apr 25, 2014 at 4:29 PM, Charles Baylis <charles.baylis@linaro.org> wrote: > This doesn't seem to have shown problems on trunk/4.9. > > I have bootstrapped and checked the patch on 4.8 > arm-unknown-linux-gnueabihf (Thumb-2) on qemu. > > I have checked the patch on 4.7 arm-unknown-linux-gnueabihf (Thumb-2) on qemu. > > OK to backport to 4.8 and 4.7? Ok by me but give 24 "working" hours for an RM to object. Ramana > > On 7 April 2014 16:02, Charles Baylis <charles.baylis@linaro.org> wrote: >> On 4 April 2014 15:50, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>> Additionally the testing has only considered Thumb2 - since we also do >>> jumptable shortening for Thumb1 and given this late change it's worth >>> also testing this on Thumb1 and making sure there are no regressions. >>> Maybe Joey can help there if you aren't set up to do this. >>> >>> Ok if no regressions and modulo RM objections. >> >> I have tested v5t Thumb-1 with no regressions on qemu. >> >>> One minor Changelog nit. >>> >>> 2014-04-02 Charles Baylis <charles.baylis@linaro.org> >>> >>> PR target/60609 >>> * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. >>> (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after >>> ADDR_DIFF_VEC. >>> >>> s/)/):/g above. >> >> Noted.
On 25 April 2014 16:31, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Fri, Apr 25, 2014 at 4:29 PM, Charles Baylis > <charles.baylis@linaro.org> wrote: >> OK to backport to 4.8 and 4.7? > > Ok by me but give 24 "working" hours for an RM to object. Committed to 4.7 as r210227. Committed to 4.8 as r210226. 2014-05-08 Charles Baylis <charles.baylis@linaro.org> Backport from mainline 2014-04-07 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * config/arm/arm.h (ASM_OUTPUT_CASE_END): Remove. (LABEL_ALIGN_AFTER_BARRIER): Align barriers which occur after ADDR_DIFF_VEC.
From 9b0c1ada23e2b210b02ebaee2f599bb5205a91d6 Mon Sep 17 00:00:00 2001 From: Charles Baylis <charles.baylis@linaro.org> Date: Thu, 3 Apr 2014 10:57:33 +0100 Subject: [PATCH] fix for PR target/60609 2014-04-02 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after ADDR_DIFF_VEC. 2014-04-02 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * g++.dg/torture/pr60609.C: New test. --- gcc/config/arm/arm.h | 11 +- gcc/testsuite/g++.dg/torture/pr60609.C | 252 +++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/g++.dg/torture/pr60609.C diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h index 7ca47a7..a4bbd12 100644 --- a/gcc/config/arm/arm.h +++ b/gcc/config/arm/arm.h @@ -2194,14 +2194,9 @@ extern int making_const_table; #undef ASM_OUTPUT_BEFORE_CASE_LABEL #define ASM_OUTPUT_BEFORE_CASE_LABEL(FILE, PREFIX, NUM, TABLE) /* Empty. */ -/* Make sure subsequent insns are aligned after a TBB. */ -#define ASM_OUTPUT_CASE_END(FILE, NUM, JUMPTABLE) \ - do \ - { \ - if (GET_MODE (PATTERN (JUMPTABLE)) == QImode) \ - ASM_OUTPUT_ALIGN (FILE, 1); \ - } \ - while (0) +#define LABEL_ALIGN_AFTER_BARRIER(LABEL) \ + (GET_CODE (PATTERN (prev_active_insn (LABEL))) == ADDR_DIFF_VEC \ + ? 1 : 0) #define ARM_DECLARE_FUNCTION_NAME(STREAM, NAME, DECL) \ do \ diff --git a/gcc/testsuite/g++.dg/torture/pr60609.C b/gcc/testsuite/g++.dg/torture/pr60609.C new file mode 100644 index 0000000..9ddec0b --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr60609.C @@ -0,0 +1,252 @@ +/* { dg-do assemble } */ + +class exception +{ +}; +class bad_alloc:exception +{ +}; +class logic_error:exception +{ +}; +class domain_error:logic_error +{ +}; +class invalid_argument:logic_error +{ +}; +class length_error:logic_error +{ +}; +class overflow_error:exception +{ +}; +typedef int mpz_t[]; +template < class > class __gmp_expr; +template <> class __gmp_expr < mpz_t > +{ + ~__gmp_expr (); +}; + +class PIP_Solution_Node; +class internal_exception +{ + ~internal_exception (); +}; +class not_an_integer:internal_exception +{ +}; +class not_a_variable:internal_exception +{ +}; +class not_an_optimization_mode:internal_exception +{ +}; +class not_a_bounded_integer_type_width:internal_exception +{ +}; +class not_a_bounded_integer_type_representation:internal_exception +{ +}; +class not_a_bounded_integer_type_overflow:internal_exception +{ +}; +class not_a_complexity_class:internal_exception +{ +}; +class not_a_control_parameter_name:internal_exception +{ +}; +class not_a_control_parameter_value:internal_exception +{ +}; +class not_a_pip_problem_control_parameter_name:internal_exception +{ +}; +class not_a_pip_problem_control_parameter_value:internal_exception +{ +}; +class not_a_relation:internal_exception +{ +}; +class ppl_handle_mismatch:internal_exception +{ +}; +class timeout_exception +{ + ~timeout_exception (); +}; +class deterministic_timeout_exception:timeout_exception +{ +}; +void __assert_fail (const char *, const char *, int, int *) +__attribute__ ((__noreturn__)); +void PL_get_pointer (void *); +int Prolog_is_address (); +inline int +Prolog_get_address (void **p1) +{ + Prolog_is_address ()? static_cast < + void >(0) : __assert_fail ("Prolog_is_address", "./swi_cfli.hh", 0, 0); + PL_get_pointer (p1); + return 0; +} + +class non_linear:internal_exception +{ +}; +class not_unsigned_integer:internal_exception +{ +}; +class not_universe_or_empty:internal_exception +{ +}; +class not_a_nil_terminated_list:internal_exception +{ +}; +class PPL_integer_out_of_range +{ + __gmp_expr < mpz_t > n; +}; +void handle_exception (); +template < typename T > T * term_to_handle (int, const char *) +{ + if (Prolog_is_address ()) + { + void *p; + Prolog_get_address (&p); + return static_cast < T * >(0); + } + throw; +} + +void +ppl_new_MIP_Problem_from_MIP_Problem () +try +{ + term_to_handle < int >(0, "ppl_new_MIP_Problem_from_MIP_Problem/2"); +} + +catch (exception &) +{ +} + +int +ppl_PIP_Tree_Node_parametric_values () +{ + try + { + PIP_Solution_Node *a = term_to_handle < PIP_Solution_Node > (0, 0); + (void)a; + return 1; + } + catch (internal_exception &) + { + } + catch (not_unsigned_integer &) + { + handle_exception (); + } + catch (non_linear &) + { + handle_exception (); + } + catch (not_a_variable &) + { + handle_exception (); + } + catch (not_an_integer &) + { + handle_exception (); + } + catch (ppl_handle_mismatch &) + { + handle_exception (); + } + catch (not_an_optimization_mode &) + { + handle_exception (); + } + catch (not_a_complexity_class &) + { + handle_exception (); + } + catch (not_a_bounded_integer_type_width &) + { + handle_exception (); + } + catch (not_a_bounded_integer_type_representation &) + { + handle_exception (); + } + catch (not_a_bounded_integer_type_overflow &) + { + handle_exception (); + } + catch (not_a_control_parameter_name &) + { + handle_exception (); + } + catch (not_a_control_parameter_value &) + { + handle_exception (); + } + catch (not_a_pip_problem_control_parameter_name &) + { + handle_exception (); + } + catch (not_a_pip_problem_control_parameter_value &) + { + handle_exception (); + } + catch (not_universe_or_empty &) + { + handle_exception (); + } + catch (not_a_relation &) + { + handle_exception (); + } + catch (not_a_nil_terminated_list &) + { + handle_exception (); + } + catch (PPL_integer_out_of_range &) + { + handle_exception (); + } + catch (int &) + { + } catch (timeout_exception &) + { + handle_exception (); + } catch (deterministic_timeout_exception &) + { + handle_exception (); + } catch (overflow_error &) + { + handle_exception (); + } catch (domain_error &) + { + handle_exception (); + } catch (length_error &) + { + handle_exception (); + } catch (invalid_argument &) + { + handle_exception (); + } catch (logic_error &) + { + handle_exception (); + } catch (bad_alloc &) + { + handle_exception (); + } catch (exception &) + { + handle_exception (); + } catch ( ...) + { + handle_exception (); + } + return 0; +} -- 1.8.3.2
Hi This bug causes the compiler to create a Thumb-2 TBB instruction with a jump table containing an out of range value in a .byte field: whatever.s:148: Error: value of 256 too large for field of 1 bytes at 100 This occurs because the jump table is followed with a ".align 1" due to ASM_OUTPUT_CASE_END, but the 'shorten' phase does not account for the space taken by this align directive. This patch addresses the issue by removing ASM_OUTPUT_CASE_END from arm.h, and ensuring that the alignment after an ADDR_DIFF_VEC is instead inserted by aligning the label following the barrier which follows it. This is achieved by defining LABEL_ALIGN_AFTER_BARRIER appropriately. Bootstrapped/checked on arm-unknown-linux-gnueabihf. OK for trunk, and backporting to 4.8? 2014-04-02 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * config/arm/arm.h (ASM_OUTPUT_CASE_END) Remove. (LABEL_ALIGN_AFTER_BARRIER) Align barriers which occur after ADDR_DIFF_VEC. 2014-04-02 Charles Baylis <charles.baylis@linaro.org> PR target/60609 * g++.dg/torture/pr60609.C: New test.