samitolvanen added a comment.
In D119296#3457740 <https://reviews.llvm.org/D119296#3457740>, @nickdesaulniers
wrote:
> 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).
I added a check for this in the existing tests. Although if isel didn't produce
a `KCFI_CHECK` here for some reason, I'm fairly sure the tests would have blown
up either way.
================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:157
+ {"dfsan_abilist.txt", SanitizerKind::DataFlow},
+ {"cfi_ignorelist.txt", SanitizerKind::CFI | SanitizerKind::KCFI},
+ {"ubsan_ignorelist.txt",
----------------
nickdesaulniers wrote:
> none of the other kernel variants of `SanitizerKind` support these ignore
> lists. Do we plan to use them for the Linux kernel?
Not at the moment. I'll drop the ignore list changes.
================
Comment at: clang/test/CodeGen/kcfi.c:43
+ call(f3) +
+ call(f4);
+}
----------------
nickdesaulniers wrote:
> How about a direct call to `f5`? That should have no hash, right? Perhaps
> test that `test` has no hash as well?
`test` is a global function, which needs to have a hash as it could be
indirectly called from another module.
Note that we don't need hashes in non-address-taken local functions, but the
initial patch emitted them anyway. I changed the code to drop those hashes and
added a directly called `static f5(void)` to the test.
================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:355
+ OutStreamer->emitLabel(Trap);
+ EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::BRK).addImm(0x801));
+ emitKCFITrapEntry(MF, Trap);
----------------
nickdesaulniers wrote:
> Is this constant something the kernel will recognize?
Yes, the kernel will recognize the constant and use it to distinguish KCFI
traps.
================
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:
----------------
nickdesaulniers wrote:
> why does `@f1` have the hash? We're not calling it indirectly.
Because I want to include a test for the prefix data being emitted correctly.
Also, a global function needs a hash even if we don't call it indirectly in
this module. I'll rename these tests to `kcfi.ll` to not make it sound like
this only tests `llvm.kcfi.check` output.
================
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:
----------------
nickdesaulniers wrote:
> why does `@f1` have the hash? We're not calling it indirectly.
See above.
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
