This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fix mcount name
ClosedPublic

Authored by nathanchance on Mar 18 2021, 11:31 AM.

Details

Summary

GCC's name for this symbol is _mcount, which the Linux kernel expects in
a few different place:

$ echo 'int main(void) { return 0; }' | riscv32-linux-gcc -c -pg -o tmp.o -x c -

$ llvm-objdump -dr tmp.o | grep mcount
                        0000000c:  R_RISCV_CALL _mcount

$ echo 'int main(void) { return 0; }' | riscv64-linux-gcc -c -pg -o tmp.o -x c -

$ llvm-objdump -dr tmp.o | grep mcount
                000000000000000c:  R_RISCV_CALL _mcount

$ echo 'int main(void) { return 0; }' | clang -c -pg -o tmp.o --target=riscv32-linux-gnu -x c -

$ llvm-objdump -dr tmp.o | grep mcount
                        0000000a:  R_RISCV_CALL_PLT     mcount

$ echo 'int main(void) { return 0; }' | clang -c -pg -o tmp.o --target=riscv64-linux-gnu -x c -

$ llvm-objdump -dr tmp.o | grep mcount
                000000000000000a:  R_RISCV_CALL_PLT     mcount

Set MCountName to "_mcount" in RISCVTargetInfo then prevent it from
getting overridden in certain OSTargetInfo constructors.

Diff Detail

Unit TestsFailed

Event Timeline

nathanchance created this revision.Mar 18 2021, 11:31 AM
nathanchance requested review of this revision.Mar 18 2021, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2021, 11:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This should really be for all OSes on RISC-V, which I think means copying it N times? :/

(GCC defines MCOUNT_NAME in riscv/riscv.h, not riscv/linux.h)

nathanchance added a comment.EditedMar 18 2021, 2:38 PM

Would it be better to hoist it into RISCVTargetInfo and modify the BSD/Fuchsia switch statements to respect that in their respective TargetInfo constructors?

Such a diff does not look too bad:

diff --git a/clang/lib/Basic/Targets/OSTargets.h b/clang/lib/Basic/Targets/OSTargets.h
index 539466c4f678..c32972c1e04e 100644
--- a/clang/lib/Basic/Targets/OSTargets.h
+++ b/clang/lib/Basic/Targets/OSTargets.h
@@ -256,6 +256,8 @@ public:
     case llvm::Triple::ppcle:
     case llvm::Triple::ppc64:
     case llvm::Triple::ppc64le:
+    case llvm::Triple::riscv32:
+    case llvm::Triple::riscv64:
       this->MCountName = "_mcount";
       break;
     case llvm::Triple::arm:
@@ -488,6 +490,8 @@ public:
     case llvm::Triple::ppc:
     case llvm::Triple::ppc64:
     case llvm::Triple::ppc64le:
+    case llvm::Triple::riscv32:
+    case llvm::Triple::riscv64:
     case llvm::Triple::sparcv9:
       this->MCountName = "_mcount";
       break;
@@ -885,7 +889,14 @@ protected:
 public:
   FuchsiaTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : OSTargetInfo<Target>(Triple, Opts) {
-    this->MCountName = "__mcount";
+    switch (Triple.getArch()) {
+    case llvm::Triple::riscv32:
+    case llvm::Triple::riscv64:
+      break;
+    default:
+      this->MCountName = "__mcount";
+      break;
+    }
     this->TheCXXABI.set(TargetCXXABI::Fuchsia);
   }
 };
diff --git a/clang/lib/Basic/Targets/RISCV.h b/clang/lib/Basic/Targets/RISCV.h
index abae51e75a19..8df6e05ebcd5 100644
--- a/clang/lib/Basic/Targets/RISCV.h
+++ b/clang/lib/Basic/Targets/RISCV.h
@@ -59,6 +59,7 @@ public:
     WCharType = SignedInt;
     WIntType = UnsignedInt;
     HasRISCVVTypes = true;
+    MCountName = "_mcount";
   }
 
   bool setCPU(const std::string &Name) override {
diff --git a/clang/test/CodeGen/mcount.c b/clang/test/CodeGen/mcount.c
index 649f0b56949d..2883a6370d34 100644
--- a/clang/test/CodeGen/mcount.c
+++ b/clang/test/CodeGen/mcount.c
@@ -12,6 +12,14 @@
 // RUN: %clang_cc1 -pg -triple mipsel-unknown-gnu-linux -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
 // RUN: %clang_cc1 -pg -triple mips64-unknown-gnu-linux -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
 // RUN: %clang_cc1 -pg -triple mips64el-unknown-gnu-linux -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv32 -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv64 -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv32-linux -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv64-linux -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv32-freebsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv64-freebsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv64-fuschia -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
+// RUN: %clang_cc1 -pg -triple riscv64-openbsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-PREFIXED,NO-MCOUNT1 %s
 // RUN: %clang_cc1 -pg -triple powerpc-netbsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-DOUBLE-PREFIXED,NO-MCOUNT1 %s
 // RUN: %clang_cc1 -pg -triple powerpc64-netbsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-DOUBLE-PREFIXED,NO-MCOUNT1 %s
 // RUN: %clang_cc1 -pg -triple powerpc64le-netbsd -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK-DOUBLE-PREFIXED,NO-MCOUNT1 %s
nathanchance retitled this revision from [RISCV] Fix mcount name for Linux to [RISCV] Fix mcount name.
nathanchance edited the summary of this revision. (Show Details)
  • Hoist MCountName assignment into RISCVTargetInfo
  • Prevent MCountName from getting assigned in FreeBSD and OpenBSD targets
  • Add more tests

I did not update Fuchsia or any other operating systems because it appears that the only supported configurations from GCC are -elf, -linux, -rtems, and -freebsd so I figured it was wise to only influence those. If anyone disagrees, let me know and I can update this.

MaskRay accepted this revision.Mar 23 2021, 4:22 PM
MaskRay added inline comments.
clang/test/CodeGen/mcount.c
17

No need to test both riscv32 and riscv32-elf. They are the same - generic ELF.

This revision is now accepted and ready to land.Mar 23 2021, 4:22 PM

Drop "riscv32" and "riscv64" from list of checked triples (thanks to Fangrui for catching it with a good explanation).

This revision was landed with ongoing or failed builds.Mar 24 2021, 6:13 PM
This revision was automatically updated to reflect the committed changes.