[BOLT][RISCV] Recognize mapping symbols

[jobnoorman]

Summary

The RISC-V psABI [1] defines them similarly to AArch64.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#mapping-symbol

Depends on D153260

[Amir]

The test is failing in pre-merge checks – can you please verify why?

[jobnoorman]

In D153277#4433169, @Amir wrote:

The test is failing in pre-merge checks – can you please verify why?

Ugh, this is caused by the pre-merge checks using the system's clang (-DBOLT_CLANG_EXE=/usr/bin/clang is passed to cmake). This patch depends on a change in the RISC-V MC layer (D153260) so won't work with an old clang.

I'm not quite sure how to solve this, other than updating the pre-merge checks to build clang itself. Would this be an option?

Note that this isn't the first time I had to make MC changes for BOLT RISC-V support and I assume it won't be the last so I'm a bit afraid this issue might come up again in the future.

[Amir]

In D153277#4433191, @jobnoorman wrote:

In D153277#4433169, @Amir wrote:

The test is failing in pre-merge checks – can you please verify why?

Ugh, this is caused by the pre-merge checks using the system's clang (-DBOLT_CLANG_EXE=/usr/bin/clang is passed to cmake). This patch depends on a change in the RISC-V MC layer (D153260) so won't work with an old clang.

I'm not quite sure how to solve this, other than updating the pre-merge checks to build clang itself. Would this be an option?

Note that this isn't the first time I had to make MC changes for BOLT RISC-V support and I assume it won't be the last so I'm a bit afraid this issue might come up again in the future.

Thanks for checking. Yes, it's an inconvenience that we need to keep in mind. It's a choice not to overburden the pre-merge builder with clang and lld builds that are needed for tests only.

LG in general, but let's wait until the dependencies are landed.

[Amir]

bolt/test/RISCV/mapping-syms.s

2

Suggestion: you can switch to llvm-mc to pick up MC changes.

[jobnoorman]

Use llvm-mc+lld instead of clang for the tests.

This patch relies on a recent MC-layer change but the pre-merge check bots use
the system's clang. Until the MC change is released, we cannot use clang.

[jobnoorman]

bolt/test/RISCV/mapping-syms.s

2

Thanks, done!

[jrtc27]

bolt/lib/Core/BinaryContext.cpp

1778

This won't handle the $x<isa> and $x<isa>.<uniquifier> symbols present for RISC-V but not AArch64

bolt/lib/Core/BinaryContext.cpp

case MCCFIInstruction::OpGnuArgsSize:
     break;
   default:
     OS << "Op#" << Operation;
     break;
   }
 }
 
 MarkerSymType BinaryContext::getMarkerType(const SymbolRef &Symbol) const {
-  // For aarch64, the ABI defines mapping symbols so we identify data in the
-  // code section (see IHI0056B). $x identifies a symbol starting code or the
-  // end of a data chunk inside code, $d indentifies start of data.
-  if (!isAArch64() || ELFSymbolRef(Symbol).getSize())
+  // For aarch64 and riscv, the ABI defines mapping symbols so we identify data
+  // in the code section (see IHI0056B). $x identifies a symbol starting code or
+  // the end of a data chunk inside code, $d indentifies start of data.
+  if ((!isAArch64() && !isRISCV()) || ELFSymbolRef(Symbol).getSize())
     return MarkerSymType::NONE;
 
   Expected<StringRef> NameOrError = Symbol.getName();
   Expected<object::SymbolRef::Type> TypeOrError = Symbol.getType();
 
   if (!TypeOrError || !NameOrError)
     return MarkerSymType::NONE;
 
   if (*TypeOrError != SymbolRef::ST_Unknown)
     return MarkerSymType::NONE;
 
   if (*NameOrError == "$x" || NameOrError->startswith("$x."))
     return MarkerSymType::CODE;
 
   if (*NameOrError == "$d" || NameOrError->startswith("$d."))
     return MarkerSymType::DATA;
 
   return MarkerSymType::NONE;
 }
 

bolt/lib/Rewrite/RewriteInstance.cpp

auto addExtraDataMarkerPerSymbol =
           if (IsData) {
             SortedMarkerSymbols.push_back(
                 MarkerSym{cantFail(Sym->getAddress()), MarkerSymType::DATA});
             LastAddr = Address;
           }
         }
       };
 
-  if (BC->isAArch64()) {
+  if (BC->isAArch64() || BC->isRISCV()) {
     addExtraDataMarkerPerSymbol(SortedFileSymbols, SortedMarkerSymbols);
     LastSymbol = std::stable_partition(
         SortedFileSymbols.begin(), SortedFileSymbols.end(),
         [this](const SymbolRef &Symbol) { return !BC->isMarker(Symbol); });
     if (!SortedFileSymbols.empty())
       --LastSymbol;
   }
 

bolt/test/RISCV/mapping-syms.s

+/// FIXME llvm-mc is used instead of clang because we need a recent change in
+/// the RISC-V MC layer (D153260). Once that one is released, we can switch to
+/// clang. (Note that the pre-merge check buildbots use the system's clang).
+// RUN: llvm-mc -triple riscv64 -mattr=+c -filetype obj -o %t.o %s
+// RUN: ld.lld -o %t %t.o
+// RUN: llvm-bolt --print-cfg --print-only=_start -o %t.bolt %t 2>&1 | FileCheck %s
+// RUN: llvm-objdump -d %t.bolt | FileCheck --check-prefix=CHECK-OBJDUMP %s
+
+// CHECK-NOT: BOLT-WARNING
+
+/// Check that .word is not disassembled by BOLT
+// CHECK: 00000000: nop
+// CHECK: 00000002: ret
+
+/// Check .word is still present in output
+// CHECK-OBJDUMP: <_start>:
+// CHECK-OBJDUMP-NEXT: nop
+// CHECK-OBJDUMP-NEXT: unimp
+// CHECK-OBJDUMP-NEXT: unimp
+// CHECK-OBJDUMP-NEXT: ret
+    .text
+    .globl _start
+    .p2align 1
+_start:
+    nop
+    .word 0x0
+    ret