samitolvanen added a comment.

In D119296#3625796 <https://reviews.llvm.org/D119296#3625796>, @MaskRay wrote:

>> It uses LLVM prefix data to store a type identifier for each function and 
>> injects verification code before indirect calls.
>
> Is "prefix data" stale now?

Yes, I forgot to update the commit message here. Fixed.

> There are 30+ comments not marked as "done". Having so many is distracting to 
> reviewers as it's unclear whether this is in a ready-for-review state.

My bad, I wasn't familiar with the convention here. I've just been marking my 
replies as done.

> I personally definitely don't consider this in a ready-for-land state.

I left some questions about your comments below. PTAL.



================
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 |
----------------
MaskRay wrote:
> 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`
This variable is only used to indicate whether `-fno-sanitize-recover` command 
line parameter can be used with the sanitizer. It makes no sense to allow this 
with KCFI as we always emit a recoverable instruction sequence, hence it's 
included here.

Also, ud2 absolutely is recoverable in the kernel, and Linux specifically uses 
ud2 to trigger warnings in assembly code.


================
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:
> 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.
> If -O2 has different behavior, having the test in clang/test/CodeGen is 
> likely a layering violation.

Good point, the `-O2` test isn't actually relevant anymore as we no longer rely 
on it. I'll drop it.

> Optimization tests should be placed in llvm/test/, in the relevant pass.

Yes, the `InstCombine` test below covers this already.



================
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)
----------------
samitolvanen wrote:
> MaskRay wrote:
> > 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.
> > If -O2 has different behavior, having the test in clang/test/CodeGen is 
> > likely a layering violation.
> 
> Good point, the `-O2` test isn't actually relevant anymore as we no longer 
> rely on it. I'll drop it.
> 
> > Optimization tests should be placed in llvm/test/, in the relevant pass.
> 
> Yes, the `InstCombine` test below covers this already.
> 
> If may be useful to have a `-x c++` test.

Would you mind elaborating on this? The main difference with `-x c++` is the 
name mangling, I'm not sure if that adds much value in this case.

> Add a not-address-taken external linkage function.

Added.


================
Comment at: clang/test/Driver/fsanitize.c:652
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=kcfi -fsanitize=cfi -flto 
-fvisibility=hidden %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-KCFI-NOCFI
+// CHECK-KCFI-NOCFI: error: invalid argument '-fsanitize=kcfi' not allowed 
with '-fsanitize=cfi'
----------------
MaskRay wrote:
> Use modern spelling `--target=`. `-target ` is deprecated
> Use modern spelling `--target=`. `-target ` is deprecated

It doesn't look like `--target` works here:

`clang-15: error: unsupported option '--target'; did you mean '-target'?`

Also the rest of the file uses `-target`, so perhaps these can all be updated 
at once when the flag is deprecated?


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:125
+
+  // Emit int3 padding to allow runtime patching of the preamble.
+  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
----------------
MaskRay wrote:
> A previous comment isn't done. This doesn't explain that the double-int3 
> before and after the identifier is an objtool requirement (or similar).
> A previous comment isn't done. This doesn't explain that the double-int3 
> before and after the identifier is an objtool requirement (or similar).

These are not objtool requirements, and I believe the comments do explain why 
the int3 padding is added. The first two are to reserve some space for runtime 
patching this code, and the latter two is to avoid call target gadgets in the 
checks. I'm obviously happy to improve the comments, is there something 
specific you'd like me to add?


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

Reply via email to