Skip to content

8366717: [lworld] Cleanup defensive fixing of JDK-8365996 #1824

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

Open
marc-chevalier wants to merge 13 commits into
base: lworld
Choose a base branch
from
Open

8366717: [lworld] Cleanup defensive fixing of JDK-8365996 #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5cdce25
Remove push_CPU_state in g1BarrierSetAssembler_aarch64.cpp
marc-chevalier Dec 12, 2025
e3d7c53
Add push_call_clobbered_registers in g1BarrierSetAssembler_aarch64.cpp
marc-chevalier Dec 12, 2025
ac19049
Asserting
marc-chevalier Dec 15, 2025
1a53198
Try the thing in oop_store_at
marc-chevalier Dec 15, 2025
e2220aa
Actually no
marc-chevalier Dec 16, 2025
7ae82a0
unpack_inline_helper
marc-chevalier Dec 16, 2025
dbe7c69
gen_c2i_adapter
marc-chevalier Dec 16, 2025
e234616
Experimenting g1_write_barrier_pre
marc-chevalier Dec 16, 2025
c749000
Cleanup
marc-chevalier Dec 17, 2025
4adfc04
assert in oop_store_at
marc-chevalier Dec 17, 2025
6696808
oop_store_at
marc-chevalier Dec 17, 2025
3f013c8
Merge remote-tracking branch 'origin/lworld' into JDK-8366717.investi…
marc-chevalier Dec 18, 2025
bf65d11
Pick the right comment again
marc-chevalier Dec 19, 2025
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
9 changes: 3 additions & 6 deletions src/hotspot/cpu/aarch64/gc/g1/g1BarrierSetAssembler_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,8 @@ void G1BarrierSetAssembler::g1_write_barrier_pre(MacroAssembler* masm,
// conventions for inline types. Save all argument registers before calling
// into the runtime.

// TODO 8366717 This came with 8284161: Implementation of Virtual Threads (Preview) later in May 2022
// Check if it's sufficient
//__ push_call_clobbered_registers();
assert_different_registers(rscratch1, pre_val); // push_CPU_state trashes rscratch1
__ push_CPU_state(true);
assert_different_registers(rscratch1, pre_val); // push_call_clobbered_registers trashes rscratch1
__ push_call_clobbered_registers();

// Calling the runtime using the regular call_VM_leaf mechanism generates
// code (generated by InterpreterMacroAssember::call_VM_leaf_base)
Expand All @@ -238,7 +235,7 @@ void G1BarrierSetAssembler::g1_write_barrier_pre(MacroAssembler* masm,
__ call_VM_leaf(CAST_FROM_FN_PTR(address, G1BarrierSetRuntime::write_ref_field_pre_entry), pre_val, thread);
}

__ pop_CPU_state(true);
__ pop_call_clobbered_registers();

__ bind(done);

Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/cpu/aarch64/gc/shared/cardTableBarrierSetAssembler_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ void CardTableBarrierSetAssembler::oop_store_at(MacroAssembler* masm, DecoratorS
// flatten object address if needed
if (!precise || (dst.index() == noreg && dst.offset() == 0)) {
if (tmp3 != noreg) {
// TODO 8366717 This change is from before the 'tmp3' arg was added to mainline, check if it's still needed. Same on x64. Also, this should be a __ lea
// Called by MacroAssembler::pack_inline_helper. We cannot corrupt the dst.base() register
// When tmp3 is given, we cannot corrupt the dst.base() register (from MacroAssembler::pack_inline_helper or do_oop_store)
Copy link
Member

@TobiHartmann TobiHartmann Dec 19, 2025

Choose a reason for hiding this comment

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

it doesn't look like an accident. If we remove the if-branch, tests are totally on fire. Let's keep it.

It's only the __ mov(tmp3, dst.base()); that could potentially be removed, right? Not the entire branch. If the mov is still needed, should it be guarded by InlineTypePassFieldsAsArgs?

Copy link
Member Author

@marc-chevalier marc-chevalier Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can just remove the mov part.

I don't really understand how to interpret that. We have

        __ mov(tmp3, dst.base());
        store_check(masm, tmp3, dst);

do you suggest we write

        store_check(masm, tmp3, dst);

? But then, tmp3 is not set to anything meaningful yet. Or

        store_check(masm, dst.base(), dst);

But that is exactly the else-branch. Moreover, I've tried to guard and it makes some test fails. For instance, we can come from

CardTableBarrierSetAssembler::oop_store_at (cardTableBarrierSetAssembler_x86.cpp:184)
CardTableBarrierSetAssembler::store_at (cardTableBarrierSetAssembler_x86.cpp:90)
MacroAssembler::store_heap_oop (macroAssembler_x86.cpp:5515)
do_oop_store (templateTable_x86.cpp:148)
(called, for instance, from TemplateTable::putfield_or_static_helper (templateTable_x86.cpp:2964))

that doesn't need InlineTypePassFieldsAsArgs but will give tmp3 and if we don't use it here (for instance, if I replace the condition by InlineTypePassFieldsAsArgs && tmp3 != noreg), we get various test failures.

Copy link
Member Author

@marc-chevalier marc-chevalier Dec 19, 2025

Choose a reason for hiding this comment

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

I don't understand why it's not a problem on mainline, tho. And I agree it's weird. I've looked, but it's hard to investigate why something works rather than not (on a somewhat diverging codebase). I spent some time on that, but eventually decided it's not that critical, as long as we have a way to make it work for Valhalla too.

Copy link
Member

@TobiHartmann TobiHartmann Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was suggesting that we should do what mainline does if !InlineTypePassFieldsAsArgs, would that make sense?

I.e. do store_check(masm, dst.base(), dst);

Copy link
Member Author

@marc-chevalier marc-chevalier Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That is the else-branch, right? Right now we have

if (tmp3 != noreg) {
  __ mov(tmp3, dst.base());
  store_check(masm, tmp3, dst);
} else {
  // It's OK to corrupt the dst.base() register.
  store_check(masm, dst.base(), dst);
}

If I understand well, you're suggesting to write

if (InlineTypePassFieldsAsArgs) {
  if (tmp3 != noreg) {
    __ mov(tmp3, dst.base());
    store_check(masm, tmp3, dst);
  } else {
    // It's OK to corrupt the dst.base() register.
    store_check(masm, dst.base(), dst);
  }
} else {
  // as mainline
  store_check(masm, dst.base(), dst);
}

but to me, it looks equivalent to

if (InlineTypePassFieldsAsArgs && tmp3 != noreg) {
  __ mov(tmp3, dst.base());
  store_check(masm, tmp3, dst);
} else {
  // It's OK to corrupt the dst.base() register.
  store_check(masm, dst.base(), dst);
}

since both else-branch are identical.

And this, I experimentally found that it's failing quite a lot, with backtraces as I mentioned. Is there an obvious mistake in my logic I'm missing?

Copy link
Member

@TobiHartmann TobiHartmann Dec 19, 2025

Choose a reason for hiding this comment

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

Ah, you are right. I missed the outer if (tmp3 != noreg) { which is Valhalla specific. As we discussed off-thread, it would be good to get a better understanding of why this fails in Valhalla. Thanks a lot!

__ mov(tmp3, dst.base());
store_check(masm, tmp3, dst);
} else {
Expand Down
12 changes: 9 additions & 3 deletions src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7162,11 +7162,18 @@ bool MacroAssembler::unpack_inline_helper(const GrowableArray<SigEntry>* sig, in

Label L_null, L_notNull;
// Don't use r14 as tmp because it's used for spilling (see MacroAssembler::spill_reg_for)
// TODO 8366717 We need to make sure that r14 (and potentially other long-life regs) are kept live in slowpath runtime calls in GC barriers
Register tmp1 = r10;
Register tmp2 = r11;

#ifndef ASSERT
RegSet clobbered_gp_regs = MacroAssembler::call_clobbered_gp_registers();
assert(clobbered_gp_regs.contains(tmp1), "tmp1 must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(tmp2), "tmp2 must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(r14), "r14 must be saved explicitly if it's not a clobber");
Copy link
Member

@TobiHartmann TobiHartmann Dec 19, 2025

Choose a reason for hiding this comment

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

Great idea with these asserts!

#endif

Register fromReg = noreg;
ScalarizedInlineArgsStream stream(sig, sig_index, to, to_count, to_index, -1);
ScalarizedInlineArgsStream stream(sig, sig_index, to, to_count, to_index, true);
bool done = true;
bool mark_done = true;
VMReg toReg;
Expand Down Expand Up @@ -7267,7 +7274,6 @@ bool MacroAssembler::unpack_inline_helper(const GrowableArray<SigEntry>* sig, in
}
}

// TODO 8366717 This is probably okay but looks fishy because stream is reset in the "Set null marker to zero" case just above. Same on x64.
sig_index = stream.sig_index();
to_index = stream.regs_index();

Expand Down
15 changes: 10 additions & 5 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 @@ -208,7 +208,6 @@ OopMap* RegisterSaver::save_live_registers(MacroAssembler* masm, int additional_
// will allow deoptimization at this safepoint to find all possible
// debug-info recordings, as well as let GC find all oops.

OopMapSet *oop_maps = new OopMapSet();
OopMap* oop_map = new OopMap(frame_size_in_slots, 0);

for (int i = 0; i < Register::number_of_registers; i++) {
Expand Down Expand Up @@ -620,18 +619,25 @@ static void gen_c2i_adapter(MacroAssembler *masm,

__ bind(skip_fixup);

// TODO 8366717 Is the comment about r13 correct? Isn't that r19_sender_sp?
// Name some registers to be used in the following code. We can use
// anything except r0-r7 which are arguments in the Java calling
// convention, rmethod (r12), and r13 which holds the outgoing sender
// convention, rmethod (r12), and r19 which holds the outgoing sender
// SP for the interpreter.
// TODO 8366717 We need to make sure that buf_array, buf_oop (and potentially other long-life regs) are kept live in slowpath runtime calls in GC barriers
Register buf_array = r10; // Array of buffered inline types
Register buf_oop = r11; // Buffered inline type oop
Register tmp1 = r15;
Register tmp2 = r16;
Register tmp3 = r17;

#ifndef ASSERT
RegSet clobbered_gp_regs = MacroAssembler::call_clobbered_gp_registers();
assert(clobbered_gp_regs.contains(buf_array), "buf_array must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(buf_oop), "buf_oop must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(tmp1), "tmp1 must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(tmp2), "tmp2 must be saved explicitly if it's not a clobber");
assert(clobbered_gp_regs.contains(tmp3), "tmp3 must be saved explicitly if it's not a clobber");
#endif

if (InlineTypePassFieldsAsArgs) {
// Is there an inline type argument?
bool has_inline_argument = false;
Expand All @@ -642,7 +648,6 @@ static void gen_c2i_adapter(MacroAssembler *masm,
// There is at least an inline type argument: we're coming from
// compiled code so we have no buffers to back the inline types
// Allocate the buffers here with a runtime call.
// TODO 8366717 Do we need to save vectors here? They could be used as arg registers, right? Same on x64.
RegisterSaver reg_save(true /* save_vectors */);
OopMap* map = reg_save.save_live_registers(masm, 0, &frame_size_in_words);

Expand Down
36 changes: 3 additions & 33 deletions src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,24 +236,8 @@ void G1BarrierSetAssembler::g1_write_barrier_pre(MacroAssembler* masm,
__ jcc(Assembler::equal, done);
generate_pre_barrier_slow_path(masm, obj, pre_val, thread, tmp, done);

if (Arguments::is_valhalla_enabled() && InlineTypePassFieldsAsArgs) {
// Barriers might be emitted when converting between (scalarized) calling conventions for inline
// types. Save all argument registers before calling into the runtime.
// TODO 8366717: use push_set() (see JDK-8283327 push/pop_call_clobbered_registers & aarch64 )
__ pusha();
__ subptr(rsp, 64);
__ movdbl(Address(rsp, 0), j_farg0);
__ movdbl(Address(rsp, 8), j_farg1);
__ movdbl(Address(rsp, 16), j_farg2);
__ movdbl(Address(rsp, 24), j_farg3);
__ movdbl(Address(rsp, 32), j_farg4);
__ movdbl(Address(rsp, 40), j_farg5);
__ movdbl(Address(rsp, 48), j_farg6);
__ movdbl(Address(rsp, 56), j_farg7);
} else {
// Determine and save the live input values
__ push_call_clobbered_registers();
}
// Determine and save the live input values
__ push_call_clobbered_registers();

// Calling the runtime using the regular call_VM_leaf mechanism generates
// code (generated by InterpreterMacroAssember::call_VM_leaf_base)
Expand All @@ -280,21 +264,7 @@ void G1BarrierSetAssembler::g1_write_barrier_pre(MacroAssembler* masm,
__ call_VM_leaf(CAST_FROM_FN_PTR(address, G1BarrierSetRuntime::write_ref_field_pre_entry), pre_val, thread);
}

if (Arguments::is_valhalla_enabled() && InlineTypePassFieldsAsArgs) {
// Restore registers
__ movdbl(j_farg0, Address(rsp, 0));
__ movdbl(j_farg1, Address(rsp, 8));
__ movdbl(j_farg2, Address(rsp, 16));
__ movdbl(j_farg3, Address(rsp, 24));
__ movdbl(j_farg4, Address(rsp, 32));
__ movdbl(j_farg5, Address(rsp, 40));
__ movdbl(j_farg6, Address(rsp, 48));
__ movdbl(j_farg7, Address(rsp, 56));
__ addptr(rsp, 64);
__ popa();
} else {
__ pop_call_clobbered_registers();
}
__ pop_call_clobbered_registers();

__ bind(done);
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/x86/gc/shared/cardTableBarrierSetAssembler_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ void CardTableBarrierSetAssembler::oop_store_at(MacroAssembler* masm, DecoratorS
// flatten object address if needed
if (!precise || (dst.index() == noreg && dst.disp() == 0)) {
if (tmp3 != noreg) {
// Called by MacroAssembler::pack_inline_helper. We cannot corrupt the dst.base() register
// When tmp3 is given, we cannot corrupt the dst.base() register (from MacroAssembler::pack_inline_helper or do_oop_store)
__ movptr(tmp3, dst.base());
store_check(masm, tmp3, dst);
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/x86/macroAssembler_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6096,7 +6096,7 @@ bool MacroAssembler::unpack_inline_helper(const GrowableArray<SigEntry>* sig, in
Register tmp1 = r10;
Register tmp2 = r13;
Register fromReg = noreg;
ScalarizedInlineArgsStream stream(sig, sig_index, to, to_count, to_index, -1);
ScalarizedInlineArgsStream stream(sig, sig_index, to, to_count, to_index, true);
bool done = true;
bool mark_done = true;
VMReg toReg;
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -918,7 +918,7 @@ static void gen_c2i_adapter(MacroAssembler *masm,
// There is at least an inline type argument: we're coming from
// compiled code so we have no buffers to back the inline types.
// Allocate the buffers here with a runtime call.
OopMap* map = RegisterSaver::save_live_registers(masm, 0, &frame_size_in_words, /*save_vectors*/ false);
OopMap* map = RegisterSaver::save_live_registers(masm, 0, &frame_size_in_words, /*save_wide_vectors*/ false);

frame_complete = __ offset();

Expand Down
6 changes: 3 additions & 3 deletions src/hotspot/share/runtime/signature_cc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ class ScalarizedInlineArgsStream : public StackObj {
int _regs_count;
int _regs_idx;
int _depth;
int _step;
const int _step;
DEBUG_ONLY(bool _finished);

public:
ScalarizedInlineArgsStream(const GrowableArray<SigEntry>* sig, int sig_idx, VMRegPair* regs, int regs_count, int regs_idx, int step = 1)
: _sig(sig), _sig_idx(sig_idx), _regs(regs), _regs_count(regs_count), _regs_idx(regs_idx), _step(step) {
ScalarizedInlineArgsStream(const GrowableArray<SigEntry>* sig, int sig_idx, VMRegPair* regs, int regs_count, int regs_idx, bool reverse = false)
: _sig(sig), _sig_idx(sig_idx), _regs(regs), _regs_count(regs_count), _regs_idx(regs_idx), _step(reverse ? -1 : 1) {
reset(sig_idx, regs_idx);
}

Expand Down