RFR: 8080775: Better argument formatting for assert() and friends

[Christian Thalinger]

> On Sep 22, 2015, at 10:19 PM, David Lindholm <david.lindholm at oracle.com> wrote:
> 
> Christian,
> 
> On 2015-09-22 18:31, Christian Thalinger wrote:
>>> On Sep 22, 2015, at 1:38 AM, David Lindholm <david.lindholm at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> Please review the following patch which removes the need to use err_msg() or err_msg_res() to format strings that are sent to assert(), guarantee(), fatal() and vm_exit_out_of_memory(). These are now variadic macros. So,
>>> 
>>> guarantee(_saved_index == _index, err_msg("saved index: %d index: %d", _saved_index, _index));
>>> 
>>> is replaced by
>>> 
>>> guarantee(_saved_index == _index, "saved index: %d index: %d", _saved_index, _index);
>> Excellent change!
> 
> Thank you!
> 
>> 
>>> This change also eliminates the need to choose if the buffer for the formatted string should be allocated on the stack (with err_msg()) or in the resource area (with err_msg_res()). There is no longer any need to allocate this extra buffer, since the va_list is sent all the way down to the functions writing the error to screen or file.
>> I’m not exactly sure but we might have ResourceMarks in the code just to have err_msg_res.  Did you see any cases like this?
> 
> I did not, but Kim Barrett have spotted one so far (I'll fix this).
> 
>> Also, does this mean there are no buffers allocated on the stack to format the message anymore?
> 
> Yes, that is correct.
> 
>> The reason we introduced err_msg_res was because Solaris Studio produced some bad code on SPARC which blew up the stack.
> 
> Yes, and this should not be needed any more.

I might be repeating myself but… excellent!

> 
> 
> Thanks,
> David
> 
>> 
>>> To accommodate for this, VMError is now AllStatic. All patterns of usages of this class was to create a VMError and immediately call report_and_die() on it.
>>> 
>>> Now, instead of having multiple constructors, there are multiple static VMError::report_and_die().
>>> 
>>> I have also removed all usages of err_msg() and err_msg_res() in assert(), guarantee(), fatal() and vm_exit_out_of_memory() in this patch. err_msg_res() is completely removed, err_msg() is still used by other functions.
>>> 
>>> Since we have about 1000 asserts with "" as detailed message, I had to disable the gcc-only warning for empty format strings.
>>> 
>>> (As a side note, there is also a JBS bug that describes a name change from assert to vmassert in our code. This changeset does not include such a name change, since we have over 19000 asserts in our code. We still have a "#define assert vmassert" just like before).
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8080775
>>> Webrev: http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/
>>> 
>>> 
>>> Testing: Passed JPRT
>>> 
>>> 
>>> Thanks,
>>> David