This is an archive of the discontinued LLVM Phabricator instance.

[ELF][ARM] Simplify some llvm-objdump tests with both ARM/Thumb states
ClosedPublic

Authored by MaskRay on Aug 21 2019, 8:02 AM.

Details

Summary

llvm-objdump can switch between ARM/Thumb states after D60927.

In a few lld tests, we run both

  • llvm-objdump -d -triple=thumbv7a-none-linux-gnueabi %t
  • llvm-objdump -d -triple=armv7a-none-linux-gnueabi %t

to test ARM/Thumb parts of the same file. In many cases we can just
run one command. There is a problem that prevents us from cleaning
more tests (e.g. test/ELF/arm-thumb-interwork-thunk.s):

In llvm-objdump, while we have ARM/Thumb (primary and secondary)
MCDisassembler and MCSubtargetInfo, we have just one MCInstrAnalysis
which is used to resolve the targets of calls in both ARM/Thumb parts.

// ThumbMCInstrAnalysis evaluating ARM parts or ARMMCInstrAnalysis evaluating Thumb parts
// will have incorrect offsets.
// An example of llvm-objdump -d -triple=thumbv7a on ARM part:
1304: 3d ff ff fa  blx     #-780                 # no <...>
1308: 06 00 00 ea  b       #24 <arm_caller+0x24> # wrong target due to wrong offset

Event Timeline

MaskRay created this revision.Aug 21 2019, 8:02 AM
peter.smith accepted this revision.Aug 21 2019, 8:08 AM

LGTM, thanks for making the update.

This revision is now accepted and ready to land.Aug 21 2019, 8:08 AM
MaskRay added a comment.EditedAug 21 2019, 8:21 AM

How can we fix the MCInstrAnalysis problem? Shall we (a) have two MCInstrAnalysis (how), (b) pass an extra parameter to evaluateBranch, or (c) make it stateful?

edit: I think I favor (c) because it will allow pseudo branch instructions. For example, RISC-V can take two instructions auipc/jalr to jump to a target.

In tools/llvm-objdump/llvm-objdump.cpp

if (MIA && (MIA->isCall(Inst) || MIA->isUnconditionalBranch(Inst) ||
            MIA->isConditionalBranch(Inst))) {
  uint64_t Target;
  if (MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target)) {  // We know the current ARM/Thumb state but MIA is decided by -triple=

For MCSubtargetInfo and MCDisassembler, we can obtain the secondary one with:

if (isArmElf(Obj) && !STI->checkFeatures("+mclass")) {
  if (STI->checkFeatures("+thumb-mode"))
    Features.AddFeature("-thumb-mode");
  else
    Features.AddFeature("+thumb-mode");
  SecondarySTI.reset(TheTarget->createMCSubtargetInfo(TripleName, MCPU,
                                                      Features.getString()));
  SecondaryDisAsm.reset(TheTarget->createMCDisassembler(*SecondarySTI, Ctx));
}

std::unique_ptr<const MCInstrAnalysis> MIA(          /////////// fixed
    TheTarget->createMCInstrAnalysis(MII.get()));
This revision was automatically updated to reflect the committed changes.

(a) have two MCInstrAnalysis (how), (b) pass an extra parameter to evaluateBranch, or (c) make it stateful?

We could also check whether the opcode is an ARM opcode or a Thumb opcode... they don't overlap at all for the relevant opcodes. But passing in the MCSubtargetInfo to evaluateBranch seems like a really easy change.

Not sure how making it "stateful" helps here.