[PATCH] D119296: KCFI sanitizer

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1434
+
+  /// Emit KCFI type identifier constants and remove unused identifiers
+  void FinalizeKCFITypes();
----------------
End with a period.

This function can be lower-case as well. There is no strong precedent to keep it upper-case.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:63
     SanitizerKind::Unreachable | SanitizerKind::Return;
-static const SanitizerMask AlwaysRecoverable =
-    SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
+static const SanitizerMask AlwaysRecoverable = SanitizerKind::KernelAddress |
+                                               SanitizerKind::KernelHWAddress |
----------------
This is incorrect.

If a violation is found, ud2 is executed. ud2 is not followed by normal control flow so I don't think recovery from the error is supported.

This seems like `Unrecoverable`


================
Comment at: clang/test/CodeGen/kcfi.c:2
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O0 %s
+// RUN: %clang_cc1 -O2 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O2 %s
+#if !__has_feature(kcfi)
----------------
MaskRay wrote:
> If `-O2` has different behavior, having the test in clang/test/CodeGen is likely a layering violation.
> Optimization tests should be placed in llvm/test/, in the relevant pass. Folks working on llvm optimizations generally don't want to test every frontend.
If may be useful to have a `-x c++` test.

Add a not-address-taken external linkage function.


================
Comment at: clang/test/CodeGen/kcfi.c:7
+
+// COM: Must emit __kcfi_typeid symbols for address-taken function declarations
+// CHECK: module asm ".weak __kcfi_typeid_f4"
----------------
Optional: `COM:` is fine. But to make non-RUN non-CHECK lines stand out (from editor highlighting), a simpler `/// ` suffices as well.


================
Comment at: llvm/docs/LangRef.rst:7222
+functions that can be called indirectly. The type data is emitted before the
+function entry. Indirect calls with the :ref:`kcfi operand bundle<ob_kcfi>`
+will emit a check that compares the type identifier to the metadata.
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119296/new/

https://reviews.llvm.org/D119296