RFR: 8080775: Better argument formatting for assert() and friends
david.lindholm at oracle.com
Wed Sep 23 08:19:16 UTC 2015
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:
>> 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!
>> 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.
>> 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
More information about the hotspot-dev