MaskRay requested changes to this revision. MaskRay added a comment. This revision now requires changes to proceed.
Please don't land the patch this state. There are several questions which should be sorted out first. a) Naming About the 'k' prefix: this is generic and does not need to be coupled with "kernel", but perhaps an argument can be made that the 'k' does not need to refer to "kernel". b) `-fsanitize=function` (D1338 <https://reviews.llvm.org/D1338>) It is a very similar feature but uses prolog data (after the function label) instead of prefix data (before the function label), with some limitation that only available on x86 and restricted to C++. I am thinking whether we can lift the C++ restriction (we'll need to move away from `CGM.GetAddrOfRTTIDescriptor`) and switch to prefix data (easier to add more ports). @pcc c) inliner Add this change to enabling inlining --- i/llvm/lib/Transforms/Utils/InlineFunction.cpp +++ w/llvm/lib/Transforms/Utils/InlineFunction.cpp @@ -1776,6 +1776,8 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI, continue; if (Tag == LLVMContext::OB_clang_arc_attachedcall) continue; + if (Tag == LLVMContext::OB_kcfi) + continue; return InlineResult::failure("unsupported operand bundle"); } d) x86-64 scheme Why is the following scheme picked? How are the double-int3 before and after movl useful? .type test,@function .globl __cfi_test # @test .type __cfi_test,@function __cfi_test: int3 int3 movl $917620134, %eax # imm = 0x36B1C5A6 int3 int3 e) `__kcfi_typeid_*` This deserves a better description in the summary. Is it useful to suppress the typeid symbol for `_Z` (if some C++ projects want to use this feature)? A mangled name has encoded the type information. IIUC the current scheme only works when two TUs have address-taken declarations of the same name but with different signatures, and done by a linker warning. ELF linkers don't error for two `SHN_ABS` `STB_GLOBAL` symbols of the same `st_value`. When the `st_value` fields differ, there will be a diagnostic. If needed, the specialized diagnostic can be added there. But I'd prefer the lld patch to be separate and be properly tested (I add myself as a reviewer for lld/ELF changes to catch possibly unintended usage, sorry!) % cat a.s .globl foo .set foo, 1 % cat b.s .globl foo .set foo, 2 % ld.lld a.o b.o ld.lld: error: duplicate symbol: foo >>> defined in a.o >>> defined in b.o ================ Comment at: clang/docs/ControlFlowIntegrity.rst:314 + +KCFI, enabled by ``-fsanitize=kcfi``, is an alternative indirect call +control-flow integrity scheme designed for low-level system software, such ---------------- Don't repeat the heading. ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2269 + for (const char &C : Name) { + if (llvm::isAlnum(C) || C == '_' || C == '.') + continue; ---------------- ``` if (!(llvm::isAlnum(C) || C == '_' || C == '.')) return false; ``` ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6688 + StringRef Suffix, + bool OnlyExternal /*=true*/) { if (auto *FnType = T->getAs<FunctionProtoType>()) ---------------- T ================ Comment at: clang/lib/CodeGen/CodeGenModule.h:1421 + /// Set type hash as prefix data to the given function + void SetKCFITypePrefix(const FunctionDecl *FD, llvm::Function *F); + ---------------- Unlikely `Emit*`, there are many `set*` functions which use the correct case, so there is no excuse using the wrong case. ================ Comment at: clang/lib/Driver/SanitizerArgs.cpp:707 + // With -fpatchable-function-entry=N,M, where M > 0, + // llvm::AsmPrinter::emitFunctionHeader injects nops before before the + // KCFI type identifier, which is currently unsupported. ---------------- ================ Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1339 + + if (Section) { + OutStreamer->PushSection(); ---------------- early return ================ Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1345 + OutStreamer->emitLabel(Loc); + OutStreamer->emitAbsoluteSymbolDiff(Symbol, Loc, 4); + ---------------- Use getCodePointerSIze(). A 4-byte PC-relative relocation is insufficient if text and data are more than 31-bit away. ================ Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1130 + const MCSectionELF &ElfSec = static_cast<const MCSectionELF &>(TextSec); + unsigned Flags = ELF::SHF_LINK_ORDER | ELF::SHF_ALLOC | ELF::SHF_WRITE; + StringRef GroupName; ---------------- Remove SHF_WRITE. The section doesn't need a dynamic relocation. ================ Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:135 + if (MAI->hasDotTypeDotSizeDirective()) { + MCSymbol *EndSym = OutContext.createTempSymbol("__cfi_func_end"); + OutStreamer->emitLabel(EndSym); ---------------- For a temporary symbol (STB_LOCAL), no need to add `__` prefix. ================ Comment at: llvm/test/CodeGen/AArch64/kcfi-bti.ll:4 +; RUN: llc -mtriple=aarch64-- -stop-after=finalize-isel < %s | FileCheck %s --check-prefixes=MIR,FINAL + +; ASM: .word 12345678 ---------------- Add a test with `"patchable-function-entry"="2"` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits