Skip to content

[ARM] Verify that disassembled instruction is correct #157360

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

Merged
merged 3 commits into from
Sep 19, 2025
Merged

[ARM] Verify that disassembled instruction is correct #157360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8ea29f
[ARM] Fix a few disassembler bugs
s-barannikov Sep 7, 2025
f52e2bd
Use UpdateThumbPredicate
s-barannikov Sep 7, 2025
66395ad
Merge branch 'main' into arm-operands-rfe
s-barannikov Sep 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
56 changes: 21 additions & 35 deletions llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,24 +1365,6 @@ static DecodeStatus DecodeRFEInstruction(MCInst &Inst, unsigned Insn,
DecodeStatus S = MCDisassembler::Success;

unsigned Rn = fieldFromInstruction(Insn, 16, 4);
unsigned mode = fieldFromInstruction(Insn, 23, 2);

switch (mode) {
case 0:
mode = ARM_AM::da;
break;
case 1:
mode = ARM_AM::ia;
break;
case 2:
mode = ARM_AM::db;
break;
case 3:
mode = ARM_AM::ib;
break;
}

Inst.addOperand(MCOperand::createImm(mode));
if (!Check(S, DecodeGPRRegisterClass(Inst, Rn, Address, Decoder)))
return MCDisassembler::Fail;

Expand Down Expand Up @@ -2779,10 +2761,6 @@ static DecodeStatus DecodeMVEModImmInstruction(MCInst &Inst, unsigned Insn,

Inst.addOperand(MCOperand::createImm(imm));

Inst.addOperand(MCOperand::createImm(ARMVCC::None));
Inst.addOperand(MCOperand::createReg(0));
Inst.addOperand(MCOperand::createImm(0));

return S;
}

Expand All @@ -2807,7 +2785,6 @@ static DecodeStatus DecodeMVEVADCInstruction(MCInst &Inst, unsigned Insn,
return MCDisassembler::Fail;
if (!fieldFromInstruction(Insn, 12, 1)) // I bit clear => need input FPSCR
Inst.addOperand(MCOperand::createReg(ARM::FPSCR_NZCV));
Inst.addOperand(MCOperand::createImm(Qd));

return S;
}
Expand Down Expand Up @@ -5956,10 +5933,6 @@ static DecodeStatus DecodeMVEVCMP(MCInst &Inst, unsigned Insn, uint64_t Address,
if (!Check(S, predicate_decoder(Inst, fc, Address, Decoder)))
return MCDisassembler::Fail;

Inst.addOperand(MCOperand::createImm(ARMVCC::None));
Inst.addOperand(MCOperand::createReg(0));
Inst.addOperand(MCOperand::createImm(0));

return S;
}

Expand Down Expand Up @@ -6103,9 +6076,23 @@ DecodeStatus ARMDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
ArrayRef<uint8_t> Bytes,
uint64_t Address,
raw_ostream &CS) const {
DecodeStatus S;
if (STI.hasFeature(ARM::ModeThumb))
return getThumbInstruction(MI, Size, Bytes, Address, CS);
return getARMInstruction(MI, Size, Bytes, Address, CS);
S = getThumbInstruction(MI, Size, Bytes, Address, CS);
else
S = getARMInstruction(MI, Size, Bytes, Address, CS);
if (S == DecodeStatus::Fail)
return S;

// Verify that the decoded instruction has the correct number of operands.
const MCInstrDesc &MCID = MCII->get(MI.getOpcode());
if (!MCID.isVariadic() && MI.getNumOperands() != MCID.getNumOperands()) {
reportFatalInternalError(MCII->getName(MI.getOpcode()) + ": expected " +
Twine(MCID.getNumOperands()) + " operands, got " +
Twine(MI.getNumOperands()) + "\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this should be a fatal error. Maybe report an error and continue? In case there are bugs not detected by tests.

}

return S;
}

DecodeStatus ARMDisassembler::getARMInstruction(MCInst &MI, uint64_t &Size,
Expand Down Expand Up @@ -6144,7 +6131,7 @@ DecodeStatus ARMDisassembler::getARMInstruction(MCInst &MI, uint64_t &Size,
const DecodeTable Tables[] = {
{DecoderTableVFP32, false}, {DecoderTableVFPV832, false},
{DecoderTableNEONData32, true}, {DecoderTableNEONLoadStore32, true},
{DecoderTableNEONDup32, true}, {DecoderTablev8NEON32, false},
{DecoderTableNEONDup32, false}, {DecoderTablev8NEON32, false},
{DecoderTablev8Crypto32, false},
};

Expand All @@ -6154,8 +6141,10 @@ DecodeStatus ARMDisassembler::getARMInstruction(MCInst &MI, uint64_t &Size,
Size = 4;
// Add a fake predicate operand, because we share these instruction
// definitions with Thumb2 where these instructions are predicable.
if (Table.DecodePred && !DecodePredicateOperand(MI, 0xE, Address, this))
return MCDisassembler::Fail;
if (Table.DecodePred && MCII->get(MI.getOpcode()).isPredicable()) {
MI.addOperand(MCOperand::createImm(ARMCC::AL));
MI.addOperand(MCOperand::createReg(ARM::NoRegister));
}
return Result;
}
}
Expand Down Expand Up @@ -6189,8 +6178,6 @@ void ARMDisassembler::AddThumb1SBit(MCInst &MI, bool InITBlock) const {
return;
}
}

MI.insert(I, MCOperand::createReg(InITBlock ? ARM::NoRegister : ARM::CPSR));
}

bool ARMDisassembler::isVectorPredicable(const MCInst &MI) const {
Expand Down Expand Up @@ -6491,7 +6478,6 @@ DecodeStatus ARMDisassembler::getThumbInstruction(MCInst &MI, uint64_t &Size,
STI);
if (Result != MCDisassembler::Fail) {
Size = 4;
Check(Result, AddThumbPredicate(MI));
Copy link
Contributor Author

@s-barannikov s-barannikov Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure about this change.
AddThumbPredicate not only adds a predicate operand, but also advances IT/VPT block states.
We don't need to add the operand here (it is added by the generated decoder), but should we advance the states? Can these instructions appear in IT block?

Also, DecodePredicateOperand that adds the operand doesn't take into account IT state. Should it?

Note that we don't call this method for instructions in VFPV8 namespace above and v8Crypto/v8NEON below.

The tests magically pass, but that doesn't give me 100% confidence in this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've figure it out. I should call UpdateThumbVFPPredicate here instead.
Not sure about other tables I mentioned, but this should preserve the existing behavior for NEONDup.

return Result;
}
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/MC/Disassembler/ARM/arm-tests.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@
# CHECK: strheq r0, [r0, -r0]
0xb0 0x00 0x00 0x01

# CHECK: rfedb #4!
# CHECK: rfedb r2!
0x14 0x0 0x32 0xf9

# CHECK: stc2l p0, c0, [r2], #-96
Expand Down