| Message ID | 20160311221624.GB16812@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
The underlying problem happens somewhere in tree-ssa-dse.c. So we get
an indirect jump to a random location instead of a jump to 0.
pr58164.c.035t.mergephi1
;; Function foo (foo, funcdef_no=0, decl_uid=1389, cgraph_uid=0, symbol_order=0)
foo ()
{
int x;
<bb 2>:
x = 0;
goto &x;
}
pr58164.c.036t.dse1
;; Function foo (foo, funcdef_no=0, decl_uid=1389, cgraph_uid=0, symbol_order=0)
Deleted dead store 'x = 0;
'
foo ()
{
int x;
<bb 2>:
goto &x;
}
On Sat, Mar 12, 2016 at 09:43:50AM +1030, Alan Modra wrote: > The underlying problem happens somewhere in tree-ssa-dse.c. So we get > an indirect jump to a random location instead of a jump to 0. Well, the testcase is there just to make sure we don't ICE on it. And, changing just DSE can't be a complete solution, because one can use uninitialized var from the beginning: int foo (void) { int x; goto *&x; } Jakub
On 03/12/2016 12:58 AM, Jakub Jelinek wrote: > On Sat, Mar 12, 2016 at 09:43:50AM +1030, Alan Modra wrote: >> The underlying problem happens somewhere in tree-ssa-dse.c. So we get >> an indirect jump to a random location instead of a jump to 0. > > Well, the testcase is there just to make sure we don't ICE on it. > And, changing just DSE can't be a complete solution, because one can use > uninitialized var from the beginning: > int > foo (void) > { > int x; > goto *&x; > } > > Jakub > I believe Alan's point is DSE deleted the assignment to x which can't be right as long as we've left in goto *&x. The goto *&x should be a use of x and thus should have kept the assignment live. Jeff
On Sat, Mar 12, 2016 at 01:26:39AM -0700, Jeff Law wrote: > On 03/12/2016 12:58 AM, Jakub Jelinek wrote: > >On Sat, Mar 12, 2016 at 09:43:50AM +1030, Alan Modra wrote: > >>The underlying problem happens somewhere in tree-ssa-dse.c. So we get > >>an indirect jump to a random location instead of a jump to 0. > > > >Well, the testcase is there just to make sure we don't ICE on it. > >And, changing just DSE can't be a complete solution, because one can use > >uninitialized var from the beginning: > >int > >foo (void) > >{ > > int x; > > goto *&x; > >} > > > > Jakub > > > I believe Alan's point is DSE deleted the assignment to x which can't be > right as long as we've left in goto *&x. > > The goto *&x should be a use of x and thus should have kept the assignment > live. Right, I wasn't trying to say that ira.c:indirect_jump_optimize is OK. It needs the patch I posted or perhaps even better a test of DF_REF_INSN_INFO rather than !DF_REF_IS_ARTIFICIAL (simply because the flag test is reading another field, and we need to read DF_REF_INSN_INFO anyway).
On Sat, Mar 12, 2016 at 07:37:25PM +1030, Alan Modra wrote: > > I believe Alan's point is DSE deleted the assignment to x which can't be > > right as long as we've left in goto *&x. > > > > The goto *&x should be a use of x and thus should have kept the assignment > > live. > > Right, I wasn't trying to say that ira.c:indirect_jump_optimize is > OK. It needs the patch I posted or perhaps even better a test of > DF_REF_INSN_INFO rather than !DF_REF_IS_ARTIFICIAL (simply because the > flag test is reading another field, and we need to read > DF_REF_INSN_INFO anyway). Ok, that was my point. BTW, DSE isn't the only one that deletes x = 0; cddce deletes it too. -fno-tree-dse -fno-tree-dce preserves it till expansion. Jakub
On March 12, 2016 10:29:40 AM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Sat, Mar 12, 2016 at 07:37:25PM +1030, Alan Modra wrote: >> > I believe Alan's point is DSE deleted the assignment to x which >can't be >> > right as long as we've left in goto *&x. >> > >> > The goto *&x should be a use of x and thus should have kept the >assignment >> > live. >> >> Right, I wasn't trying to say that ira.c:indirect_jump_optimize is >> OK. It needs the patch I posted or perhaps even better a test of >> DF_REF_INSN_INFO rather than !DF_REF_IS_ARTIFICIAL (simply because >the >> flag test is reading another field, and we need to read >> DF_REF_INSN_INFO anyway). > >Ok, that was my point. BTW, DSE isn't the only one that deletes x = 0; >cddce deletes it too. -fno-tree-dse -fno-tree-dce preserves it till >expansion. GIMPLE_GOTO doesn't have VOPs and I don't think that we'd want VUSEs on all gotos. But just having them on indirect gotos would be inconsistent. I believe the code is undefined anyway and out of scope of a reasonable QOI. Using alloca to create/jump to code on the stack should work (we might transform that into a decl though). Richard. > Jakub
On 03/12/2016 04:10 AM, Richard Biener wrote: > On March 12, 2016 10:29:40 AM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >> On Sat, Mar 12, 2016 at 07:37:25PM +1030, Alan Modra wrote: >>>> I believe Alan's point is DSE deleted the assignment to x which >> can't be >>>> right as long as we've left in goto *&x. >>>> >>>> The goto *&x should be a use of x and thus should have kept the >> assignment >>>> live. >>> >>> Right, I wasn't trying to say that ira.c:indirect_jump_optimize is >>> OK. It needs the patch I posted or perhaps even better a test of >>> DF_REF_INSN_INFO rather than !DF_REF_IS_ARTIFICIAL (simply because >> the >>> flag test is reading another field, and we need to read >>> DF_REF_INSN_INFO anyway). >> >> Ok, that was my point. BTW, DSE isn't the only one that deletes x = 0; >> cddce deletes it too. -fno-tree-dse -fno-tree-dce preserves it till >> expansion. > > GIMPLE_GOTO doesn't have VOPs and I don't think that we'd want VUSEs on all gotos. But just having them on indirect gotos would be inconsistent. > > I believe the code is undefined anyway and out of scope of a reasonable QOI. Undefined? Most likely. But we still have to do something sensible. As Jakub noted, a user could create the problematic code just as easily as DCE/DSE, so IRA probably needs to be tolerant of this situation. So it seems like you're suggesting we leave DCE/DSE alone (declaring this usage undefined) and fix IRA to be tolerant, right? > Using alloca to create/jump to code on the stack should work (we might transform that into a decl though). Given that executable stacks are a huge security hole, I'd be willing to go out on a limb and declare that undefined as well. It's not as clear cut, but that's the argument I'd make. And yes, I realize that goes in opposition to what GCC has allowed for 20+ years. I still think it'd be the right thing to do. jeff
On Sat, Mar 12, 2016 at 6:07 PM, Jeff Law <law@redhat.com> wrote: > On 03/12/2016 04:10 AM, Richard Biener wrote: >> >> On March 12, 2016 10:29:40 AM GMT+01:00, Jakub Jelinek <jakub@redhat.com> >> wrote: >>> >>> On Sat, Mar 12, 2016 at 07:37:25PM +1030, Alan Modra wrote: >>>>> >>>>> I believe Alan's point is DSE deleted the assignment to x which >>> >>> can't be >>>>> >>>>> right as long as we've left in goto *&x. >>>>> >>>>> The goto *&x should be a use of x and thus should have kept the >>> >>> assignment >>>>> >>>>> live. >>>> >>>> >>>> Right, I wasn't trying to say that ira.c:indirect_jump_optimize is >>>> OK. It needs the patch I posted or perhaps even better a test of >>>> DF_REF_INSN_INFO rather than !DF_REF_IS_ARTIFICIAL (simply because >>> >>> the >>>> >>>> flag test is reading another field, and we need to read >>>> DF_REF_INSN_INFO anyway). >>> >>> >>> Ok, that was my point. BTW, DSE isn't the only one that deletes x = 0; >>> cddce deletes it too. -fno-tree-dse -fno-tree-dce preserves it till >>> expansion. >> >> >> GIMPLE_GOTO doesn't have VOPs and I don't think that we'd want VUSEs on >> all gotos. But just having them on indirect gotos would be inconsistent. >> >> I believe the code is undefined anyway and out of scope of a reasonable >> QOI. > > Undefined? Most likely. But we still have to do something sensible. As > Jakub noted, a user could create the problematic code just as easily as > DCE/DSE, so IRA probably needs to be tolerant of this situation. > > So it seems like you're suggesting we leave DCE/DSE alone (declaring this > usage undefined) and fix IRA to be tolerant, right? Tolerant as in not crash? Yes. Note that DCE/DSE would be happy if the stores were global memory. After my recent fix even addresses based on functions and labels work here. What does not work is if you jump to automatic storage the compiler knows how to compute liveness of as gotos are not considered here. I can't think of a way to make the IL handle this without severely pessimizing regular DCE/DSE or making it very "hacky" in only giving VUSEs to gotos that possibly may reach "local" memory. That said, I can of course try and will once I see a testcase we break that matters in real life from a correctness perspective - like I did for that ARM kernel patching bug. >> Using alloca to create/jump to code on the stack should work (we might >> transform that into a decl though). > > Given that executable stacks are a huge security hole, I'd be willing to go > out on a limb and declare that undefined as well. It's not as clear cut, > but that's the argument I'd make. Well, I thought about somebody trying to build trampolines in a way exposed to GCC. > And yes, I realize that goes in opposition to what GCC has allowed for 20+ > years. I still think it'd be the right thing to do. Did we allow this? Not by design but rather by accident I suppose. Richard. > jeff >
On 03/14/2016 03:56 AM, Richard Biener wrote: >> Undefined? Most likely. But we still have to do something sensible. As >> Jakub noted, a user could create the problematic code just as easily as >> DCE/DSE, so IRA probably needs to be tolerant of this situation. >> >> So it seems like you're suggesting we leave DCE/DSE alone (declaring this >> usage undefined) and fix IRA to be tolerant, right? > > Tolerant as in not crash? Yes. Right. Tolerant as in not crash. > >>> Using alloca to create/jump to code on the stack should work (we might >>> transform that into a decl though). >> >> Given that executable stacks are a huge security hole, I'd be willing to go >> out on a limb and declare that undefined as well. It's not as clear cut, >> but that's the argument I'd make. > > Well, I thought about somebody trying to build trampolines in a way exposed > to GCC. Right or other dynamic, short-lived code fragments. > >> And yes, I realize that goes in opposition to what GCC has allowed for 20+ >> years. I still think it'd be the right thing to do. > > Did we allow this? Not by design but rather by accident I suppose. I don't think it was ever specifically allowed or disallowed; like many of the old extensions, it was never crisply defined. I can distinctly remember having to declare that taking the address of a blob of code on the stack, then calling/jumping to it after the containing function went out of scope as undefined. I think it was the address of a trampoline, but I'm not entirely sure -- there's a small chance it was user-created code. I only remember it because I was surprised at how controversial it was to declare that as undefined :( That most likely predates egcs, so the discussion is not likely in the public archives. It may have been a private discussion between Kenner, Jim, Doug and myself or some subset thereof. jeff
diff --git a/gcc/ira.c b/gcc/ira.c index 5e7a2ed..c4d76fc 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -3862,7 +3862,10 @@ indirect_jump_optimize (void) int regno = REGNO (SET_SRC (x)); if (DF_REG_DEF_COUNT (regno) == 1) { - rtx_insn *def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (regno)); + df_ref def = DF_REG_DEF_CHAIN (regno); + if (!DF_REF_IS_ARTIFICIAL (def)) + { + rtx_insn *def_insn = DF_REF_INSN (def); rtx note = find_reg_note (def_insn, REG_LABEL_OPERAND, NULL_RTX); if (note) @@ -3873,6 +3876,7 @@ indirect_jump_optimize (void) } } } + } if (rebuild_p) {