| Message ID | 52D1D6D6.2090301@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11/01/14 23:42, Kugan wrote: > Hi, > > aarch64_build_constant incorrectly truncates the immediate when > constants are generated with MOVN. This causes coinor-osi tests to fail > (tracked also in https://bugs.launchpad.net/gcc-linaro/+bug/1263576) > > Attached patch fixes this. Also attaching a reduced testcase that > reproduces this. Tested on aarch64-none-linux-gnu with no new > regressions. Is this OK for trunk? > > Thanks, > Kugan > > gcc/ > +2013-10-15 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> > + Kugan Vivekanandarajah <kuganv@linaro.org> > + > + PR target/59588 > + * config/aarch64/aarch64.c (aarch64_build_constant): Fix incorrect > + truncation. > + > > > gcc/testsuite/ > +2014-01-11 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> > + Kugan Vivekanandarajah <kuganv@linaro.org> > + > + PR target/59695 > + * g++.dg/pr59695.C: New file. > + > > > p.txt > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 3d32ea5..854666f 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -2486,7 +2486,7 @@ aarch64_build_constant (int regnum, HOST_WIDE_INT val) > if (ncount < zcount) > { > emit_move_insn (gen_rtx_REG (Pmode, regnum), > - GEN_INT ((~val) & 0xffff)); > + GEN_INT (~((~val) & 0xffff))); I think that would be better written as GEN_INT (val | ~(HOST_WIDE_INT) 0xffff); Note the cast after the ~ to ensure we invert the right number of bits. Otherwise OK. R. > tval = 0xffff; > } > else > diff --git a/gcc/testsuite/g++.dg/pr59695.C b/gcc/testsuite/g++.dg/pr59695.C > index e69de29..0da06cb 100644 > --- a/gcc/testsuite/g++.dg/pr59695.C > +++ b/gcc/testsuite/g++.dg/pr59695.C > @@ -0,0 +1,125 @@ > + > +/* PR target/53055 */ > +/* { dg-do run { target aarch64*-*-* } } */ > +/* { dg-options "-O0" } */ > + > +#define DEFINE_VIRTUALS_FNS(i) virtual void xxx##i () {} \ > + virtual void foo1_##i () {}\ > + virtual void foo2_##i () {}\ > + virtual void foo3_##i () {}\ > + virtual void foo4_##i () {}\ > + virtual void foo5_##i () {}\ > + virtual void foo6_##i () {}\ > + virtual void foo7_##i () {}\ > + virtual void foo8_##i () {}\ > + virtual void foo9_##i () {}\ > + virtual void foo10_##i () {}\ > + virtual void foo11_##i () {}\ > + virtual void foo12_##i () {}\ > + virtual void foo13_##i () {}\ > + virtual void foo14_##i () {}\ > + virtual void foo15_##i () {}\ > + virtual void foo16_##i () {}\ > + virtual void foo17_##i () {}\ > + virtual void foo18_##i () {}\ > + virtual void foo19_##i () {}\ > + virtual void foo20_##i () {}\ > + virtual void foo21_##i () {}\ > + virtual void foo22_##i () {}\ > + > +class base_class_2 > +{ > + > +public: > + /* Define lots of virtual functions */ > + DEFINE_VIRTUALS_FNS (1) > + DEFINE_VIRTUALS_FNS (2) > + DEFINE_VIRTUALS_FNS (3) > + DEFINE_VIRTUALS_FNS (4) > + DEFINE_VIRTUALS_FNS (5) > + DEFINE_VIRTUALS_FNS (6) > + DEFINE_VIRTUALS_FNS (7) > + DEFINE_VIRTUALS_FNS (8) > + DEFINE_VIRTUALS_FNS (9) > + DEFINE_VIRTUALS_FNS (10) > + DEFINE_VIRTUALS_FNS (11) > + DEFINE_VIRTUALS_FNS (12) > + DEFINE_VIRTUALS_FNS (13) > + DEFINE_VIRTUALS_FNS (14) > + DEFINE_VIRTUALS_FNS (15) > + DEFINE_VIRTUALS_FNS (16) > + DEFINE_VIRTUALS_FNS (17) > + DEFINE_VIRTUALS_FNS (18) > + DEFINE_VIRTUALS_FNS (19) > + DEFINE_VIRTUALS_FNS (20) > + > + base_class_2(); > + virtual ~base_class_2 (); > +}; > + > +base_class_2::base_class_2() > +{ > +} > + > +base_class_2::~base_class_2 () > +{ > +} > + > +class base_class_1 > +{ > +public: > + virtual ~base_class_1(); > + base_class_1(); > +}; > + > +base_class_1::base_class_1() > +{ > +} > + > +base_class_1::~base_class_1() > +{ > +} > + > +class base_Impl_class : > + virtual public base_class_2, public base_class_1 > +{ > +public: > + base_Impl_class (); > + virtual ~base_Impl_class (); > +}; > + > +base_Impl_class::base_Impl_class () > +{ > +} > + > +base_Impl_class::~base_Impl_class () > +{ > +} > + > + > +class test_cls : public base_Impl_class > +{ > +public: > + test_cls(); > + virtual ~test_cls(); > +}; > + > +test_cls::test_cls() > +{ > +} > + > +test_cls::~test_cls() > +{ > +} > + > +int main() > +{ > + test_cls *test = new test_cls; > + base_class_2 *p1 = test; > + > + /* PR 53055 destructor thunk offsets are not setup > + correctly resulting in crash. */ > + delete p1; > + return 0; > +} > + >
On 13/01/14 21:05, Richard Earnshaw wrote: > On 11/01/14 23:42, Kugan wrote: >> Hi, >> >> aarch64_build_constant incorrectly truncates the immediate when >> constants are generated with MOVN. This causes coinor-osi tests to fail >> (tracked also in https://bugs.launchpad.net/gcc-linaro/+bug/1263576) >> >> Attached patch fixes this. Also attaching a reduced testcase that >> reproduces this. Tested on aarch64-none-linux-gnu with no new >> regressions. Is this OK for trunk? >> >> Thanks, >> Kugan >> >> gcc/ >> +2013-10-15 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> >> + Kugan Vivekanandarajah <kuganv@linaro.org> >> + >> + PR target/59588 >> + * config/aarch64/aarch64.c (aarch64_build_constant): Fix incorrect >> + truncation. >> + >> >> >> gcc/testsuite/ >> +2014-01-11 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> >> + Kugan Vivekanandarajah <kuganv@linaro.org> >> + >> + PR target/59695 >> + * g++.dg/pr59695.C: New file. >> + >> >> >> p.txt >> >> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> index 3d32ea5..854666f 100644 >> --- a/gcc/config/aarch64/aarch64.c >> +++ b/gcc/config/aarch64/aarch64.c >> @@ -2486,7 +2486,7 @@ aarch64_build_constant (int regnum, HOST_WIDE_INT val) >> if (ncount < zcount) >> { >> emit_move_insn (gen_rtx_REG (Pmode, regnum), >> - GEN_INT ((~val) & 0xffff)); >> + GEN_INT (~((~val) & 0xffff))); > > I think that would be better written as > > GEN_INT (val | ~(HOST_WIDE_INT) 0xffff); > > Note the cast after the ~ to ensure we invert the right number of bits. > > Otherwise OK. > Thanks Richard. Is this OK for back-porting to 4.8 as well? Thanks, Kugan
On 15/01/14 10:38, Kugan wrote: > On 13/01/14 21:05, Richard Earnshaw wrote: >> On 11/01/14 23:42, Kugan wrote: >>> Hi, >>> >>> aarch64_build_constant incorrectly truncates the immediate when >>> constants are generated with MOVN. This causes coinor-osi tests to fail >>> (tracked also in https://bugs.launchpad.net/gcc-linaro/+bug/1263576) >>> >>> Attached patch fixes this. Also attaching a reduced testcase that >>> reproduces this. Tested on aarch64-none-linux-gnu with no new >>> regressions. Is this OK for trunk? >>> >>> Thanks, >>> Kugan >>> >>> gcc/ >>> +2013-10-15 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> >>> + Kugan Vivekanandarajah <kuganv@linaro.org> >>> + >>> + PR target/59588 >>> + * config/aarch64/aarch64.c (aarch64_build_constant): Fix incorrect >>> + truncation. >>> + >>> >>> >>> gcc/testsuite/ >>> +2014-01-11 Matthew Gretton-Dann <matthew.gretton-dann@linaro.org> >>> + Kugan Vivekanandarajah <kuganv@linaro.org> >>> + >>> + PR target/59695 >>> + * g++.dg/pr59695.C: New file. >>> + >>> >>> >>> p.txt >>> >>> >>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >>> index 3d32ea5..854666f 100644 >>> --- a/gcc/config/aarch64/aarch64.c >>> +++ b/gcc/config/aarch64/aarch64.c >>> @@ -2486,7 +2486,7 @@ aarch64_build_constant (int regnum, HOST_WIDE_INT val) >>> if (ncount < zcount) >>> { >>> emit_move_insn (gen_rtx_REG (Pmode, regnum), >>> - GEN_INT ((~val) & 0xffff)); >>> + GEN_INT (~((~val) & 0xffff))); >> >> I think that would be better written as >> >> GEN_INT (val | ~(HOST_WIDE_INT) 0xffff); >> >> Note the cast after the ~ to ensure we invert the right number of bits. >> >> Otherwise OK. >> > > Thanks Richard. Is this OK for back-porting to 4.8 as well? > Yes. R.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 3d32ea5..854666f 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -2486,7 +2486,7 @@ aarch64_build_constant (int regnum, HOST_WIDE_INT val) if (ncount < zcount) { emit_move_insn (gen_rtx_REG (Pmode, regnum), - GEN_INT ((~val) & 0xffff)); + GEN_INT (~((~val) & 0xffff))); tval = 0xffff; } else diff --git a/gcc/testsuite/g++.dg/pr59695.C b/gcc/testsuite/g++.dg/pr59695.C index e69de29..0da06cb 100644 --- a/gcc/testsuite/g++.dg/pr59695.C +++ b/gcc/testsuite/g++.dg/pr59695.C @@ -0,0 +1,125 @@ + +/* PR target/53055 */ +/* { dg-do run { target aarch64*-*-* } } */ +/* { dg-options "-O0" } */ + +#define DEFINE_VIRTUALS_FNS(i) virtual void xxx##i () {} \ + virtual void foo1_##i () {}\ + virtual void foo2_##i () {}\ + virtual void foo3_##i () {}\ + virtual void foo4_##i () {}\ + virtual void foo5_##i () {}\ + virtual void foo6_##i () {}\ + virtual void foo7_##i () {}\ + virtual void foo8_##i () {}\ + virtual void foo9_##i () {}\ + virtual void foo10_##i () {}\ + virtual void foo11_##i () {}\ + virtual void foo12_##i () {}\ + virtual void foo13_##i () {}\ + virtual void foo14_##i () {}\ + virtual void foo15_##i () {}\ + virtual void foo16_##i () {}\ + virtual void foo17_##i () {}\ + virtual void foo18_##i () {}\ + virtual void foo19_##i () {}\ + virtual void foo20_##i () {}\ + virtual void foo21_##i () {}\ + virtual void foo22_##i () {}\ + +class base_class_2 +{ + +public: + /* Define lots of virtual functions */ + DEFINE_VIRTUALS_FNS (1) + DEFINE_VIRTUALS_FNS (2) + DEFINE_VIRTUALS_FNS (3) + DEFINE_VIRTUALS_FNS (4) + DEFINE_VIRTUALS_FNS (5) + DEFINE_VIRTUALS_FNS (6) + DEFINE_VIRTUALS_FNS (7) + DEFINE_VIRTUALS_FNS (8) + DEFINE_VIRTUALS_FNS (9) + DEFINE_VIRTUALS_FNS (10) + DEFINE_VIRTUALS_FNS (11) + DEFINE_VIRTUALS_FNS (12) + DEFINE_VIRTUALS_FNS (13) + DEFINE_VIRTUALS_FNS (14) + DEFINE_VIRTUALS_FNS (15) + DEFINE_VIRTUALS_FNS (16) + DEFINE_VIRTUALS_FNS (17) + DEFINE_VIRTUALS_FNS (18) + DEFINE_VIRTUALS_FNS (19) + DEFINE_VIRTUALS_FNS (20) + + base_class_2(); + virtual ~base_class_2 (); +}; + +base_class_2::base_class_2() +{ +} + +base_class_2::~base_class_2 () +{ +} + +class base_class_1 +{ +public: + virtual ~base_class_1(); + base_class_1(); +}; + +base_class_1::base_class_1() +{ +} + +base_class_1::~base_class_1() +{ +} + +class base_Impl_class : + virtual public base_class_2, public base_class_1 +{ +public: + base_Impl_class (); + virtual ~base_Impl_class (); +}; + +base_Impl_class::base_Impl_class () +{ +} + +base_Impl_class::~base_Impl_class () +{ +} + + +class test_cls : public base_Impl_class +{ +public: + test_cls(); + virtual ~test_cls(); +}; + +test_cls::test_cls() +{ +} + +test_cls::~test_cls() +{ +} + +int main() +{ + test_cls *test = new test_cls; + base_class_2 *p1 = test; + + /* PR 53055 destructor thunk offsets are not setup + correctly resulting in crash. */ + delete p1; + return 0; +} +