From f74109bd178c92a9dff1ca6fce03b25f51a0384f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sikstr=C3=B6m?= Date: Fri, 9 Aug 2024 07:29:56 +0000 Subject: [PATCH] 8337939: ZGC: Make assertions and checks less convoluted and explicit Reviewed-by: stefank, ayang, eosterlund --- src/hotspot/share/gc/z/zAddress.cpp | 6 ++-- src/hotspot/share/gc/z/zAddress.inline.hpp | 30 +++++++++++++++---- src/hotspot/share/gc/z/zBarrier.inline.hpp | 6 ++-- src/hotspot/share/gc/z/zBarrierSet.inline.hpp | 4 +-- src/hotspot/share/gc/z/zHeapIterator.cpp | 2 +- src/hotspot/share/gc/z/zMark.cpp | 4 +-- src/hotspot/share/gc/z/zMark.inline.hpp | 4 +-- src/hotspot/share/gc/z/zPage.inline.hpp | 2 +- .../share/gc/z/zReferenceProcessor.cpp | 4 +-- src/hotspot/share/gc/z/zVerify.cpp | 24 +++++++++------ 10 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/hotspot/share/gc/z/zAddress.cpp b/src/hotspot/share/gc/z/zAddress.cpp index d1c199fad07..1cd33e44a05 100644 --- a/src/hotspot/share/gc/z/zAddress.cpp +++ b/src/hotspot/share/gc/z/zAddress.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -93,9 +93,7 @@ static void initialize_check_oop_function() { #ifdef CHECK_UNHANDLED_OOPS if (ZVerifyOops) { // Enable extra verification of usages of oops in oopsHierarchy.hpp - check_oop_function = [](oopDesc* obj) { - (void)to_zaddress(obj); - }; + check_oop_function = &check_is_valid_zaddress; } #endif } diff --git a/src/hotspot/share/gc/z/zAddress.inline.hpp b/src/hotspot/share/gc/z/zAddress.inline.hpp index 4adbf50bc86..bbc92a7e2aa 100644 --- a/src/hotspot/share/gc/z/zAddress.inline.hpp +++ b/src/hotspot/share/gc/z/zAddress.inline.hpp @@ -333,10 +333,22 @@ inline void dereferenceable_test(zaddress addr) { } #endif -inline zaddress to_zaddress(uintptr_t value) { - const zaddress addr = zaddress(value); +inline void check_is_valid_zaddress(zaddress addr) { assert_is_valid(addr); DEBUG_ONLY(dereferenceable_test(addr)); +} + +inline void check_is_valid_zaddress(uintptr_t value) { + check_is_valid_zaddress(zaddress(value)); +} + +inline void check_is_valid_zaddress(oopDesc* o) { + check_is_valid_zaddress(uintptr_t(o)); +} + +inline zaddress to_zaddress(uintptr_t value) { + const zaddress addr = zaddress(value); + check_is_valid_zaddress(addr); return addr; } @@ -344,7 +356,7 @@ inline zaddress to_zaddress(oopDesc* o) { return to_zaddress(uintptr_t(o)); } -inline oop to_oop(zaddress addr) { +inline void assert_is_oop_or_null(zaddress addr) { const oop obj = cast_to_oop(addr); assert(!ZVerifyOops || oopDesc::is_oop_or_null(obj), "Broken oop: " PTR_FORMAT " [" PTR_FORMAT " " PTR_FORMAT " " PTR_FORMAT " " PTR_FORMAT "]", p2i(obj), @@ -352,7 +364,16 @@ inline oop to_oop(zaddress addr) { *(uintptr_t*)(untype(addr) + 0x08), *(uintptr_t*)(untype(addr) + 0x10), *(uintptr_t*)(untype(addr) + 0x18)); - return obj; +} + +inline void assert_is_oop(zaddress addr) { + assert(!is_null(addr), "Should not be null"); + assert_is_oop_or_null(addr); +} + +inline oop to_oop(zaddress addr) { + assert_is_oop_or_null(addr); + return cast_to_oop(addr); } inline zaddress operator+(zaddress addr, size_t size) { @@ -378,7 +399,6 @@ inline void assert_is_valid(zaddress_unsafe addr) { DEBUG_ONLY(is_valid(addr, true /* assert_on_failure */);) } - inline uintptr_t untype(zaddress_unsafe addr) { return static_cast(addr); } diff --git a/src/hotspot/share/gc/z/zBarrier.inline.hpp b/src/hotspot/share/gc/z/zBarrier.inline.hpp index b3191e9ae3f..a3eb7a9ca67 100644 --- a/src/hotspot/share/gc/z/zBarrier.inline.hpp +++ b/src/hotspot/share/gc/z/zBarrier.inline.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -745,7 +745,7 @@ inline void ZBarrier::mark_and_remember(volatile zpointer* p, zaddress addr) { template inline void ZBarrier::mark(zaddress addr) { - assert(!ZVerifyOops || oopDesc::is_oop(to_oop(addr), false), "must be oop"); + assert_is_oop(addr); if (ZHeap::heap()->is_old(addr)) { ZGeneration::old()->mark_object_if_active(addr); @@ -757,7 +757,7 @@ inline void ZBarrier::mark(zaddress addr) { template inline void ZBarrier::mark_young(zaddress addr) { assert(ZGeneration::young()->is_phase_mark(), "Should only be called during marking"); - assert(!ZVerifyOops || oopDesc::is_oop(to_oop(addr), false), "must be oop"); + assert_is_oop(addr); assert(ZHeap::heap()->is_young(addr), "Must be young"); ZGeneration::young()->mark_object(addr); diff --git a/src/hotspot/share/gc/z/zBarrierSet.inline.hpp b/src/hotspot/share/gc/z/zBarrierSet.inline.hpp index d53b69345dd..174cdfd9e90 100644 --- a/src/hotspot/share/gc/z/zBarrierSet.inline.hpp +++ b/src/hotspot/share/gc/z/zBarrierSet.inline.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -430,7 +430,7 @@ public: template inline void ZBarrierSet::AccessBarrier::clone_in_heap(oop src, oop dst, size_t size) { - assert_is_valid(to_zaddress(src)); + check_is_valid_zaddress(src); if (dst->is_objArray()) { // Cloning an object array is similar to performing array copy. diff --git a/src/hotspot/share/gc/z/zHeapIterator.cpp b/src/hotspot/share/gc/z/zHeapIterator.cpp index 50fc921131f..e149a976add 100644 --- a/src/hotspot/share/gc/z/zHeapIterator.cpp +++ b/src/hotspot/share/gc/z/zHeapIterator.cpp @@ -146,7 +146,7 @@ private: oop load_oop(oop* p) { const oop o = Atomic::load(p); - assert_is_valid(to_zaddress(o)); + check_is_valid_zaddress(o); return RawAccess<>::oop_load(p); } diff --git a/src/hotspot/share/gc/z/zMark.cpp b/src/hotspot/share/gc/z/zMark.cpp index eb342495f57..d33b86c83e5 100644 --- a/src/hotspot/share/gc/z/zMark.cpp +++ b/src/hotspot/share/gc/z/zMark.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -365,7 +365,7 @@ void ZMark::follow_array_object(objArrayOop obj, bool finalizable) { } // Should be convertible to colorless oop - assert_is_valid(to_zaddress(obj)); + check_is_valid_zaddress(obj); zpointer* const addr = (zpointer*)obj->base(); const size_t length = (size_t)obj->length(); diff --git a/src/hotspot/share/gc/z/zMark.inline.hpp b/src/hotspot/share/gc/z/zMark.inline.hpp index b5302593610..9edc57a6000 100644 --- a/src/hotspot/share/gc/z/zMark.inline.hpp +++ b/src/hotspot/share/gc/z/zMark.inline.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -47,7 +47,7 @@ template inline void ZMark::mark_object(zaddress addr) { - assert(!ZVerifyOops || oopDesc::is_oop(to_oop(addr)), "Should be oop"); + assert_is_oop(addr); ZPage* const page = _page_table->get(addr); if (page->is_allocating()) { diff --git a/src/hotspot/share/gc/z/zPage.inline.hpp b/src/hotspot/share/gc/z/zPage.inline.hpp index fcf69c685f7..d8ecad57190 100644 --- a/src/hotspot/share/gc/z/zPage.inline.hpp +++ b/src/hotspot/share/gc/z/zPage.inline.hpp @@ -311,7 +311,7 @@ inline bool ZPage::mark_object(zaddress addr, bool finalizable, bool& inc_live) assert(is_in(addr), "Invalid address"); // Verify oop - (void)to_oop(addr); + assert_is_oop(addr); // Set mark bit const BitMap::idx_t index = bit_index(addr); diff --git a/src/hotspot/share/gc/z/zReferenceProcessor.cpp b/src/hotspot/share/gc/z/zReferenceProcessor.cpp index df8cb2b0e95..1252de2ac27 100644 --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -143,7 +143,7 @@ bool ZReferenceProcessor::is_inactive(zaddress reference, oop referent, Referenc return !is_null(reference_next(reference)); } else { // Verification - (void)to_zaddress(referent); + check_is_valid_zaddress(referent); // A non-FinalReference is inactive if the referent is null. The referent can only // be null if the application called Reference.enqueue() or Reference.clear(). diff --git a/src/hotspot/share/gc/z/zVerify.cpp b/src/hotspot/share/gc/z/zVerify.cpp index d47886ec7c2..b735965e9d4 100644 --- a/src/hotspot/share/gc/z/zVerify.cpp +++ b/src/hotspot/share/gc/z/zVerify.cpp @@ -111,6 +111,16 @@ static bool z_is_null_relaxed(zpointer o) { return (untype(o) & ~color_mask) == 0; } +static void z_verify_oop_object(zaddress addr, zpointer o, void* p) { + const oop obj = cast_to_oop(addr); + guarantee(oopDesc::is_oop(obj), BAD_OOP_ARG(o, p)); +} + +static void z_verify_root_oop_object(zaddress addr, void* p) { + const oop obj = cast_to_oop(addr); + guarantee(oopDesc::is_oop(obj), BAD_OOP_ARG(addr, p)); +} + static void z_verify_old_oop(zpointer* p) { const zpointer o = *p; assert(o != zpointer::null, "Old should not contain raw null"); @@ -121,7 +131,7 @@ static void z_verify_old_oop(zpointer* p) { // safepoint after reference processing, where we hold the driver lock and // know there is no concurrent remembered set processing in the young generation. const zaddress addr = ZPointer::uncolor(o); - guarantee(oopDesc::is_oop(to_oop(addr)), BAD_OOP_ARG(o, p)); + z_verify_oop_object(addr, o, p); } else { const zaddress addr = ZBarrier::load_barrier_on_oop_field_preloaded(nullptr, o); // Old to young pointers might not be mark good if the young @@ -143,15 +153,11 @@ static void z_verify_young_oop(zpointer* p) { guarantee(ZPointer::is_marked_young(o), BAD_OOP_ARG(o, p)); if (ZPointer::is_load_good(o)) { - guarantee(oopDesc::is_oop(to_oop(ZPointer::uncolor(o))), BAD_OOP_ARG(o, p)); + z_verify_oop_object(ZPointer::uncolor(o), o, p); } } } -static void z_verify_root_oop_object(zaddress o, void* p) { - guarantee(oopDesc::is_oop(to_oop(o)), BAD_OOP_ARG(o, p)); -} - static void z_verify_uncolored_root_oop(zaddress* p) { assert(!ZHeap::heap()->is_in((uintptr_t)p), "Roots shouldn't be in heap"); const zaddress o = *p; @@ -168,7 +174,7 @@ static void z_verify_possibly_weak_oop(zpointer* p) { const zaddress addr = ZBarrier::load_barrier_on_oop_field_preloaded(nullptr, o); guarantee(ZHeap::heap()->is_old(addr) || ZPointer::is_marked_young(o), BAD_OOP_ARG(o, p)); guarantee(ZHeap::heap()->is_young(addr) || ZHeap::heap()->is_object_live(addr), BAD_OOP_ARG(o, p)); - guarantee(oopDesc::is_oop(to_oop(addr)), BAD_OOP_ARG(o, p)); + z_verify_oop_object(addr, o, p); // Verify no missing remset entries. We are holding the driver lock here and that // allows us to more precisely verify the remembered set, as there is no concurrent @@ -211,14 +217,14 @@ public: // Minor collections could have relocated the object; // use load barrier to find correct object. const zaddress addr = ZBarrier::load_barrier_on_oop_field_preloaded(nullptr, o); - z_verify_root_oop_object(addr, p); + z_verify_oop_object(addr, o, p); } else { // Don't know the state of the oop if (is_valid(o)) { // it looks like a valid colored oop; // use load barrier to find correct object. const zaddress addr = ZBarrier::load_barrier_on_oop_field_preloaded(nullptr, o); - z_verify_root_oop_object(addr, p); + z_verify_oop_object(addr, o, p); } } }