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