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

[Evgenii Stepanov via llvm-commits]

On Wed, Oct 26, 2016 at 5:42 PM, Eric Christopher <echristo at gmail.com> wrote:
>
>
> On Wed, Oct 26, 2016 at 5:12 PM Peter Collingbourne <peter at pcc.me.uk> wrote:
>>
>> 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`).
>>
>
> This set of APIs is pretty bad. See my recent patches to try to unify it.
> I'd appreciate trying to use TLOF->getNameWithPrefix if you can rather than
> calling into the mangler specifically.

How do I get a TLOF in an LTO pass?

>
>>
>>
>> ================
>> 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
>>
>>
>>
>