Skip to content

8320275: assert(_chunk->bitmap().at(index)) failed: Bit not set at index #16837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pchilano wants to merge 5 commits into from
Closed

8320275: assert(_chunk->bitmap().at(index)) failed: Bit not set at index #16837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f117421
v1
pchilano Nov 27, 2023
247f041
remove round up on java_calling_convention
pchilano Nov 29, 2023
42478a4
add is_aligned assert in stackChunkOopDesc::bit_index_for
pchilano Nov 29, 2023
4f580f5
add comment in clear_bitmap_bits()
pchilano Nov 30, 2023
917c4b6
Merge branch 'master' into JDK-8320275
pchilano Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,

uint int_args = 0;
uint fp_args = 0;
uint stk_args = 0; // inc by 2 each time
uint stk_args = 0;

for (int i = 0; i < total_args_passed; i++) {
switch (sig_bt[i]) {
Expand All @@ -322,8 +322,9 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set1(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_VOID:
Expand All @@ -340,6 +341,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set2(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -348,15 +350,17 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set1(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_DOUBLE:
assert((i + 1) < total_args_passed && sig_bt[i + 1] == T_VOID, "expecting half");
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set2(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -367,7 +371,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
}
}

return align_up(stk_args, 2);
return stk_args;
}

// Patch the callers callsite with entry to compiled code if it exists.
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/cpu/arm/sharedRuntime_arm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,6 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
}
}

if (slot & 1) slot++;
return slot;
}

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
ShouldNotReachHere();
}
}
return align_up(stk, 2);
return stk;
}

#if defined(COMPILER1) || defined(COMPILER2)
Expand Down
12 changes: 8 additions & 4 deletions src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,

uint int_args = 0;
uint fp_args = 0;
uint stk_args = 0; // inc by 2 each time
uint stk_args = 0;

for (int i = 0; i < total_args_passed; i++) {
switch (sig_bt[i]) {
Expand All @@ -278,8 +278,9 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set1(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_VOID:
Expand All @@ -295,6 +296,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set2(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -303,15 +305,17 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set1(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_DOUBLE:
assert((i + 1) < total_args_passed && sig_bt[i + 1] == T_VOID, "expecting half");
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set2(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -321,7 +325,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
}
}

return align_up(stk_args, 2);
return stk_args;
}

// Patch the callers callsite with entry to compiled code if it exists.
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/s390/sharedRuntime_s390.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
ShouldNotReachHere();
}
}
return align_up(stk, 2);
return stk;
}

int SharedRuntime::c_calling_convention(const BasicType *sig_bt,
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/cpu/x86/sharedRuntime_x86_32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
}
}

// return value can be odd number of VMRegImpl stack slots make multiple of 2
return align_up(stack, 2);
return stack;
}

// Patch the callers callsite with entry to compiled code if it exists.
Expand Down
12 changes: 8 additions & 4 deletions src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,

uint int_args = 0;
uint fp_args = 0;
uint stk_args = 0; // inc by 2 each time
uint stk_args = 0;

for (int i = 0; i < total_args_passed; i++) {
switch (sig_bt[i]) {
Expand All @@ -510,8 +510,9 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set1(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_VOID:
Expand All @@ -528,6 +529,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (int_args < Argument::n_int_register_parameters_j) {
regs[i].set2(INT_ArgReg[int_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -536,15 +538,17 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set1(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set1(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
stk_args += 1;
}
break;
case T_DOUBLE:
assert((i + 1) < total_args_passed && sig_bt[i + 1] == T_VOID, "expecting half");
if (fp_args < Argument::n_float_register_parameters_j) {
regs[i].set2(FP_ArgReg[fp_args++]->as_VMReg());
} else {
stk_args = align_up(stk_args, 2);
regs[i].set2(VMRegImpl::stack2reg(stk_args));
stk_args += 2;
}
Expand All @@ -555,7 +559,7 @@ int SharedRuntime::java_calling_convention(const BasicType *sig_bt,
}
}

return align_up(stk_args, 2);
return stk_args;
}

// Patch the callers callsite with entry to compiled code if it exists.
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/c1/c1_FrameMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ CallingConvention* FrameMap::java_calling_convention(const BasicTypeArray* signa
}
}

intptr_t out_preserve = SharedRuntime::java_calling_convention(sig_bt, regs, sizeargs);
intptr_t out_preserve = align_up(SharedRuntime::java_calling_convention(sig_bt, regs, sizeargs), 2);
LIR_OprList* args = new LIR_OprList(signature->length());
for (i = 0; i < sizeargs;) {
BasicType t = sig_bt[i];
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/code/nmethod.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3019,7 +3019,7 @@ void nmethod::print_nmethod_labels(outputStream* stream, address block_begin, bo
assert(sig_index == sizeargs, "");
}
const char* spname = "sp"; // make arch-specific?
intptr_t out_preserve = SharedRuntime::java_calling_convention(sig_bt, regs, sizeargs);
SharedRuntime::java_calling_convention(sig_bt, regs, sizeargs);
int stack_slot_offset = this->frame_size() * wordSize;
int tab1 = 14, tab2 = 24;
int sig_index = 0;
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/oops/method.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,8 @@ class Method : public Metadata {
void remove_unshareable_flags() NOT_CDS_RETURN;

// the number of argument reg slots that the compiled method uses on the stack.
int num_stack_arg_slots() const { return constMethod()->num_stack_arg_slots(); }
int num_stack_arg_slots(bool rounded = true) const {
return rounded ? align_up(constMethod()->num_stack_arg_slots(), 2) : constMethod()->num_stack_arg_slots(); }

virtual void metaspace_pointers_do(MetaspaceClosure* iter);
virtual MetaspaceObj::Type type() const { return MethodType; }
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/oops/stackChunkOop.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class stackChunkOopDesc : public instanceOopDesc {

inline void* gc_data() const;
inline BitMapView bitmap() const;
inline BitMap::idx_t bit_index_for(intptr_t* p) const;
inline BitMap::idx_t bit_index_for(address p) const;
inline intptr_t* address_for_bit(BitMap::idx_t index) const;
template <typename OopT> inline BitMap::idx_t bit_index_for(OopT* p) const;
template <typename OopT> inline OopT* address_for_bit(BitMap::idx_t index) const;
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/oops/stackChunkOop.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,13 @@ inline BitMapView stackChunkOopDesc::bitmap() const {
return bitmap;
}

inline BitMap::idx_t stackChunkOopDesc::bit_index_for(intptr_t* p) const {
inline BitMap::idx_t stackChunkOopDesc::bit_index_for(address p) const {
return UseCompressedOops ? bit_index_for((narrowOop*)p) : bit_index_for((oop*)p);
}

template <typename OopT>
inline BitMap::idx_t stackChunkOopDesc::bit_index_for(OopT* p) const {
assert(is_aligned(p, alignof(OopT)), "should be aligned: " PTR_FORMAT, p2i(p));
assert(p >= (OopT*)start_address(), "Address not in chunk");
return p - (OopT*)start_address();
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/foreignGlobals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ int ForeignGlobals::compute_out_arg_bytes(const GrowableArray<VMStorage>& out_re

int ForeignGlobals::java_calling_convention(const BasicType* signature, int num_args, GrowableArray<VMStorage>& out_regs) {
VMRegPair* vm_regs = NEW_RESOURCE_ARRAY(VMRegPair, num_args);
int slots = SharedRuntime::java_calling_convention(signature, vm_regs, num_args);
int slots = align_up(SharedRuntime::java_calling_convention(signature, vm_regs, num_args), 2);
for (int i = 0; i < num_args; i++) {
VMRegPair pair = vm_regs[i];
// note, we ignore second here. Signature should consist of register-size values. So there should be
Expand Down
30 changes: 22 additions & 8 deletions src/hotspot/share/runtime/continuationFreezeThaw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,7 +1775,7 @@ class ThawBase : public StackObj {
inline void before_thaw_java_frame(const frame& hf, const frame& caller, bool bottom, int num_frame);
inline void after_thaw_java_frame(const frame& f, bool bottom);
inline void patch(frame& f, const frame& caller, bool bottom);
void clear_bitmap_bits(intptr_t* start, int range);
void clear_bitmap_bits(address start, address end);

NOINLINE void recurse_thaw_interpreted_frame(const frame& hf, frame& caller, int num_frames);
void recurse_thaw_compiled_frame(const frame& hf, frame& caller, int num_frames, bool stub_caller);
Expand Down Expand Up @@ -2166,13 +2166,22 @@ inline void ThawBase::patch(frame& f, const frame& caller, bool bottom) {
assert(!bottom || (_cont.is_empty() != Continuation::is_cont_barrier_frame(f)), "");
}

void ThawBase::clear_bitmap_bits(intptr_t* start, int range) {
void ThawBase::clear_bitmap_bits(address start, address end) {
assert(is_aligned(start, wordSize), "should be aligned: " PTR_FORMAT, p2i(start));
assert(is_aligned(end, VMRegImpl::stack_slot_size), "should be aligned: " PTR_FORMAT, p2i(end));

// we need to clear the bits that correspond to arguments as they reside in the caller frame
// or they will keep objects that are otherwise unreachable alive
log_develop_trace(continuations)("clearing bitmap for " INTPTR_FORMAT " - " INTPTR_FORMAT, p2i(start), p2i(start+range));
// or they will keep objects that are otherwise unreachable alive.

// Align `end` if UseCompressedOops is not set to avoid UB when calculating the bit index, since
// `end` could be at an odd number of stack slots from `start`, i.e might not be oop aligned.
// If that's the case the bit range corresponding to the last stack slot should not have bits set
// anyways and we assert that before returning.
address effective_end = UseCompressedOops ? end : align_down(end, wordSize);
Copy link
Member

@dean-long dean-long Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the align_down for correctness, or just for the benefit of the new assert at line 2179? Since it's not immediately obvious, I think it deserves a comment.

Copy link
Contributor Author

@pchilano pchilano Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because end is not necessarily word aligned anymore the pointer arithmetic we do in bit_index_for() would be UB, since p can point to the middle of an oop (in practice we would probably not see any issue because that's implemented as a substraction and then an arithmetic shift right which will round down the result). So we need to align end down if UseCompressedOops is not set. That last half word part should not contain an oop anyways so the assert is to verify that. I added a comment, please take a look.

log_develop_trace(continuations)("clearing bitmap for " INTPTR_FORMAT " - " INTPTR_FORMAT, p2i(start), p2i(effective_end));
stackChunkOop chunk = _cont.tail();
chunk->bitmap().clear_range(chunk->bit_index_for(start),
chunk->bit_index_for(start+range));
chunk->bitmap().clear_range(chunk->bit_index_for(start), chunk->bit_index_for(effective_end));
assert(chunk->bitmap().count_one_bits(chunk->bit_index_for(effective_end), chunk->bit_index_for(end)) == 0, "bits should not be set");
}

NOINLINE void ThawBase::recurse_thaw_interpreted_frame(const frame& hf, frame& caller, int num_frames) {
Expand Down Expand Up @@ -2225,7 +2234,9 @@ NOINLINE void ThawBase::recurse_thaw_interpreted_frame(const frame& hf, frame& c
_cont.tail()->fix_thawed_frame(caller, SmallRegisterMap::instance);
} else if (_cont.tail()->has_bitmap() && locals > 0) {
assert(hf.is_heap_frame(), "should be");
clear_bitmap_bits(heap_frame_bottom - locals, locals);
address start = (address)(heap_frame_bottom - locals);
address end = (address)heap_frame_bottom;
clear_bitmap_bits(start, end);
}

DEBUG_ONLY(after_thaw_java_frame(f, is_bottom_frame);)
Expand Down Expand Up @@ -2298,7 +2309,10 @@ void ThawBase::recurse_thaw_compiled_frame(const frame& hf, frame& caller, int n
// can only fix caller once this frame is thawed (due to callee saved regs); this happens on the stack
_cont.tail()->fix_thawed_frame(caller, SmallRegisterMap::instance);
} else if (_cont.tail()->has_bitmap() && added_argsize > 0) {
clear_bitmap_bits(heap_frame_top + ContinuationHelper::CompiledFrame::size(hf) + frame::metadata_words_at_top, added_argsize);
address start = (address)(heap_frame_top + ContinuationHelper::CompiledFrame::size(hf) + frame::metadata_words_at_top);
int stack_args_slots = f.cb()->as_compiled_method()->method()->num_stack_arg_slots(false /* rounded */);
Copy link
Member

@dean-long dean-long Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could trust the added_argsize value here, but that would require more changes to where rounding is done.

int argsize_in_bytes = stack_args_slots * VMRegImpl::stack_slot_size;
clear_bitmap_bits(start, start + argsize_in_bytes);
}

DEBUG_ONLY(after_thaw_java_frame(f, is_bottom_frame);)
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ void frame::describe(FrameValues& values, int frame_no, const RegisterMap* reg_m
assert(sig_index == sizeargs, "");
}
int stack_arg_slots = SharedRuntime::java_calling_convention(sig_bt, regs, sizeargs);
assert(stack_arg_slots == m->num_stack_arg_slots(), "");
assert(stack_arg_slots == m->num_stack_arg_slots(false /* rounded */), "");
int out_preserve = SharedRuntime::out_preserve_stack_slots();
int sig_index = 0;
int arg_index = (m->is_static() ? 0 : -1);
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/runtime/sharedRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2004,7 +2004,7 @@ void SharedRuntime::check_member_name_argument_is_last_argument(const methodHand
assert(member_arg_pos >= 0 && member_arg_pos < total_args_passed, "oob");
assert(sig_bt[member_arg_pos] == T_OBJECT, "dispatch argument must be an object");

int comp_args_on_stack = java_calling_convention(sig_bt, regs_without_member_name, total_args_passed - 1);
java_calling_convention(sig_bt, regs_without_member_name, total_args_passed - 1);

for (int i = 0; i < member_arg_pos; i++) {
VMReg a = regs_with_member_name[i].first();
Expand Down Expand Up @@ -3102,7 +3102,7 @@ void AdapterHandlerLibrary::create_native_wrapper(const methodHandle& method) {
BasicType ret_type = si.return_type();

// Now get the compiled-Java arguments layout.
int comp_args_on_stack = SharedRuntime::java_calling_convention(sig_bt, regs, total_args_passed);
SharedRuntime::java_calling_convention(sig_bt, regs, total_args_passed);

// Generate the compiled-to-native wrapper code
nm = SharedRuntime::generate_native_wrapper(&_masm, method, compile_id, sig_bt, regs, ret_type);
Expand Down
Loading