Skip to content

Handle AArch32 immediates and literal/displacement ranges #23

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
fhunleth wants to merge 1 commit into from
Closed

Handle AArch32 immediates and literal/displacement ranges #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions erts/configure
View file
Open in desktop

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file looks like a cleanup to make the configure script more coherent. This should be a different commit. I see this file is generated by autoconf, does autoconf needs to be updated too or is this just a configure update that you committed while the configure.ac is already fixed ?

Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,7 @@ EMU_CC
ERTS_BUILD_FALLBACK_POLL
FLAVORS
PRIMARY_FLAVOR
JIT_ARCH_BITS
JIT_ARCH
JIT_ENABLED
M4
Expand Down Expand Up @@ -25459,6 +25460,7 @@ fi


JIT_ARCH=
JIT_ARCH_BITS=

if test ${enable_jit} != no
then :
Expand Down Expand Up @@ -25499,6 +25501,7 @@ printf "%s\n" "$as_me: WARNING: JIT disabled due to annoying popus on x86 Macs w
# instruction cache, and don't require us to manually issue
# instruction barriers on all threads.
JIT_ARCH=arm
JIT_ARCH_BITS=64
;;
*)
# We need to use `DC CVAU`, `IC IVAU`, and `ISB SY` to clear
Expand All @@ -25507,6 +25510,7 @@ printf "%s\n" "$as_me: WARNING: JIT disabled due to annoying popus on x86 Macs w

if test "$ethr_arm_isb_sy_instr_val$ethr_arm_dc_cvau_instr_val$ethr_arm_ic_ivau_instr_val" = "111"; then
JIT_ARCH=arm
JIT_ARCH_BITS=64
else
enable_jit=no
{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: WARNING: JIT disabled due to lack of cache-clearing instructions" >&5
Expand All @@ -25515,6 +25519,22 @@ printf "%s\n" "$as_me: WARNING: JIT disabled due to lack of cache-clearing instr
;;
esac
;;
arm)
case "$OPSYS" in
linux)
# These platforms have dedicated system calls for clearing
# instruction cache, and don't require us to manually issue
# instruction barriers on all threads.
JIT_ARCH=arm
JIT_ARCH_BITS=32
;;
*)
enable_jit=no
{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: WARNING: JIT disabled due to lack of support on $ARCH-$OPSYS" >&5
printf "%s\n" "$as_me: WARNING: JIT disabled due to lack of support on $ARCH-$OPSYS" >&2;}
;;
esac
;;
*)
if test ${enable_jit} = yes; then
as_fn_error $? "JIT only works on x86 64-bit and ARM 64-bit" "$LINENO" 5
Expand Down Expand Up @@ -25816,6 +25836,7 @@ fi




#
# Check for working poll().
#
Expand Down
143 changes: 128 additions & 15 deletions erts/emulator/beam/jit/arm/32/beam_asm.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ struct BeamAssembler : public BeamAssemblerCommon {
const int margin_bytes = margin_words * sizeof(Eterm);
Label next = a.newLabel();

a.sub(TMP, E, imm(margin_bytes));
sub(TMP, E, margin_bytes);
a.cmp(HTOP, TMP);

a.b_ls(next);
Expand Down Expand Up @@ -330,23 +330,23 @@ struct BeamAssembler : public BeamAssemblerCommon {
ERTS_CT_ASSERT((Spec & (Update::eReductions | Update::eStack |
Update::eHeap)) == Spec);
if (Spec & Update::eStack) {
a.str(E, arm::Mem(c_p, offsetof(Process, stop)));
this->safe_str(E, arm::Mem(c_p, offsetof(Process, stop)));
} else {
#ifdef DEBUG
/* Store some garbage in the process structure to catch missing
* updates. */
a.str(active_code_ix, arm::Mem(c_p, offsetof(Process, stop)));
this->safe_str(active_code_ix, arm::Mem(c_p, offsetof(Process, stop)));
#endif
}
if (Spec & Update::eHeap) {
a.str(HTOP, arm::Mem(c_p, offsetof(Process, htop)));
this->safe_str(HTOP, arm::Mem(c_p, offsetof(Process, htop)));
} else {
#ifdef DEBUG
a.str(active_code_ix, arm::Mem(c_p, offsetof(Process, htop)));
this->safe_str(active_code_ix, arm::Mem(c_p, offsetof(Process, htop)));
#endif
}
if (Spec & Update::eReductions) {
a.str(FCALLS, arm::Mem(c_p, offsetof(Process, fcalls)));
this->safe_str(FCALLS, arm::Mem(c_p, offsetof(Process, fcalls)));
}
// We do not have any X register cached in machine registers
// so nothing else needs to be saved.
Expand All @@ -358,13 +358,13 @@ struct BeamAssembler : public BeamAssemblerCommon {
(Spec & (Update::eReductions | Update::eStack | Update::eHeap |
Update::eCodeIndex)) == Spec);
if (Spec & Update::eStack) {
a.ldr(E, arm::Mem(c_p, offsetof(Process, stop)));
this->safe_ldr(E, arm::Mem(c_p, offsetof(Process, stop)));
}
if (Spec & Update::eHeap) {
a.ldr(HTOP, arm::Mem(c_p, offsetof(Process, htop)));
this->safe_ldr(HTOP, arm::Mem(c_p, offsetof(Process, htop)));
}
if (Spec & Update::eReductions) {
a.ldr(FCALLS, arm::Mem(c_p, offsetof(Process, fcalls)));
this->safe_ldr(FCALLS, arm::Mem(c_p, offsetof(Process, fcalls)));
}

if (Spec & Update::eCodeIndex) {
Expand Down Expand Up @@ -444,7 +444,7 @@ struct BeamAssembler : public BeamAssemblerCommon {
}

void emit_branch_if_eq(a32::Gp reg, Uint value, Label lbl) {
if (value <= 255) {
if (isAArch32Immediate(value)) {
a.cmp(reg, imm(value));
} else {
mov_imm(TMP, value);
Expand All @@ -454,7 +454,7 @@ struct BeamAssembler : public BeamAssemblerCommon {
}

void emit_branch_if_ne(a32::Gp reg, Uint value, Label lbl) {
if (value <= 255) {
if (isAArch32Immediate(value)) {
a.cmp(reg, imm(value));
} else {
mov_imm(TMP, value);
Expand Down Expand Up @@ -512,12 +512,17 @@ struct BeamAssembler : public BeamAssemblerCommon {
mov_imm(to, 0);
}

static bool isAArch32Immediate(uint64_t value) {
uint32_t encoded;
return arm::Utils::encodeAArch32Imm(value, &encoded);
}

void sub(a32::Gp to, a32::Gp src, int64_t val) {
if (val < 0) {
add(to, src, -val);
} else if (val == 0 && to != src) {
a.mov(to, src);
} else if (val <= 255) {
} else if (isAArch32Immediate(val)) {
a.sub(to, src, imm(val));
} else {
ASSERT(src != TMP);
Expand All @@ -531,7 +536,7 @@ struct BeamAssembler : public BeamAssemblerCommon {
sub(to, src, -val);
} else if (val == 0 && to != src) {
a.mov(to, src);
} else if (val <= 255) {
} else if (isAArch32Immediate(val)) {
a.add(to, src, imm(val));
} else {
ASSERT(src != TMP);
Expand All @@ -541,9 +546,9 @@ struct BeamAssembler : public BeamAssemblerCommon {
}

void subs(a32::Gp to, a32::Gp src, int64_t val) {
if (val >= 0 && val <= 255) {
if (val >= 0 && isAArch32Immediate(val)) {
a.subs(to, src, imm(val));
} else if (val < 0 && -val <= 255) {
} else if (val < 0 && isAArch32Immediate(-val)) {
a.adds(to, src, imm(-val));
} else {
ASSERT(src != TMP);
Expand All @@ -552,6 +557,78 @@ struct BeamAssembler : public BeamAssemblerCommon {
}
}

void safe_str(a32::Gp gp, arm::Mem mem) {

@ziopio ziopio Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are these safe_* wrappers really needed in BeamAssembler? in case they are, I would like to reduce code repetition as much as possible.

size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();
constexpr size_t max_disp = 4095;

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check the Displacement enum, it is better to use disp4KB, I know it is not defined for the BeamAssembler, if needed we would have to refactor this to avoid code repetitions.


ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= max_disp) {
a.str(gp, mem);
} else {
a32::Gp addr = (gp != TMP) ? TMP : VAR;
ASSERT(addr != gp);
add(addr, a32::Gp(mem.baseId()), offset);
a.str(gp, arm::Mem(addr));
}
}

void safe_ldr(a32::Gp gp, arm::Mem mem) {
size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();
constexpr size_t max_disp = 4095;

ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= max_disp) {
a.ldr(gp, mem);
} else {
a32::Gp addr = (gp != TMP) ? TMP : VAR;
ASSERT(addr != gp);
add(addr, a32::Gp(mem.baseId()), offset);
a.ldr(gp, arm::Mem(addr));
}
}

void safe_strb(a32::Gp gp, arm::Mem mem) {
size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();
constexpr size_t max_disp = 4095;

ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= max_disp) {
a.strb(gp, mem);
} else {
a32::Gp addr = (gp != TMP) ? TMP : VAR;
ASSERT(addr != gp);
add(addr, a32::Gp(mem.baseId()), offset);
a.strb(gp, arm::Mem(addr));
}
}

void safe_ldrb(a32::Gp gp, arm::Mem mem) {
size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();
constexpr size_t max_disp = 4095;

ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= max_disp) {
a.ldrb(gp, mem);
} else {
a32::Gp addr = (gp != TMP) ? TMP : VAR;
ASSERT(addr != gp);
add(addr, a32::Gp(mem.baseId()), offset);
a.ldrb(gp, arm::Mem(addr));
}
}

void ldur(a32::Gp reg, arm::Mem mem) {
// TODO
ASSERT(false);
Expand Down Expand Up @@ -1484,6 +1561,42 @@ class BeamModuleAssembler : public BeamAssembler,
}
}

void safe_ldrb(a32::Gp gp, arm::Mem mem) {
size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();

ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= disp4KB) {
preserve_cache(
[&]() {
a.ldrb(gp, mem);
},
gp);
} else {
add(TMP, a32::Gp(mem.baseId()), offset);
a.ldrb(gp, arm::Mem(TMP));
}
}

void safe_strb(a32::Gp gp, arm::Mem mem) {
size_t abs_offset = std::abs(mem.offset());
auto offset = mem.offset();

ASSERT(mem.hasBaseReg() && !mem.hasIndex());
ASSERT(gp.isGp());

if (abs_offset <= disp4KB) {
a.strb(gp, mem);
} else {
a32::Gp addr = (gp != TMP) ? TMP : VAR;
ASSERT(addr != gp);
add(addr, a32::Gp(mem.baseId()), offset);
a.strb(gp, arm::Mem(addr));
}
}

void safe_stmia(a32::Gp gp1,
a32::Gp gp2,

Expand Down
10 changes: 5 additions & 5 deletions erts/emulator/beam/jit/arm/32/beam_asm_global.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ void BeamGlobalAssembler::emit_garbage_collect() {
* twice. */
a.sub(ARG2, ARG3, HTOP);
a.lsr(ARG2, ARG2, imm(2));
a.sub(ARG2, ARG2, imm(S_RESERVED));
sub(ARG2, ARG2, S_RESERVED);

/* Save our return address in c_p->i so we can tell where we crashed if we
* did so during GC. */
a.str(a32::lr, arm::Mem(c_p, offsetof(Process, i)));
safe_str(a32::lr, arm::Mem(c_p, offsetof(Process, i)));

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You do not need safe_str here, offsetof(Process, i) is a predictable integer and we know it can be rappresented by an immediate with 12bits.

It is really important that the safe_str, safe_ldr ecc... functions are NOT spammed through the code base, I should probably comment a disclaimer on them.

They make the LDR/STR OP safe regarding the displacement handling. But they are also very dangerous if abused.

These functions use either TMP or VAR as temporary storage to compute and load the address. TMP and VAR are not an esclusive of these wrappers. This means that you could have callers of these functions storing important data on TMP and/or VAR and then use LDR and STR. In these cases, if you swap a a.ldr with a safe_ldr to fix a bug you could introduce a very HARD to detect register corruption...

So in your case I do not know which safe_* function solved your problem.
I really doubt every change here is necessary, so I would like to pin point the real fix and the check it is safe. It probably is safe since it works, but we cannot give it for granted...

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I should rename it like: dynamic_str or something like that... on arm64 they have a really safe version, I kept the name, but did not get the luxury of safety...

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah also sub and add can overwrite TMP and must be used with caution


emit_enter_runtime<Update::eStack | Update::eHeap>();

Expand All @@ -125,7 +125,7 @@ void BeamGlobalAssembler::emit_garbage_collect() {
emit_leave_runtime<Update::eStack | Update::eHeap>();
emit_leave_runtime_frame();

a.ldr(TMP, arm::Mem(c_p, offsetof(Process, state.value)));
safe_ldr(TMP, arm::Mem(c_p, offsetof(Process, state.value)));
a.tst(TMP, imm(ERTS_PSFLG_EXITING));
a.b_ne(labels[do_schedule]);

Expand All @@ -142,7 +142,7 @@ void BeamGlobalAssembler::emit_garbage_collect() {
*
* Assumes that c_p->current points into the MFA of an export entry. */
void BeamGlobalAssembler::emit_bif_export_trap() {
a.ldr(ARG1, arm::Mem(c_p, offsetof(Process, current)));
safe_ldr(ARG1, arm::Mem(c_p, offsetof(Process, current)));
sub(ARG1, ARG1, offsetof(Export, info.mfa));

emit_leave_erlang_frame();
Expand Down Expand Up @@ -187,7 +187,7 @@ void BeamGlobalAssembler::emit_export_trampoline() {
ssize_t func_offset = offsetof(Export, trampoline.bif.address);

lea(ARG2, arm::Mem(ARG1, offsetof(Export, info.mfa)));
a.ldr(ARG3, arm::Mem(c_p, offsetof(Process, i)));
safe_ldr(ARG3, arm::Mem(c_p, offsetof(Process, i)));
a.ldr(ARG4, arm::Mem(ARG1, func_offset));

/* `call_bif_shared` assumes that the return address has been pushed to
Expand Down
9 changes: 6 additions & 3 deletions erts/emulator/beam/jit/arm/32/beam_asm_module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,10 @@ arm::Mem BeamModuleAssembler::embed_constant(const ArgVal &value,
}

auto it = _constants.emplace(value,
Constant{.latestOffset = maxOffset,
Constant{.latestOffset = std::max<ssize_t>(
currOffset,
maxOffset -
STUB_CHECK_INTERVAL),

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a safer and less optimal offset that will embed constants sooner and more often. This may or may not be part of the fix. It is not bad but I would like to know.

.anchor = a.newLabel(),
.value = value});
const Constant &constant = it->second;
Expand Down Expand Up @@ -723,7 +726,7 @@ void BeamModuleAssembler::emit_veneer(const Veneer &veneer) {

a.align(AlignMode::kCode, 4);
a.bind(pointer);
a.embedLabel(veneer.target);
a.embedLabel(veneer.target, 4);

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is defensive, as the asmjit should deduce that a label is 4 bytes by the target architecture, I prefer to not write it as it should not fix anything.

}
}

Expand All @@ -742,7 +745,7 @@ void BeamModuleAssembler::emit_constant(const Constant &constant) {
} else if (value.isWord()) {
a.embedUInt32(value.as<ArgWord>().get());
} else if (value.isLabel()) {
a.embedLabel(rawLabels.at(value.as<ArgLabel>().get()));
a.embedLabel(rawLabels.at(value.as<ArgLabel>().get()), 4);

@ziopio ziopio Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same

} else {
switch (value.getType()) {
case ArgVal::BytePtr:
Expand Down
Loading