RFR: 8145073: Filename and linenumber are not printed for assers any more.

[David Lindholm]

David, Thomas,

I would also like to restore the old behavior for now, and thereby 
fixing this regression. We can change these APIs later.


Thanks
David L

On 2015-12-10 13:01, David Holmes wrote:
> Hi Thomas,
>
> On 10/12/2015 8:35 PM, Thomas Stüfe wrote:
>> Hi David, David,
>>
>> sorry about that and thank you for fixing!
>>
>> I changed that behaviour (inconsequentially, because I left out windows
>> and left the comment in) because the return code for
>> os::exception_name() is used in a number of places as input for
>> printing, without doing a null check first:
>>
>> e.g os_solaris_x86.cpp:
>>
>> 364        warning("Ignoring %s - see 4229104 or 6499219",
>> 365                os::exception_name(sig, buf, sizeof(buf)));
>
> But here sig is known to be SIGPIPE or SIGXFSZ so exception_name can 
> never return NULL.
>
>> e.g. os_solaris.cpp
>>
>> st->print("%s: ", os::exception_name(sig, buf, buflen));
>
> This is in print_signal_handler so sig will always be a signal value - 
> so again NULL is not possible.
>
>> So, I think it is besser to never return NULL at all. But if we go back
>> to the old behaviour, we should fix those callsites and add NULL checks
>> to them.
>
> I don't like the way NULL is used to indicate "hey you didn't really 
> pass me a signal number!". The _id in VMError is defined:
>
>   static int         _id;               // Solaris/Linux signals: 0 - 
> SIGRTMAX
>                                         // Windows exceptions: 
> 0xCxxxxxxx system errors
>                                         // 0x8xxxxxxx system warnings
>
> nothing there about assertions! But in fact it can have a range of 
> non-signal related values and primarily of interest in this case 
> INTERNAL_ERROR (0xe0000000). So while I don't like the way this is 
> managed, restoring the NULL behaviour is a lot simpler than trying to 
> rework everything.
>
> Thanks,
> David H.
> -------
>
>> Kind regards, Thomas
>>
>>
>>
>> On Thu, Dec 10, 2015 at 11:26 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi David,
>>
>>     On 10/12/2015 6:21 PM, David Lindholm wrote:
>>
>>         Hello Runtime,
>>
>>         Please review this patch that fixes the posix version of
>>         os::exception_name(). The problem is that this function in a 
>> recent
>>         change started returning a string saying "invalid (<num>)"
>>         instead of
>>         NULL for invalid exception numbers, breaking the API. Accoring
>>         to os.hpp
>>         the API is still:
>>
>>             // returns a string to describe the exception/signal;
>>             // returns NULL if exception_code is not an OS 
>> exception/signal.
>>
>>         .. and this is also what windows does. This change in return
>>         value for
>>         the posix version makes the NULL-checks for calls to this
>>         function in
>>         vmError.cpp invalid, leading to that we don't get filename and
>>         linenumber printed in the hs_err file for asserts (for example).
>>
>>         Bug: https://bugs.openjdk.java.net/browse/JDK-8145073
>>         Webrev: http://cr.openjdk.java.net/~david/JDK-8145073/webrev.00/
>>
>>
>>     Sorry about that. I worked with Thomas on his change in this area. I
>>     hadn't realized something was explicitly treating a NULL return to
>>     mean "not really a signal/exception" :(
>>
>>     Your fix to restore this behaviour looks good.
>>
>>     Thanks,
>>     David H.
>>
>>
>>         Testing: JPRT.
>>
>>
>>         Thanks,
>>         David
>>
>>
>>