Hi Jeff,
On Fri, Sep 22, 2017 at 11:49:04AM -0600, Jeff Law wrote:
> The go test suite has been run with and without this patch. I've found
> os/signal seems to flip between pass and fail without rhyme or reason.
> It may be related to system load. TestCgoCallbackGC now passes for
> reasons unknown. I haven't run it enough to know if it is sensitive to
> load or other timing factors.
Many Go tests flip-flop. I don't know why.
> I also flipped things so that clash protection is enabled by default and
> re-ran the tests. The idea being to see if I could exercise the path
> that uses SP_ADJUST a bit more. But that gave me the same results.
> While I think the change in the return value in
> rs6000_emit_probe_stack_range_stack_clash is more correct, I don't have
> a good way to test it.
Did you also test Ada? It needs testing. I wanted to try it myself,
but your patch doesn't apply (you included the changelog bits in the
patch), and I cannot easily manually apply it either because you sent
it as base64 instead of as text.
(... Okay, I think I have it working; testing now).
Some comments, mostly trivial comment stuff:
> +/* Allocate SIZE_INT bytes on the stack using a store with update style insn
> + and set the appropriate attributes for the generated insn. Return the
> + generated insn.
"Return the first generated insn"?
> + SIZE_INT is used to create the CFI note for the allocation.
> +
> + SIZE_RTX is an rtx containing the size of the adjustment. Note that
> + since stacks grow to lower addresses its runtime value is -SIZE_INT.
The size_rtx doesn't always correspond to size_int so this a bit
misleading.
(These comments were in the original code already, oh well).
> + COPY_REG, if non-null, should contain a copy of the original
> + stack pointer at exit from this function.
"Return a copy of the original stack pointer in COPY_REG if that is
non-null"? It wasn't clear to me that it is this function that should
set it :-)
> +static rtx_insn *
> +rs6000_emit_probe_stack_range_stack_clash (HOST_WIDE_INT orig_size,
> + rtx copy_reg)
> +{
> + rtx orig_sp = copy_reg;
> +
> + HOST_WIDE_INT probe_interval
> + = 1 << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
HOST_WIDE_INT_1U << ...
> + /* If explicitly requested,
> + or the rounded size is not the same as the original size
> + or the the rounded size is greater than a page,
> + then we will need a copy of the original stack pointer. */
> + if (rounded_size != orig_size
> + || rounded_size > probe_interval
> + || copy_reg)
> + {
> + /* If the caller requested a copy of the incoming stack pointer,
> + then ORIG_SP == COPY_REG and will not be NULL.
> +
> + If no copy was requested, then we use r0 to hold the copy. */
> + if (orig_sp == NULL_RTX)
> + orig_sp = gen_rtx_REG (Pmode, 0);
> + emit_move_insn (orig_sp, stack_pointer_rtx);
Maybe just write the "if" as "if (!copy_reg)"? You can lose the first
half of the comment then (since it is obvious then).
> + for (int i = 0; i < rounded_size; i += probe_interval)
> + {
> + rtx_insn *insn
> + = rs6000_emit_allocate_stack_1 (probe_interval,
> + probe_int, orig_sp);
> + if (!retval)
> + retval = insn;
> + }
Maybe "if (i == 0)" is clearer?
> @@ -25509,6 +25703,23 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx
> copy_reg, int copy_off)
> warning (0, "stack limit expression is not supported");
> }
>
> + if (flag_stack_clash_protection)
> + {
> + if (size < (1 << PARAM_VALUE
> (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE)))
HOST_WIDE_INT_1U again.
> +/* Probe a range of stack addresses from REG1 to REG3 inclusive. These are
> + absolute addresses. REG2 contains the backchain that must be stored into
> + *sp at each allocation.
I would just remove "These are absolute addresses.", or write something
like "These are addresses, not offsets", but that is kind of obvious
isn't it ;-)
> +static const char *
> +output_probe_stack_range_stack_clash (rtx reg1, rtx reg2, rtx reg3)
> +{
> + static int labelno = 0;
> + char loop_lab[32];
> + rtx xops[3];
> +
> + HOST_WIDE_INT probe_interval
> + = 1 << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
Once more :-) Maybe a helper function is in order? Would avoid the
huge length names at least.
Bootstrap+testsuite finished on BE, but I forgot to enable stack-clash
protection by default, whoops. Will have results later today (also LE).
Segher
