Skip to content

Conversation

@romainthomas
Copy link
Contributor

@romainthomas romainthomas commented Dec 7, 2024

While trying to evaluate the branch of this MIPS (EL) non-branch instruction: ldxc1 $f2, $4($7) the function fails on the assert: assert(isImm() && ...).

This commit ensures that the operand is an immediate before accessing the value.

  • Triple: mipsel-unknown-linux-gnu-elf with +mips32r2
  • Instruction: ldxc1 $f2, $4($7)
  • Raw instruction: 0x81, 0x0, 0xe4, 0x4c

While trying to evaluate the branch of this MIPS (EL) non-branch
instruction: `ldxc1 $f2, $4($7)` the function fails on the assert:
`assert(isImm() && ...)`.

This commit ensures that the operand is an immediate before accessing the
value.

- Triple: `mipsel-unknown-linux-gnu-elf` with '+mips32r2'
- Instruction: `ldxc1 $f2, $4($7)`
- Raw instruction: `0x81, 0x0, 0xe4, 0x4c`
@github-actions
Copy link

github-actions bot commented Dec 7, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff a9eb8f0e3dbaf16b6bd83eecb960b6ea8ecaa8c3 d0b9d784e2e1257eeab6c8a52f4845a10ae151f6 --extensions cpp -- llvm/lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp b/llvm/lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
index 4b38ce70c3..eaf4de9d37 100644
--- a/llvm/lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
+++ b/llvm/lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
@@ -143,7 +143,7 @@ public:
     switch (Info->get(Inst.getOpcode()).operands()[NumOps - 1].OperandType) {
     case MCOI::OPERAND_UNKNOWN:
     case MCOI::OPERAND_IMMEDIATE: {
-      const MCOperand& Op = Inst.getOperand(NumOps - 1);
+      const MCOperand &Op = Inst.getOperand(NumOps - 1);
       if (!Op.isImm())
         return false;
       // j, jal, jalx, jals

@brad0
Copy link
Contributor

brad0 commented Dec 7, 2024

cc @yingopq @wzssyqa

@nikic
Copy link
Contributor

nikic commented Dec 7, 2024

Test?

@romainthomas
Copy link
Contributor Author

From what I understand, most of the logic of Mips disassembler testing is done in llvm/test/MC/Disassembler/Mips/. There is also some disassembler unittests in llvm/unittests/MC but not for the Mips Target (only AMDGPU, X86, SystemZ).

I don't think we could exercise this code path with a lit test1 using llvm-mc so a unittest would be more appropriated. But since there is no unittest for Mips it would require bootstrapping them.

Who should own this bootstrapping? Me in this PR? Me in a different PR? The Mips code owners?

Footnotes

  1. I can be completely wrong on this point

@s-barannikov
Copy link
Contributor

I don't think we could exercise this code path with a lit test using llvm-mc

I think you can do that using llvm-objdump --symbolize-operands.

@wzssyqa
Copy link
Contributor

wzssyqa commented Dec 9, 2024

Can you give out a test case for it?

@romainthomas
Copy link
Contributor Author

Feel free to re-open this PR if you think it is relevant and when there is a clear status about Mips testing in LLVM (c.f. #119056 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants