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

[David Lindholm]

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.


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
>>