Page MenuHomePhabricator

[LowerTypeTests] Emit cfi_jt aliases regardless of function export
Needs ReviewPublic

Authored by nickdesaulniers on Aug 11 2021, 3:34 PM.

Details

Summary

A constant complaint we get is that the typeid symbols in the CFI
jump tables causes confusing stack traces in applications. Emit the more
readable cfi_jt aliases regardless of function export (LTO vs Thin LTO).

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Aug 11 2021, 3:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 11 2021, 3:34 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Thanks, Nick. I tested a kernel build with both ThinLTO and regular LTO + CFI, and can confirm that with this patch vmlinux has the previously missing .cfi_jt symbols again.

tejohnson added inline comments.Aug 16 2021, 8:45 PM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1555

Where and how does the new alias symbol get used in the LTO case? I only see it used when importing in the backend in the ThinLTO case.

pcc added inline comments.Sep 9 2021, 1:57 PM
compiler-rt/test/cfi/cfi_jt_aliases.c
1 ↗(On Diff #365863)

This isn't an execution test so I don't think it should be together with the other execution tests. Also I think it will fail on non-ELF platforms because you're using llvm-readelf etc. It seems better to make this a test of the LTO pipeline (see e.g. llvm/test/ThinLTO/X86/cache-icall.ll).

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1555

I think the idea is that these symbols only exist for debugging purposes.

I think this could be cleaned up a bit though to ensure that the symbols aren't being exported unnecessarily. Can we set the linkage to internal for non-exported symbols and add to llvm.used to ensure that the symbols aren't dropped?

causes confusing stack traces in applications

Peter also pointed out that jump table symbols don't actually show up in stack traces, but in the kernel's case, everywhere else where we need to print out a function name, including ftrace.

kees added a subscriber: kees.Wed, Oct 13, 1:11 PM

Ping; Nick, do you have a moment to rework this patch? AIUI, this is still important to getting CFI more sane for Android.

nickdesaulniers marked an inline comment as done.
  • rebase, convert compiler-rt test to ThinLTO test. TODO: llvm.used and linkage
nickdesaulniers planned changes to this revision.Wed, Oct 13, 2:43 PM
  • emit cfi_jt symbols as InternalLinkage and add them to llvm.used
nickdesaulniers marked 2 inline comments as done.Wed, Oct 13, 3:24 PM
nickdesaulniers added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1555

I've added this; I've fixed up one of the regressions it causes, llvm/test/Transforms/LowerTypeTests/export-icall.ll, but I'm not sure about the other two:

  1. compiler-rt/test/cfi/icall/external-call.c
  2. compiler-rt/test/cfi/icall/weak.c
nickdesaulniers marked an inline comment as done.Wed, Oct 13, 4:47 PM
nickdesaulniers added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1555

Specifically, it seems that the use of GlobalValue::InternalLinkage rather than GlobalValue::ExternalLinkage is what causes those two tests to regress.

nickdesaulniers planned changes to this revision.Wed, Oct 13, 5:42 PM
  • change linkage to internal only if !exported, as @pcc said
  • use M rather than *F->getParent(), store Functions[I]->isExported() in local var

I tested the latest version with a CFI kernel build, and everything still looks good. I see the same number of .cfi_jt symbols with ThinLTO and full LTO.