[lldb][RISCV] Use uint64_t for emulating ADDI

[itf]

In RISC-V, the same instruction is used for both signed and unsigned additions.

Signed integer overflow is undefined behavior in C++, but signed integer overflow is defined behavior when using ADDI in RISC-V.

As we are emulating the RISC-V behavior we should be using uint.

This fix the failure with ubsan introduced by #159842: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp:807:40: runtime error: signed integer overflow: -9223372036854775808 + -16 cannot be represented in type 'int64_t' (aka 'long')

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-lldb

Author: Ivan Tadeu Ferreira Antunes Filho (itf)

Changes

In RISC-V, the ADDI instruction simply performs a binary addition on the registers. The same instruction is used for both signed and unsigned additions.

As we are emulating the riscv behavior we should be using uint.

This fix the failure with ubsan: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp:807:40: runtime error: signed integer overflow: -9223372036854775808 + -16 cannot be represented in type 'int64_t' (aka 'long')

---

Full diff: https://github.com/llvm/llvm-project/pull/160550.diff

1 Files Affected:

• (modified) lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp (+1-1)

diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 20661290ca4c6..5c1b7d4943b3f 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -804,7 +804,7 @@ class Executor {
     return transformOptional(
                inst.rs1.ReadI64(m_emu),
                [&](int64_t rs1) {
-                 int64_t result = rs1 + int64_t(SignExt(inst.imm));
+                 uint64_t result = rs1 + uint64_t(SignExt(inst.imm));
                  // Check if this is a stack pointer adjustment.
                  if (inst.rd.rd == RISCV_GPR_SP &&
                      inst.rs1.rs == RISCV_GPR_SP) {

[dmpots]

LGTM

[satyajanga]

In RISC-V, the same instruction is used for both signed and unsigned additions.

As we are emulating the riscv behavior we should be using uint.

Can you please elaborate the second sentence on why we should be using uint?

[itf]

In RISC-V, the same instruction is used for both signed and unsigned additions.
As we are emulating the riscv behavior we should be using uint.

Can you please elaborate the second sentence on why we should be using uint?

Signed integer overflow is undefined behavior in C++, but signed integer overflow is defined behavior when using ADDI in riscv: it just does the same as summing unsigned integers.

So in order to have the same behavior in the case of an overflow we should be be using uint, as far as I can tell; but I'm not 100% sure of this.

[clayborg]

Is there a test case we can use to emulate this instruction correctly? I see lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp as the best place to do this. Testing the edge case that was failing to show it failing before and succeeding after the fix.

[itf]

Is there a test case we can use to emulate this instruction correctly? I see lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp as the best place to do this. Testing the edge case that was failing to show it failing before and succeeding after the fix.

I don't think we can write a test case for "confirm this is not undefined behavior", or at least I don't know a way to test for undefined behavior directly from a test

[DavidSpickett]

I think Greg means add a test that uses the values that previously caused UB. If that test when run under UBSAN does not fail, then we know UB is not being invoked anymore.

[rupprecht]

I think Greg means add a test that uses the values that previously caused UB. If that test when run under UBSAN does not fail, then we know UB is not being invoked anymore.

These two tests fail w/ ubsan enabled:

• lldb/unittests/Instruction/RISCV/TestRiscvInstEmulation.cpp, specifically TestRiscvInstEmulation.TestSimpleRiscvFunction
• lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py