[AVR] Fix incorrect decoding of RJMP and RCALL

[benshi001]

Summary

This patch fixes the inaccurate decoding of the offset operand of
the RCALL & RJMP instructions.

[benshi001]

This patch fixes https://github.com/llvm/llvm-project/issues/47093 .

[aykevl]

Shouldn't this also affect llvm/test/MC/AVR/inst-rjmp.s?
In any case, I think it's better to (also) have tests in llvm/test/MC/AVR for this.

[benshi001]

In D140815#4021212, @aykevl wrote:

Shouldn't this also affect llvm/test/MC/AVR/inst-rjmp.s?
In any case, I think it's better to (also) have tests in llvm/test/MC/AVR for this.

Actually this bug does not affect llvm/test/MC/AVR/inst-rjmp.s, so I can not add any test in llvm/test/MC/AVR.

Since llvm-objdump decodes all address offsets of rjmp/rcall (before linking) to 0, so you can see those zeros in llvm/test/MC/AVR/inst-rjmp.s, which are not affected.

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

[benshi001]

llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp

300

This is modified by clang-format .

[aykevl]

In D140815#4021441, @benshi001 wrote:

Since llvm-objdump decodes all address offsets of rjmp/rcall (before linking) to 0, so you can see those zeros in llvm/test/MC/AVR/inst-rjmp.s, which are not affected.

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

Hmm, yeah that's a problem.
@MaskRay do you have an idea how to properly test this? I don't like moving these tests into lld (it's a separate subproject), but the alternative seems to be to use a binary file to test against which isn't great either.

[benshi001]

In D140815#4032392, @aykevl wrote:

In D140815#4021441, @benshi001 wrote:

Since llvm-objdump decodes all address offsets of rjmp/rcall (before linking) to 0, so you can see those zeros in llvm/test/MC/AVR/inst-rjmp.s, which are not affected.

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

Hmm, yeah that's a problem.
@MaskRay do you have an idea how to properly test this? I don't like moving these tests into lld (it's a separate subproject), but the alternative seems to be to use a binary file to test against which isn't great either.

I do see many .o files in clang/test/, but none in llvm/test/MC/ . Do we need to create such a precedent ?

[MaskRay]

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

It's not appropriate to add a lld test to test an assembly issue (https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer). Such tests live in llvm/test/MC/

If you want to disassembly an arbitrary instruction, you may use directives like .word: after disassembly, an instruction isn't differentiable with a .word xxx.
If you want to craft an ET_EXEC or ET_DYN file, use yaml2obj. See llvm/test/tools/llvm-{objcopy,readobj} for some examples.

You can use llvm-readelf -x ... to dump the content of a section.

[benshi001]

In D140815#4032938, @MaskRay wrote:

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

It's not appropriate to add a lld test to test an assembly issue (https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer). Such tests live in llvm/test/MC/

If you want to disassembly an arbitrary instruction, you may use directives like .word: after disassembly, an instruction isn't differentiable with a .word xxx.
If you want to craft an ET_EXEC or ET_DYN file, use yaml2obj. See llvm/test/tools/llvm-{objcopy,readobj} for some examples.

You can use llvm-readelf -x ... to dump the content of a section.

Thanks. Using the .short directive seems the most clear way !

[benshi001]

llvm/test/MC/AVR/inst-rjmp.s

49

The positive offset is tested here and the negative offset is tested in inst-rcall.s.

[aykevl]

That's indeed a nice solution, didn't think of that!

llvm/test/MC/AVR/inst-rcall.s

13

It may be useful for the reader to add what this means: .short 0xdfea ; rcall .-44

llvm/lib/Target/AVR/AVRInstrFormats.td

 //===----------------------------------------------------------------------===//
 class FBRk<bit f, dag outs, dag ins, string asmstr, list<dag> pattern>
     : AVRInst16<outs, ins, asmstr, pattern> {
   bits<12> k;
 
   let Inst{15 - 13} = 0b110;
   let Inst{12} = f;
   let Inst{11 - 0} = k;
+
+  let DecoderMethod = "decodeFBRk";
 }
 
 //===----------------------------------------------------------------------===//
 // 32 bits branching instructions: <|1001|010k|kkkk|fffk|kkkk|kkkk|kkkk|kkkk|>
 // f = secondary opcode = 3 bits
 // k = constant address = 22 bits
 //===----------------------------------------------------------------------===//
 class F32BRk<bits<3> f, dag outs, dag ins, string asmstr, list<dag> pattern>

llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp

 
 static DecodeStatus decodeFMUL2RdRr(MCInst &Inst, unsigned Insn,
                                     uint64_t Address,
                                     const MCDisassembler *Decoder);
 
 static DecodeStatus decodeMemri(MCInst &Inst, unsigned Insn, uint64_t Address,
                                 const MCDisassembler *Decoder);
 
+static DecodeStatus decodeFBRk(MCInst &Inst, unsigned Insn, uint64_t Address,
+                               const MCDisassembler *Decoder);
+
 static DecodeStatus decodeLoadStore(MCInst &Inst, unsigned Insn,
                                     uint64_t Address,
                                     const MCDisassembler *Decoder);
 
 #include "AVRGenDisassemblerTables.inc"
 
 static DecodeStatus decodeFIOARr(MCInst &Inst, unsigned Insn, uint64_t Address,
                                  const MCDisassembler *Decoder) {
static DecodeStatus decodeMemri(MCInst &Inst, unsigned Insn, uint64_t Address,
   Inst.addOperand(
       MCOperand::createReg((Insn & 0x40) ? AVR::R29R28 : AVR::R31R30));
   // Append the immediate offset operand.
   Inst.addOperand(MCOperand::createImm(Insn & 0x3f));
 
   return MCDisassembler::Success;
 }
 
+static DecodeStatus decodeFBRk(MCInst &Inst, unsigned Insn, uint64_t Address,
+                               const MCDisassembler *Decoder) {
+  // Decode the opcode.
+  switch (Insn & 0xf000) {
+  case 0xc000:
+    Inst.setOpcode(AVR::RJMPk);
+    break;
+  case 0xd000:
+    Inst.setOpcode(AVR::RCALLk);
+    break;
+  default: // Unknown relative branch instruction.
+    return MCDisassembler::Fail;
+  }
+  // Decode the relative offset.
+  int16_t Offset = ((int16_t)((Insn & 0xfff) << 4)) >> 3;
+  Inst.addOperand(MCOperand::createImm(Offset));
+  return MCDisassembler::Success;
+}
+
 static DecodeStatus decodeLoadStore(MCInst &Inst, unsigned Insn,
                                     uint64_t Address,
                                     const MCDisassembler *Decoder) {
   // Get the register will be loaded or stored.
   unsigned RegVal = GPRDecoderTable[(Insn >> 4) & 0x1f];
 
   // Decode LDD/STD with offset less than 8.
   if ((Insn & 0xf000) == 0x8000) {
     unsigned RegBase = (Insn & 0x8) ? AVR::R29R28 : AVR::R31R30;
     unsigned Offset = Insn & 7; // We need not consider offset > 7.
-    if ((Insn & 0x200) == 0) { // Decode LDD.
+    if ((Insn & 0x200) == 0) {  // Decode LDD.
       Inst.setOpcode(AVR::LDDRdPtrQ);
       Inst.addOperand(MCOperand::createReg(RegVal));
       Inst.addOperand(MCOperand::createReg(RegBase));
       Inst.addOperand(MCOperand::createImm(Offset));
     } else { // Decode STD.
       Inst.setOpcode(AVR::STDPtrQRr);
       Inst.addOperand(MCOperand::createReg(RegBase));
       Inst.addOperand(MCOperand::createImm(Offset));

llvm/test/MC/AVR/inst-rcall.s

 ; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN:     | llvm-objdump -d - | FileCheck --check-prefix=INST %s
 
 
 foo:
 
   rcall  .+0
   rcall  .-8
   rcall  .+12
   rcall  .+46
+  .short  0xdfea
 
 ; CHECK: rcall  .Ltmp0+0             ; encoding: [A,0b1101AAAA]
 ; CHECK:                             ;   fixup A - offset: 0, value: .Ltmp0+0, kind: fixup_13_pcrel
 ; CHECK: rcall  .Ltmp1-8             ; encoding: [A,0b1101AAAA]
 ; CHECK:                             ;   fixup A - offset: 0, value: .Ltmp1-8, kind: fixup_13_pcrel
 ; CHECK: rcall  .Ltmp2+12            ; encoding: [A,0b1101AAAA]
 ; CHECK:                             ;   fixup A - offset: 0, value: .Ltmp2+12, kind: fixup_13_pcrel
 ; CHECK: rcall  .Ltmp3+46            ; encoding: [A,0b1101AAAA]
 ; CHECK:                             ;   fixup A - offset: 0, value: .Ltmp3+46, kind: fixup_13_pcrel
 
+; INST: rcall   .+0
+; INST: rcall   .+0
+; INST: rcall   .+0
+; INST: rcall   .+0
+; INST: rcall   .-44

llvm/test/MC/AVR/inst-rjmp.s

foo:
   rjmp  .+8
   rjmp  end
   rjmp  .+0
 end:
   rjmp .-4
   rjmp .-6
 x:
   rjmp x
+  .short 0xc00f
 
 ; CHECK: rjmp    .Ltmp0+2                ; encoding: [A,0b1100AAAA]
 ; CHECK:                                 ;   fixup A - offset: 0, value: .Ltmp0+2, kind: fixup_13_pcrel
 ; CHECK: rjmp    .Ltmp1-2                ; encoding: [A,0b1100AAAA]
 ; CHECK:                                 ;   fixup A - offset: 0, value: .Ltmp1-2, kind: fixup_13_pcrel
 ; CHECK: rjmp    foo                     ; encoding: [A,0b1100AAAA]
 ; CHECK:                                 ;   fixup A - offset: 0, value: foo, kind: fixup_13_pcrel
 ; CHECK: rjmp    .Ltmp2+8                ; encoding: [A,0b1100AAAA]

 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
 ; INST: rjmp  .+0
+; INST: rjmp  .+30