RE: [PATCH] Fix stack red zone bug (PR38644)

["Jiangning Liu" <jiangning dot liu at arm dot com>]

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Friday, September 30, 2011 4:15 PM
> To: Jiangning Liu
> Cc: 'Jakub Jelinek'; 'Richard Guenther'; Andrew Pinski; gcc-
> patches@gcc.gnu.org
> Subject: Re: [PATCH] Fix stack red zone bug (PR38644)
> 
> "Jiangning Liu" <jiangning.liu@arm.com> writes:
> >> -----Original Message-----
> >> From: Jakub Jelinek [mailto:jakub@redhat.com]
> >> Sent: Thursday, September 29, 2011 6:14 PM
> >> To: Jiangning Liu
> >> Cc: 'Richard Guenther'; Andrew Pinski; gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH] Fix stack red zone bug (PR38644)
> >>
> >> On Thu, Sep 29, 2011 at 06:08:50PM +0800, Jiangning Liu wrote:
> >> > As far as I know different back-ends are implementing different
> >> > prologue/epilogue in GCC. If one day this part can be refined and
> >> abstracted
> >> > as well, I would say solving this stack-red-zone problem in shared
> >> > prologue/epilogue code would be a perfect solution, and barrier
> can
> >> be
> >> > inserted there.
> >> >
> >> > I'm not saying you are wrong on keeping scheduler using a pure
> >> barrier
> >> > interface. From engineering point of view, I only feel my proposal
> is
> >> so far
> >> > so good, because this patch at least solve the problem for all
> >> targets in a
> >> > quite simple way. Maybe it can be improved in future based on this.
> >>
> >> But you don't want to listen about any other alternative, other
> >> backends are
> >> happy with being able to put the best kind of barrier at the best
> spot
> >> in the epilogue and don't need a "generic" solution which won't
> model
> >> very
> >> well the target diversity anyway.
> >
> > Jakub,
> >
> > Appreciate for your attention on this issue,
> >
> > 1) Can you clarify who are the "others back-ends"? Does it cover most
> of the
> > back-ends being supported by GCC right now?
> 
> Not answering for Jakub of course, but as a maintainer of a backend, I
> know
> MIPS doesn't have the required barrier at the moment.  But that's a bug.
> 
> Like others in this thread, I'm strongly of the opinion that this
> should
> be modelled directly in the IL.  And it's already supposed to be
> modelled
> in the IL.  Target-independent code emits the required barriers in
> cases
> where it rather than the backend patterns are responsible.  E.g.:
> 
> 	  emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
> 	  emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
> 
> 	  emit_move_insn (hard_frame_pointer_rtx, fp);
> 	  emit_stack_restore (SAVE_NONLOCAL, stack);
> 
> from expand_builtin_longjmp() and:
> 
>   if (sa != 0)
>     {
>       sa = validize_mem (sa);
>       /* These clobbers prevent the scheduler from moving
> 	 references to variable arrays below the code
> 	 that deletes (pops) the arrays.  */
>       emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
>       emit_clobber (gen_rtx_MEM (BLKmode, stack_pointer_rtx));
>     }
> 
> from emit_stack_restore().  Backends that fail to follow suit are IMO
> just buggy.
> 
> FWIW, I intend to fix MIPS this weekend.

Richard S.,

Appreciate your attention on this issue and investigation on MIPS target.

Really glad to know you "find" a potential bug for MIPS through this
discussion. To some extension this proved my hypothesis previously.

> 
> You seem to feel strongly about this because it's a wrong-code bug that
> is very easy to introduce and often very hard to detect.  And I
> defintely
> sympathise with that.  If we were going to to do it in a target-
> independent
> way, though, I think it would be better to scan patterns like epilogue
> and
> automatically introduce barriers before assignments to
> stack_pointer_rtx
> (subject to the kind of hook in your patch).  But I still don't think
> that's better than leaving the onus on the backend.  The backend is
> still responsible for much more complicated things like determning
> the correct deallocation and register-restore sequence, and for
> determining the correct CFI sequence.
> 

I think middle-end in GCC is actually shared code rather than the part
exactly in the middle. A pass working on RTL can be a middle end just
because the code can be shared for all targets, and some passes can even
work for both GIMPLE and RTL.

Actually some optimizations need to work through "shared part" (middle-end)
plus "target specific part" (back-end). You are thinking the interface
between this "shared part" and "target specific part" should be using
"barrier" as a properly model. To some extension I agree with this. However,
it doesn't mean the fix should be in back-end rather than middle end,
because obviously this problem is a common ABI issue for all targets. If we
can abstract this issue to be a shared part, why shouldn't we do it in
middle end to reduce the onus of back-end? Back-end should handle the target
specific things rather than only the complicated things. 

If a complicated problem can be implemented in a "shared code" manner, we
still want to put it into middle end rather than back-end. I believe those
optimizations based on SSA form are complicated enough, but they are all in
middle end. This is the logic I'm seeing in GCC. For this particular issue,
I don't think that hook interface I'm proposing is more complicated than the
barrier. Instead, it is easier for back-end implementer to be aware of the
potential issue before really solving stack red zone problem, because it is
very clearly listed in target hook list.

You can fix MIPS, so how would the remaining targets be going? Do we need to
write document to warn all other targets on this somewhere? If we do this,
why don't we use hook-up code itself to document it?

Thanks,
-Jiangning

> Richard