RFR: 8080775: Better argument formatting for assert() and friends
David Lindholm
david.lindholm at oracle.com
Fri Sep 25 12:52:56 UTC 2015
Per and Kim,
The latest webrev is here:
http://cr.openjdk.java.net/~david/JDK-8080775/webrev.02/
It contains fixes for Pers comments and for the comments from Kim that I
have gotten so far.
Thanks,
David
On 2015-09-25 08:15, Per Liden wrote:
> Hi,
>
> On 2015-09-24 16:25, David Lindholm wrote:
>> Hi Per,
>>
>> Thanks for the review! Comments inline.
>>
>>
>> On 2015-09-24 14:56, Per Liden wrote:
>>> This is a really nice cleanup! Just a few comments:
>>
>> Thanks!
>>
>>>
>>> g1AllocRegion.inline.hpp:
>>> -------------------------
>>> Could we rename G1_ALLOC_REGION_ASSERT() to something like
>>> assert_alloc_region(), to match the style used by assert_heap_locked()
>>> and friends in g1CollectedHeap.hpp.
>>
>> Fixed.
>>
>>> heapRegionSet.hpp/heapRegionSet.cpp:
>>> ------------------------------------
>>> It feels like we should get rid of hrs_err_msg and hrs_ext_msg here
>>> also, and replace it with an assert_heap_region_set() or something, or
>>> is there a reason why we're keeping those?
>>
>> Yes, per Kims suggestion I will add another RFE to fix this in another
>> changeset.
>>
>>> vmError.cpp:
>>> ------------
>>> VMError::_current_step could be a local variable in report() instead
>>> of a static.
>>
>> Yes, but the local variable must still be static, otherwise
>> crashreporting during crashreporting won't work.
>
> Ah, ok. In that case I think it's better to leave next to
> current_step_info, since that are tightly connected to each other.
>
> cheers,
> /Per
>
>>
>>> VMError::_current_step_info could be reset to "" in the END macro,
>>> instead of outside of report().
>>
>> Yes, I reset both current_step_info and current_step in END now.
>>
>>> VMError::_verbose could be an argument to report() instead of a static.
>>
>> Fixed.
>>
>>
>> Thanks,
>> David
>>
>>>
>>> cheers,
>>> /Per
>>>
>>>
>>>> I will also file a new RFE to fix src/share/vm/gc/g1/heapRegionSet.cpp
>>>> per your suggestion.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>> On 2015-09-23 20:32, Kim Barrett wrote:
>>>>> On Sep 22, 2015, at 7:38 AM, David Lindholm
>>>>> <david.lindholm at oracle.com> wrote:
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8080775
>>>>>> Webrev: http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/
>>>>> Another batch, this time covering gc through memory. I need to
>>>>> take a
>>>>> break from this, but I should be able to finish tomorrow.
>>>>>
>>>>> Mostly formatting issues. BTW, although I'm finding some of
>>>>> these, I'm
>>>>> also noticing that you tidied up a lot more. Thanks for that.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/cms/compactibleFreeListSpace.cpp
>>>>> 2877 assert(n * word_sz == fc->size(),
>>>>> 2878 "Chunk size " SIZE_FORMAT " is not exactly splittable by "
>>>>> 2879 SIZE_FORMAT " sized chunks of size " SIZE_FORMAT,
>>>>> 2880 fc->size(), n, word_sz);
>>>>>
>>>>> Pre-existing: Indentation of message arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/bufferingOopClosure.cpp
>>>>> 157 assert((got) ==
>>>>> (expected), \
>>>>> 158 "Expected: %d, got: %d, when running testCount(%d,
>>>>> %d, %d)", \
>>>>> 159 (got), (expected), num_narrow, num_full,
>>>>> do_oop_order)
>>>>>
>>>>> 194 assert((got) ==
>>>>> (expected), \
>>>>> 195 "Expected: %d, got: %d. testIsBufferEmptyOrFull(%d,
>>>>> %d, %s, %s)", \
>>>>> 196 (got), (expected), num_narrow,
>>>>> num_full, \
>>>>> 197 BOOL_TO_STR(expect_empty),
>>>>> BOOL_TO_STR(expect_full))
>>>>>
>>>>> 234 assert(boc.is_buffer_empty(),
>>>>> 235 "Should be empty after call to done().
>>>>> testEmptyAfterDone(%d, %d)",
>>>>> 236 num_narrow, num_full);
>>>>>
>>>>> Pre-existing: Indentation of message arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1AllocRegion.hpp
>>>>> 37 #define G1_ALLOC_REGION_MSG(message) \
>>>>> 38 "[%s] %s c: %u b: %s r: " PTR_FORMAT " u: " SIZE_FORMAT, \
>>>>> 39 _name, message, _count, BOOL_TO_STR(_bot_updates), \
>>>>> 40 p2i(_alloc_region), _used_bytes_before
>>>>>
>>>>> Misaligned line continuations.
>>>>>
>>>>> I think this doesn't belong in this header; it's not something that a
>>>>> client can use. Suggest moving it to g1AllocRegion.inline.hpp.
>>>>>
>>>>> Suggest changing this to G1_ALLOC_REGION_ASSERT, with predicate
>>>>> argument and full assert expression as expansion. Macros with
>>>>> unusual
>>>>> syntactic expansions can be confusing, so best avoided unless they
>>>>> provide some benefit that can't be obtained in some other way.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1AllocRegion.hpp
>>>>> 49 class G1AllocRegion VALUE_OBJ_CLASS_SPEC {
>>>>> 50 friend class ar_ext_msg;
>>>>>
>>>>> ar_ext_msg no longer exists.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1Allocator.cpp
>>>>> 432 assert((end_alignment_in_bytes >> LogHeapWordSize) <
>>>>> HeapRegion::min_region_size_in_words(),
>>>>> 433 "alignment " SIZE_FORMAT " too large",
>>>>> end_alignment_in_bytes);
>>>>>
>>>>> Pre-existing: Indentation of message arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1BiasedArray.cpp
>>>>> 44 guarantee(biased_index >= bias() && biased_index < (bias() +
>>>>> length()),
>>>>> 45 "Biased index out of bounds, index: " SIZE_FORMAT " bias: "
>>>>> SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(), length());
>>>>>
>>>>> 50 guarantee(biased_index >= bias() && biased_index <= (bias() +
>>>>> length()),
>>>>> 51 "Biased index out of inclusive bounds, index: " SIZE_FORMAT
>>>>> " bias: " SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(),
>>>>> length());
>>>>>
>>>>> Pre-existing: Indentation of message arguments.
>>>>>
>>>>> Maybe also split really long lines?
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1BlockOffsetTable.cpp
>>>>> 364 assert((_array->offset_array(orig_index) == 0 &&
>>>>> 365 blk_start == boundary) ||
>>>>> 366 (_array->offset_array(orig_index) > 0 &&
>>>>> 367 _array->offset_array(orig_index) <= N_words),
>>>>>
>>>>> [Pre-existing, and not modified by this changeset, but nearby.]
>>>>>
>>>>> Indentation of 366 and 367 are wrong.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1BlockOffsetTable.hpp
>>>>> 151 assert(offset <= N_words,
>>>>> 152 "%s - "
>>>>> 153 "offset: " SIZE_FORMAT ", N_words: %u",
>>>>> 154 msg, offset, (uint)N_words);
>>>>>
>>>>> Maybe eliminate the line break in the format string?
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1BlockOffsetTable.inline.hpp
>>>>> 95 assert(result >= _reserved.start() && result <
>>>>> _reserved.end(),
>>>>> 96 "bad address from index result " PTR_FORMAT
>>>>> 97 " _reserved.start() " PTR_FORMAT " _reserved.end() "
>>>>> 98 PTR_FORMAT,
>>>>> 99 p2i(result), p2i(_reserved.start()),
>>>>> p2i(_reserved.end()));
>>>>>
>>>>> Maybe eliminate the second line break in the format string?
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/g1CollectedHeap.cpp
>>>>> 2540 assert(_old_marking_cycles_started ==
>>>>> _old_marking_cycles_completed ||
>>>>> 2541 _old_marking_cycles_started == _old_marking_cycles_completed
>>>>> + 1,
>>>>> 2542 "Wrong marking cycle count (started: %d, completed: %d)",
>>>>> 2543 _old_marking_cycles_started, _old_marking_cycles_completed);
>>>>>
>>>>> Pre-existing: Indentation of arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/heapRegionSet.cpp
>>>>>
>>>>> Suggest eliminating hrs_ext_msg and hrs_err_msg in a manner
>>>>> similar to
>>>>> elimination of ar_ext_msg, taking into account earlier comments in
>>>>> that area.
>>>>>
>>>>> Please do that under a new RFE; this change set is quite larg enough
>>>>> already.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/g1/heapRegionType.hpp
>>>>> 30 #define hrt_assert_is_valid(tag) \
>>>>> 31 assert(is_valid((tag)), "invalid HR type: %u", (uint) (tag))
>>>>>
>>>>> Pre-existing: I don't think this should be a macro. I suggest it
>>>>> should be a private member function using NOT_DEBUG_RETURN and an
>>>>> ASSERT-only definition in the .cpp.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8137046
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/parallel/mutableNUMASpace.cpp
>>>>> 87 assert(words_to_fill >=
>>>>> CollectedHeap::min_fill_size(),
>>>>> 88 "Remaining size (" SIZE_FORMAT ") is too small to
>>>>> fill (based on " SIZE_FORMAT " and " SIZE_FORMAT ")",
>>>>> 89 words_to_fill, words_left_to_fill,
>>>>> CollectedHeap::filler_array_max_size());
>>>>>
>>>>> Pre-existing: Indentation of message arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/shared/gcCause.hpp
>>>>> 97 assert(cause !=
>>>>> GCCause::_old_generation_too_full_to_scavenge &&
>>>>> 98 cause !=
>>>>> GCCause::_old_generation_expanded_on_last_scavenge,
>>>>> 99 "This GCCause may be correct but is not expected yet:
>>>>> %s",
>>>>> 100 to_string(cause));
>>>>>
>>>>> Pre-existing: Indentation of arguments.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> src/share/vm/gc/shared/space.cpp
>>>>> 606 assert(oop(last)->is_oop(),PTR_FORMAT " should be an object
>>>>> start", p2i(last));
>>>>>
>>>>> Add space between comma and PTR_FORMAT.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
More information about the hotspot-dev
mailing list