[PATCH] D25927: [cfi] Implement cfi-icall using inline assembly.

Peter Collingbourne via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 26 17:12:12 PDT 2016


pcc added inline comments.


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:669
+  if (!Dest->hasLocalLinkage())
+    OS << ".globl " << Dest->getName() << "\n";
+  OS << ".type " << Dest->getName() << ", function\n";
----------------
eugenis wrote:
> eugenis wrote:
> > pcc wrote:
> > > I wonder whether we want to escape these names? This appears to be the escaping we'd need to use:
> > > http://llvm-cs.pcc.me.uk/lib/MC/MCSymbol.cpp#53
> > > 
> > > Maybe it's not necessary though since we pretty much expect all names we see to satisfy `MCAsmInfo::isValidUnquotedName`.
> > Looks like we should escape the names.
> > It's easy to call a function "\n" with __asm__ in the declaration, and that breaks the jumptable code.
> > 
> It appears that neither llvm-mc nor gnu as support quoted identifier properly.
> Gnu as allows  a quoted string in, for example, .globl directive. Llvm-mc does too, but it forgets to unescape it, so, say, "\n" turns into a 2-character string when saves as textual asm and read back.
> 
> Neither support quoted strings in expressions we need to declare aliases, like
>   "\n" = .jumptable + 4.
> 
> I think I'll just make it a fatal error to apply cfi-icall to any name that requires escaping.
> 
Makes sense, I suppose. Though I think you'll also need to apply IR mangling (`Mangler::getNameWithPrefix`).


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:693
 
+static bool isValidUnquotedName(StringRef Name) {
+  if (Name.empty())
----------------
Maybe `isValidAsmUnquotedName` or something?


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:709
 /// for the functions, build the bit sets and lower the llvm.type.test calls.
-void LowerTypeTestsModule::buildBitSetsFromFunctionsX86(
+void LowerTypeTestsModule::buildBitSetsFromFunctionsNative(
     ArrayRef<Metadata *> TypeIds, ArrayRef<Function *> Functions) {
----------------
This function should have a FIXME to replace the inline asm with some better representation.


================
Comment at: test/Transforms/LowerTypeTests/function.ll:1-4
+; RUN: opt -S -lowertypetests -mtriple=i686-unknown-linux-gnu < %s | FileCheck --check-prefix=X86 %s
+; RUN: opt -S -lowertypetests -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck --check-prefix=X86 %s
+; RUN: opt -S -lowertypetests -mtriple=arm-unknown-linux-gnu < %s | FileCheck --check-prefix=ARM %s
+; RUN: opt -S -lowertypetests -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck --check-prefix=ARM %s
----------------
Maybe use `check-prefixes` here to reduce duplication below?


Repository:
  rL LLVM

https://reviews.llvm.org/D25927





More information about the llvm-commits mailing list