RFR: 8227713: Convert G1's assert_ macros into NOT_DEBUG_RETURN functions [v4]

[Kim Barrett]

On Sat, 23 Dec 2023 02:13:10 GMT, Lei Zaakjyu <duke at openjdk.org> wrote:

>> Description:
>> In G1CollectedHeap there are several "assert_*" macros (e.g. assert_heap_locked_and_not_at_safepoint) that were presumably implemented as macros due to bad stack traces in hs_err files. 
>> 
>> Convert them to functions using NOT_DEBUG_RETURN to avoid the macros.
>
> Lei Zaakjyu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use the 'DEBUG_ONLY' macro

There was some discussion about this sort of thing a long time ago, and there was not a consensus for it.  The problem is that turning these macros into functions loses source location in the error message, and a stack trace to see where the problem occurred isn't always available.  This could be addressed using std::source_location, but that isn't available until C++20.  Based on that discussion, I don't think this change should be made at this time.

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 318:

> 316:   inline static void assert_heap_not_locked_and_not_at_safepoint() NOT_DEBUG_RETURN;
> 317:   inline static void assert_at_safepoint_on_vm_thread() NOT_DEBUG_RETURN;
> 318:   inline static void assert_used_and_recalculate_used_equal(G1CollectedHeap* g1h) NOT_DEBUG_RETURN;

I think the intent was to leave these at namespace scope, rather than putting them in this class.
And perhaps move some of them to some more shared location, as many are pretty generic and
not specific to G1.

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 318:

> 316:   inline static void assert_heap_not_locked_and_not_at_safepoint() NOT_DEBUG_RETURN;
> 317:   inline static void assert_at_safepoint_on_vm_thread() NOT_DEBUG_RETURN;
> 318:   inline static void assert_used_and_recalculate_used_equal(G1CollectedHeap* g1h) NOT_DEBUG_RETURN;

assert_used_and_recalculate_used_equal could be an ordinary member function with out an explicit parameter.

src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp line 323:

> 321:   BOOL_TO_STR(Thread::current()->is_VM_thread())
> 322: 
> 323: DEBUG_ONLY(

We generally only use DEBUG_ONLY style macros for single small expressions, not for large multi-line
blocks of code.  For those we use `#ifdef ASSERT` and the like.

-------------

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17188#pullrequestreview-1795373926
PR Review Comment: https://git.openjdk.org/jdk/pull/17188#discussion_r1435441018
PR Review Comment: https://git.openjdk.org/jdk/pull/17188#discussion_r1435441256
PR Review Comment: https://git.openjdk.org/jdk/pull/17188#discussion_r1435440514