RFR: 8080775: Better argument formatting for assert() and friends
David Lindholm
david.lindholm at oracle.com
Thu Sep 24 14:25:38 UTC 2015
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.
> 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