[RuntimeDyldChecker] Support offset in decode_operand expr

[StephenFan]

Summary

In RISCV's relocations, some relocations are comprised of two relocation types. For example, R_RISCV_PCREL_HI20 and R_RISCV_PCREL_LO12_I compose a PC relative relocation. In general the compiler will set a label in the position of R_RISCV_PCREL_HI20. So, to test the R_RISCV_PCREL_LO12_I relocation, we need decode instruction at position of the label points to R_RISCV_PCREL_HI20 plus 4 (the size of a riscv non-compress instruction).

[jrtc27]

Why can't you just add a label for the instruction you want to test?

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp

588

Don't change formatting for code you didn't touch

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

22 ↗

(On Diff #356867)

This is just (named_data - test_pcrel32) & 0xfff sign-extended from 12 to 64 bits. The + 0x800 for the HI is to compensate for the sign extension for the LO, feeding it back into the LO calculation is backwards and results in circular reasoning.

[lhames]

In D105528#2861144, @jrtc27 wrote:

Why can't you just add a label for the instruction you want to test?

Actually I really like the idea of allowing offsets here. Avoiding artificial labels has two benefits: improved readability, and clearer layout control on MachO where labels implicitly start new subsections.

This is just (named_data - test_pcrel32) & 0xfff sign-extended from 12 to 64 bits. The + 0x800 for the HI is to compensate for the sign extension for the LO, feeding it back into the LO calculation is backwards and results in circular reasoning.

I don't know RISCV, so I defer to Jessica here.

Side note: Some comparisons may be easier to express with the bit-slicing operator. E.g. this example from MachO_arm64_relocations.s:

# jitlink-check: decode_operand(test_local_call, 0)[25:0] = (named_func - test_local_call)[27:2]
        .globl  test_local_call
        .p2align  2
test_local_call:
        bl   named_func

Please address Jessica's feedback, but otherwise LGTM. Thanks Stephen!

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp

240–247

You should error out for the other binop tokens with an unexpected token error.

[StephenFan]

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

22 ↗

(On Diff #356867)

This is just (named_data - test_pcrel32) & 0xfff sign-extended from 12 to 64 bits. The + 0x800 for the HI is to compensate for the sign extension for the LO, feeding it back into the LO calculation is backwards and results in circular reasoning.

@jrtc27 Thank you for reminding me! You're right! By thinking again, here is my understanding:

If the pc relative value is 0x00001fff, if don't add 0x800, then the value will be split to high 20bits(0x00001) and low 12bits(0xfff). However, the low 12bits will be considered as a signed number. so the MSB of 0xfff which originally represent 2^11 now changed to -2^11. The difference is 2^12. So we need to add 0x800 to compensate the difference. Are there any errors in my understanding?

[jrtc27]

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

22 ↗

(On Diff #356867)

2^12 is 0x1000. You're conditionally adding 0x1000 to the hi20 value based on whether the 12th bit is set which, given hi20 throws away the low 12 bits, is the same as unconditionally adding 0x800 and relying on the carry bit of 0x800 + 0x800 to give you 0x1000.

[StephenFan]

In D105528#2864094, @lhames wrote:

Side note: Some comparisons may be easier to express with the bit-slicing operator. E.g. this example from MachO_arm64_relocations.s:

Thanks, lhames! Bit-slicing looks like a great idea! I will change it.

# jitlink-check: decode_operand(test_local_call, 0)[25:0] = (named_func - test_local_call)[27:2]
        .globl  test_local_call
        .p2align  2
test_local_call:
        bl   named_func

Please address Jessica's feedback, but otherwise LGTM. Thanks Stephen!

Ok! I will!

[StephenFan]

Address @jrtc27 and @lhames 's comment

[StephenFan]

Address @jrtc27 and @lhames 's comment

[jrtc27]

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

22 ↗

(On Diff #359611)

(... & 0xfff)[11:0] is a strange thing to write, you don't need both

[StephenFan]

Address @jrtc27's comments

[StephenFan]

Fail fast when SymbolMem's size < Offset + instruction size

[StephenFan]

Fail faster when SymbolMem's size < Offset + instruction size

[lhames]

@jrtc27 Any other feedback on this?

Otherwise LGTM.

[luismarques]

LGTM.

Note: I don't know what is the current standard for the ExecutionEngine test exhaustiveness, but the error condition, the subtracted offset, and the invalid offset cases aren't exercised in the tests.

[lhames]

In D105528#2903836, @luismarques wrote:

LGTM.

Note: I don't know what is the current standard for the ExecutionEngine test exhaustiveness, but the error condition, the subtracted offset, and the invalid offset cases aren't exercised in the tests.

Testing of the ExecutionEngine libraries is basic (though reasonably effective) -- definitely not exhaustive, but improving.

Testing of the test infrastructure itself (e.g. the jitlink checker) is currently non-existent.

Improving library testing is the higher priority, but more testing of the test tools would be nice too.

[StephenFan]

Disable support of negative offset. A negative offset may get a invalid address.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp

 //===--- RuntimeDyldChecker.cpp - RuntimeDyld tester framework --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
if (!Checker.isSymbolValid(Symbol))
           EvalResult(("Cannot decode unknown symbol '" + Symbol + "'").str()),
           "");
 
     // if there is an offset number expr
     int64_t Offset = 0;
     BinOpToken BinOp;
     std::tie(BinOp, RemainingExpr) = parseBinOpToken(RemainingExpr);
     switch (BinOp) {
     case BinOpToken::Add:
     case BinOpToken::Sub: {
       EvalResult Number;
       std::tie(Number, RemainingExpr) = evalNumberExpr(RemainingExpr);
       Offset =
           BinOp == BinOpToken::Add ? Number.getValue() : -Number.getValue();
       break;
     }
     case BinOpToken::Invalid:
       break;
     default:
       return std::make_pair(unexpectedToken(RemainingExpr, RemainingExpr,
                    "expected '+ or -' for offset or ',' if no offset"), "");
     }
 
     if (!RemainingExpr.startswith(","))
else if (Expr[0] == '*')
       std::tie(SubExprResult, RemainingExpr) = evalLoadExpr(Expr);
     else if (isalpha(Expr[0]) || Expr[0] == '_')
       std::tie(SubExprResult, RemainingExpr) = evalIdentifierExpr(Expr, PCtx);
     else if (isdigit(Expr[0]))
       std::tie(SubExprResult, RemainingExpr) = evalNumberExpr(Expr);
     else
       return std::make_pair(
           unexpectedToken(Expr, Expr,
                           "expected '(', '*', identifier, or number"), "");
 
     if (SubExprResult.hasError())
       return std::make_pair(SubExprResult, RemainingExpr);
 
     // Evaluate bit-slice if present.
     if (RemainingExpr.startswith("["))
       std::tie(SubExprResult, RemainingExpr) =
           evalSliceExpr(std::make_pair(SubExprResult, RemainingExpr));
evalComplexExpr(const std::pair<EvalResult, StringRef> &LHSAndRemaining,
 
     return evalComplexExpr(std::make_pair(ThisResult, RemainingExpr), PCtx);
   }
 
   bool decodeInst(StringRef Symbol, MCInst &Inst, uint64_t &Size,
                   int64_t Offset) const {
     MCDisassembler *Dis = Checker.Disassembler;
     StringRef SymbolMem = Checker.getSymbolContent(Symbol);
+    if (SymbolMem.size() < Offset + Size)
+      return false;
     ArrayRef<uint8_t> SymbolBytes(SymbolMem.bytes_begin() + Offset,
                                   SymbolMem.size() - Offset);
 
     MCDisassembler::DecodeStatus S =
         Dis->getInstruction(Inst, Size, SymbolBytes, 0, nulls());
 
     return (S == MCDisassembler::Success);
   }