(9) RFR: 8064815 Zero+PPC64: Stack overflow when running Maven

[Volker Simonis]

On Tue, Nov 18, 2014 at 11:15 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> Hi Volker,
>
> On Mon, 2014-11-17 at 20:14 +0100, Volker Simonis wrote:
>> On Mon, Nov 17, 2014 at 2:54 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
>> > Hi Volker,
>> >
>> > On Fri, 2014-11-14 at 14:46 +0100, Volker Simonis wrote:
>> >> Hi Severin,
>> >>
>> >> I can sponsor this change if we get one more review.
>> >
>> > Thanks!
>> >
>> >> The only comment I have is that in ZeroStack::suggest_size() there
>> >> doesn't seem to be a handling for the potentially negative values
>> >> returned by ZeroStack::abi_stack_available().
>> >
>> > OK. I could do something like this.
>> >
>> > diff --git a/src/cpu/zero/vm/stack_zero.cpp
>> > b/src/cpu/zero/vm/stack_zero.cpp
>> > --- a/src/cpu/zero/vm/stack_zero.cpp
>> > +++ b/src/cpu/zero/vm/stack_zero.cpp
>> > @@ -30,7 +30,9 @@
>> >
>> >  int ZeroStack::suggest_size(Thread *thread) const {
>> >    assert(needs_setup(), "already set up");
>> > -  return align_size_down(abi_stack_available(thread) / 2, wordSize);
>> > +  ssize_t abi_available = abi_stack_available(thread);
>> > +  assert(abi_available >= 0, "available abi stack must be >= 0");
>> > +  return align_size_down(abi_available / 2, wordSize);
>> >  }
>> >
>> > Did you have something else in mind? I'm not sure if it's necessary
>> > since suggest_size is only being called when the zero stack is set up
>> > (src/cpu/zero/vm/stubGenerator_zero.cpp). Thoughts?
>> >
>>
>> I think an assertion would be good.
>
> Sure. Updated webrev coming soon.
>
>> (but 'abi_available' should be an
>> 'int' because 'ssize_t' is only specified to hold values  from -1 to
>> SSIZE_MAX).
>
> OK, thanks! In that case I think it's safer to also change back the
> return type of abi_stack_available(). ssize_t worked since it's defined
> as signed int on linux.
>

I don't like ssize_t because of it's unclear semantics. I understand
that it happens to work on Linux because it's defined as a signed int
there. But then why not use an int in the first place? So I'd prefer
if you'd use an int.

Thanks,
Volker

>> As far as I can see, the possibly negative result of
>> ZeroStack::suggest_size() is casted into a size_t in
>> StubGenerator::call_stub() before doing an alloca() and just a few
>> lines later we call EntryFrame::build() which will do an overflow
>> check. Nevertheless its better to fail early in the case of a problem.
>
> Makes sense. Thanks for clarifications and the review!
>
> Cheers,
> Severin
>
>> Thank,
>> Volker
>>
>> > Thanks,
>> > Severin
>> >
>> >> On Fri, Nov 14, 2014 at 2:18 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
>> >> > Hi,
>> >> >
>> >> > Could I please get a review and sponsor for the following fix:
>> >> >
>> >> > bug: https://bugs.openjdk.java.net/browse/JDK-8064815
>> >> > webrev:
>> >> > https://jerboaa.fedorapeople.org/bugs/openjdk/JDK-8064815/webrev.0/
>> >> >
>> >> > When running Maven ("mvn") on a Zero variant build on PPC/PPC64 hardware
>> >> > it throws a StackOverflowError. This is because the stack bound
>> >> > calculation does not account for red and yellow pages.
>> >> >
>> >> > The bug has a slightly different patch attached. The changes to
>> >> > hotspot/src/os/linux/vm/os_linux.cpp aren't needed for this bug.
>> >> >
>> >> > Testing done: A Zero variant build of OpenJDK 9 on PPC/PPC64 throws
>> >> > StackOverflowError without this fix and works fine with this fix
>> >> > applied.
>> >> >
>> >> > Note that this problem seems to surface on architectures where pages are
>> >> > large. PPC is one such instance. Page size there is 64KB and Zero
>> >> > initially sets its minimal stack allowance to 64KB (one page),
>> >> > src/os_cpu/linux_zero/vm/os_linux_zero.cpp. In os::init_2 this gets
>> >> > potentially increased if min_stack_allowed is small. The case on PPC
>> >> > Zero.
>> >> >
>> >> > However, then later at runtime the calculation of available stack is
>> >> > wrong since it does not account for red and yellow pages. Thus it thinks
>> >> > there is too little stack available where in fact more stack is
>> >> > available.
>> >> >
>> >> > Thanks,
>> >> > Severin
>> >> >
>> >
>> >
>> >
>
>
>