(jdk10) RFR(xxs): 8176872: [s390] wrong pc shown in error logs

[David Holmes]

On 4/04/2017 5:32 PM, Thomas Stüfe wrote:
> Hi David,
>
> On Tue, Apr 4, 2017 at 9:25 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     On 4/04/2017 3:54 AM, Thomas Stüfe wrote:
>
>         Hi Dmitry, David,
>
>         sorry for dropping the ball on this one. Here the latest version - I
>         only fixed the cosmetics Dmitry requested:
>
>         http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.02/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.02/webrev/>
>
>
>     Looks okay. I'm a little unsure about only fixing this in the shared
>     code for non-s390. But I guess we can deal with other platforms later.
>
>
> Not sure what you mean, but a common solution for all posix platforms
> would be even easier. Instead of fixing the s390 signal handler and the
> secondary signal handler for all posix platforms, one could adjust the
> pc at the entry of error reporting, in VMError::report_and_die(). But
> that would require an #ifndef WIN32 and you guys frown upon ifdefs, so
> that is why I avoided this solution.

Well there are frowns and there are frowns :) I don't like that the 
other platforms don't use si_addr too - seems the "right" way to get the 
pc and what would be done if writing the signal handler today. But then 
I also don't understand why handle_linux_signal is duplicated across CPU 
specific files! Maybe there are some subtle differences but it should be 
pretty OS specific rather than CPU specific.

Anyway ...

> But lets commit this solution, and if other platforms should ever have
> the same problem, we can modify the code.

... lets proceed as is.

Thanks,
David

> ..Thomas
>
>
>     Thanks,
>     David
>     -----
>
>         Thanks for reviewing!
>
>         Kind Regards, Thomas
>
>         On Sun, Mar 26, 2017 at 12:22 PM, Dmitry Samersoff
>         <dmitry.samersoff at oracle.com
>         <mailto:dmitry.samersoff at oracle.com>
>         <mailto:dmitry.samersoff at oracle.com
>         <mailto:dmitry.samersoff at oracle.com>>> wrote:
>
>             Thomas,
>
>             Looks good to me,
>
>             We may consider to always use info->si_addr.
>
>             Nits:
>
>             vmError_posix.cpp
>
>             118:
>               Please change uc ? ... to   (uc == NULL) ? ...
>
>             122: (and os_linux_s390.cpp:513)
>
>               Space missed after (address) ...
>
>
>             -Dmitry
>
>
>             On 2017-03-21 16:40, Thomas Stüfe wrote:
>             > Hi all,
>             >
>             > please take a look at this tiny fix. It fixes the pc shown
>         as faulting
>             > address for SIGILL and SIGFPE in hs_err files.
>             >
>             > https://bugs.openjdk.java.net/browse/JDK-8176872
>         <https://bugs.openjdk.java.net/browse/JDK-8176872>
>             <https://bugs.openjdk.java.net/browse/JDK-8176872
>         <https://bugs.openjdk.java.net/browse/JDK-8176872>>
>             >
>
>         http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.00/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.00/webrev/>
>
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.00/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8176872-s390-wrong-pc-in-errorlogs/jdk10-webrev.00/webrev/>>
>             >
>             > When determining the crash pc, in all posix platform signal
>             handlers pc is
>             > taken from the context. However, context.pc on zlinux
>         points to the
>             > instruction *after* the faulting op. The correct way,
>         according to
>             POSIX,
>             > would be to take the address from siginfo_t->si_addr for
>         signals
>             SIGILL,
>             > SIGFPE.
>             >
>             > (actually, this patch would make sense for all POSIX
>         platforms,
>             but only
>             > s390 seems to show this error, so I leave the patch local
>         to s390.)
>             >
>             > Kind Regards, Thomas
>             >
>
>
>             --
>             Dmitry Samersoff
>             Oracle Java development team, Saint Petersburg, Russia
>             * I would love to change the world, but they won't give me
>         the sources.
>
>
>