nickdesaulniers added a comment.
Please add a test where calls to `@llvm.kcfi.check` produce `KCFI_CHECK` during
instruction selection. (You can use `-stop-before=finalize-isel` to dump the IR
prior to isel; this can stay a .ll test, I think, rather than .mir).
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2273-2275
+ std::string Name = "__kcfi_typeid_" + F.getName().str();
+ if (!allowKCFIIdentifier(Name))
+ continue;
----------------
You could probably avoid re-checking `"__kcfi_typeid_"` repeatedly?
================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:157
+ {"dfsan_abilist.txt", SanitizerKind::DataFlow},
+ {"cfi_ignorelist.txt", SanitizerKind::CFI | SanitizerKind::KCFI},
+ {"ubsan_ignorelist.txt",
----------------
none of the other kernel variants of `SanitizerKind` support these ignore
lists. Do we plan to use them for the Linux kernel?
================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:700
+ D.Diag(diag::err_drv_argument_not_allowed_with) << "-fsanitize=kcfi"
+ << "-fsanitize=cfi*";
+
----------------
Can we use `lastArgumentForMask` here to be slightly more precise?
================
Comment at: clang/test/CodeGen/kcfi.c:31
+
+// CHECK-DAG: define internal i32 @f3() #[[#ATTR]] prefix i32 [[#HASH]]
+static int f3(void) { return 1; }
----------------
TIL about CHECK-DAG!
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive
================
Comment at: clang/test/CodeGen/kcfi.c:43
+ call(f3) +
+ call(f4);
+}
----------------
How about a direct call to `f5`? That should have no hash, right? Perhaps test
that `test` has no hash as well?
================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:359
+ MCSection *getKCFISection(const MCSection &TextSec,
+ const std::string &Name) const;
+
----------------
can we take a StringRef instead (and include the right header)?
================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:774
+ // Emit an .alt_entry directive for the actual function symbol.
+ if (MAI->hasSubsectionsViaSymbols())
OutStreamer->emitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
----------------
consider saving this to a `bool` scoped to the `if (F.hasPrefixData()) {` block.
================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1111
+
+ const MCSectionELF &ElfSec = static_cast<const MCSectionELF &>(TextSec);
+ unsigned Flags = ELF::SHF_LINK_ORDER;
----------------
or just `cast`.
https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates
================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1120
+ return Ctx->getELFSection(Name, ELF::SHT_PROGBITS,
+ Flags | ELF::SHF_ALLOC | ELF::SHF_WRITE, 0,
+ GroupName, true, ElfSec.getUniqueID(),
----------------
Why don't you initialize `Flags` to these values, rather than set them here?
================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1121
+ Flags | ELF::SHF_ALLOC | ELF::SHF_WRITE, 0,
+ GroupName, true, ElfSec.getUniqueID(),
+ cast<MCSymbolELF>(TextSec.getBeginSymbol()));
----------------
`/*IsComdat=*/true`
================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:355
+ OutStreamer->emitLabel(Trap);
+ EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::BRK).addImm(0x801));
+ emitKCFITrapEntry(MF, Trap);
----------------
Is this constant something the kernel will recognize?
================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:3
+
+target triple = "aarch64--"
+
----------------
put this in the RUN line?
================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:10
+; CHECK-NEXT: .word 12345678
+define void @f1(i8* %x) #0 prefix i32 12345678 {
+; CHECK-LABEL: f1:
----------------
why does `@f1` have the hash? We're not calling it indirectly.
================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:25
+; CHECK-NEXT: ret
+ call void @llvm.kcfi.check(i8* %x, i32 12345678)
+ ret void
----------------
perhaps add a `call` to `%x` itself so we can see how the generated control
flow looks?
================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:3
+
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
put this in the RUN line?
================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:11
+; CHECK-NEXT: .zero 2
+define void @f1(i8* %x) #0 prefix i32 12345678 {
+; CHECK-LABEL: f1:
----------------
why does `@f1` have the hash? We're not calling it indirectly.
================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:23
+; CHECK-NEXT: retq
+ call void @llvm.kcfi.check(i8* %x, i32 12345678)
+ ret void
----------------
perhaps add a `call` to `%x` itself so we can see how the generated control
flow looks?
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
