[PATCH] Fix stack red zone bug (PR38644)

[Richard Sandiford]

"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.

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.

Richard