[PATCH] D155756: -fsanitize={function,kcfi}: Switch to xxh3_64bits

samitolvanen added a comment. Changing the hash function breaks compatibility with the Rust KCFI implementation . I would personally like

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

This revision was automatically updated to reflect the committed changes. Closed by commit rG62fa708ceb02: [RISCV] Implement KCFI operand bundle lowering (authored by samitolvanen). Herald added a subscriber: wangpc. Changed prior to commit:

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

samitolvanen added a comment. > Needs some thoughts to merge the above paragraphs with the existing one Great points. I updated the commit message and tried to capture most of these, PTAL. Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:332 + continue; +while

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

samitolvanen updated this revision to Diff 531476. samitolvanen marked 9 inline comments as done. samitolvanen added a comment. Addressed feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148385/new/ https://reviews.llvm.org/D148385 Files:

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

samitolvanen added a comment. In D148385#4397995 , @MaskRay wrote: > `KCFI_CHECK` lowering has some complexity to allocate a temporary register. > This needs to follow the calling convention which can be modified by many > compiler options and function

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

samitolvanen accepted this revision. samitolvanen added a comment. This revision is now accepted and ready to land. Looks good to me, but maybe worth waiting for someone more familiar with compiler-rt to take a look as well. Comment at:

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

samitolvanen updated this revision to Diff 520795. samitolvanen added a comment. Dropped the arch-specific KCFI machine function pass. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148385/new/ https://reviews.llvm.org/D148385 Files:

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

samitolvanen planned changes to this revision. samitolvanen added a comment. Uploaded D149915 to remove the arch-specific KCFI passes. I'll update this patch once we clean that up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

samitolvanen updated this revision to Diff 514708. samitolvanen added a comment. Addressed most of the feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148385/new/ https://reviews.llvm.org/D148385 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

samitolvanen marked 9 inline comments as done. samitolvanen added a comment. In D148385#4275617 , @jrtc27 wrote: > I can't help but feel the core of this should be target-independent, with TLI > or similar hooks to actually do the target-specific bits?

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

samitolvanen created this revision. Herald added subscribers: jobnoorman, luke, VincentWu, ormris, vkmr, frasercrmck, evandro, luismarques, apazos, sameer.abuasal, pengfei, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27,

[PATCH] D145999: [RISCV] Reserve X18 by default for Android

samitolvanen accepted this revision. samitolvanen added a comment. LGTM. Folding D145979 into this one might also make sense. Comment at: clang/test/Driver/riscv-fixed-x-register.c:343 + +// Check that x18 is reserved on Android by default

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added a comment. In D134831#4137408 , @inglorion wrote: > Is this intended to warn on code that casts a function taking a pointer to > some non-void type to a function that takes a void*? Yes, this is intended to warn if the function types

[PATCH] D139395: Add CFI integer types normalization

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG71c7313f42d2: Add CFI integer types normalization (authored by rcvalle, committed by samitolvanen). Changed prior to commit:

[PATCH] D139395: Add CFI integer types normalization

samitolvanen accepted this revision. samitolvanen added a comment. Thanks for fixing the MSan issue, Ramon. There's still a clang-format error that trips the Debian build above, but it's trivial so I can fix it when relanding the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen updated this revision to Diff 494756. samitolvanen added a comment. This revision is now accepted and ready to land. Herald added subscribers: ormris, steven_wu. Don't drop type hashes from VisibleToRegularObj symbols. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D139395: Add CFI integer types normalization

This revision was automatically updated to reflect the committed changes. Closed by commit rGb1e9ab7438a0: Add CFI integer types normalization (authored by rcvalle, committed by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139395: Add CFI integer types normalization

samitolvanen added a reviewer: samitolvanen. samitolvanen accepted this revision. samitolvanen added a comment. This revision is now accepted and ready to land. Thanks, LGTM. @pcc, does this version look fine to you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added a comment. In D138337#4080628 , @nickdesaulniers wrote: >> If you're referring to ibt-seal, I assume a similar optimization could be >> added for BTI too. > > Yes, please. Does it make sense to add to this patch, or a follow up on

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added a comment. In D138337#4080615 , @nickdesaulniers wrote: > The test is for x86, does this also work for aarch64 with BTI? IBT/BTI doesn't affect this specific optimization, it's only about KCFI. If you're referring to ibt-seal, I

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added a comment. In D138337#4078843 , @nickdesaulniers wrote: > Is https://reviews.llvm.org/D142163 a blocker for this? Yes, and this patch needs to be updated to take `VisibleToRegularObj` into account. I'll update this patch once we

[PATCH] D139395: Add CFI integer types normalization

samitolvanen added a comment. In D139395#4067391 , @pcc wrote: > I discussed this out of band with Ramon and we agreed that the new option > should be marked as experimental because the rustc implementation is not yet > finalized. I think that the

[PATCH] D139395: Add CFI integer types normalization

samitolvanen added a comment. Thanks for the patch, Ramon. This looks like a reasonable approach to me, and just for reference, here appears to be the corresponding rustc change: https://github.com/rust-lang/rust/pull/105452/commits/9087c336103d0fa0b465acf8dbc1e4651250fb05 @pcc did you have

[PATCH] D141172: [ModuleUtils][KCFI] Set patchable-function-prefix for synthesized functions

This revision was automatically updated to reflect the committed changes. Closed by commit rGfd5e26270660: [ModuleUtils][KCFI] Set patchable-function-prefix for synthesized functions (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141172: [ModuleUtils][KCFI] Set patchable-function-prefix for synthesized functions

samitolvanen updated this revision to Diff 487536. samitolvanen marked an inline comment as done. samitolvanen added a comment. Addressed feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141172/new/ https://reviews.llvm.org/D141172 Files:

[PATCH] D141172: [ModuleUtils][KCFI] Set patchable-function-prefix for synthesized functions

samitolvanen added inline comments. Comment at: llvm/test/Instrumentation/AddressSanitizer/kcfi-offset.ll:1 +;; Test that we set patchable-function-prefix for asan.module_ctor when kcfi-offset is defined. + MaskRay wrote: >

[PATCH] D141172: [ModuleUtils][KCFI] Set patchable-function-prefix for synthesized functions

samitolvanen created this revision. Herald added a subscriber: hiraditya. Herald added a project: All. samitolvanen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. When -fpatchable-function-entry is used to emit prefix

[PATCH] D140363: Remove incorrectly implemented -mibt-seal

samitolvanen accepted this revision. samitolvanen added a comment. This revision is now accepted and ready to land. I agree, it's probably best to temporarily revert this until Joao has time to address the issues you mentioned. The Linux kernel doesn't use `-mibt-seal` yet, so dropping the

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

samitolvanen accepted this revision. samitolvanen added a comment. This revision is now accepted and ready to land. In D140035#3996066 , @joaomoreira wrote: > Regarding not being able to reproduce this in kernel -- never mind... I was > misled by setup

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

samitolvanen added a comment. In D140035#3995703 , @joaomoreira wrote: > Regarding https://github.com/ClangBuiltLinux/linux/issues/1737: > > Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the > kernel did boot fine on my

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added a comment. In D138337#3972493 , @pcc wrote: > Isn't that just a bug in the optimization? It shouldn't be applying this to > symbols where `VisibleToRegularObj` is set. The current patch simply replicates ibt-seal behavior, but sure,

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added a comment. In D138337#3972009 , @pcc wrote: > Can't this be implicit if LTO is being used? I would prefer to keep this behind a flag (similarly to `-mibt-seal`), so we can better control when and where the optimization enabled. For

[PATCH] D135411: Add generic KCFI operand bundle lowering

This revision was automatically updated to reflect the committed changes. Closed by commit rGcacd3e73d7f8: Add generic KCFI operand bundle lowering (authored by samitolvanen). Changed prior to commit: https://reviews.llvm.org/D135411?vs=476615=477315#toc Repository: rG LLVM Github Monorepo

[PATCH] D138458: [Clang][Driver] Add KCFI to SupportsCoverage

This revision was automatically updated to reflect the committed changes. Closed by commit rG5a3d6ce956c4: [Clang][Driver] Add KCFI to SupportsCoverage (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138458/new/

[PATCH] D138458: [Clang][Driver] Add KCFI to SupportsCoverage

samitolvanen updated this revision to Diff 477228. samitolvanen marked 2 inline comments as done. samitolvanen added a comment. Switched to `[[#]]`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138458/new/ https://reviews.llvm.org/D138458 Files:

[PATCH] D138458: [Clang][Driver] Add KCFI to SupportsCoverage

samitolvanen added inline comments. Comment at: clang/test/CodeGen/sanitize-coverage.c:94 + // CHECK-NOT: call void @__sanitizer_cov_trace_const_cmp + // KCFI-NOT: call void %{{.}}() [ "kcfi"(i32 {{.*}}) ] + f(); MaskRay wrote: > `%c()` > > `{{.}}` matches

[PATCH] D138458: [Clang][Driver] Add KCFI to SupportsCoverage

samitolvanen updated this revision to Diff 477017. samitolvanen marked an inline comment as done. samitolvanen added a comment. Addressed feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138458/new/ https://reviews.llvm.org/D138458 Files:

[PATCH] D138458: [Clang][Driver] Add KCFI to SupportsCoverage

samitolvanen created this revision. Herald added a project: All. samitolvanen requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. Allow -fsanitize=kcfi to be enabled with -fsanitize=trace-{pc,cmp}. Link:

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen added reviewers: nickdesaulniers, pcc, MaskRay, kees, joaomoreira. samitolvanen added a comment. Herald added a subscriber: StephenFan. ClangBuiltLinux issue: https://github.com/ClangBuiltLinux/linux/issues/1737 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

samitolvanen created this revision. Herald added subscribers: pengfei, hiraditya, inglorion. Herald added a project: All. samitolvanen requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, MaskRay. Herald added projects: clang, LLVM. With -fsanitize=kcfi, Clang

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 476615. samitolvanen added a comment. Fixed clang-format warnings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/ https://reviews.llvm.org/D135411 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 476491. samitolvanen added a comment. Dropped the unneeded constructor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/ https://reviews.llvm.org/D135411 Files:

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 476260. samitolvanen added a comment. Dropped the CodeGen dependency. Note that Clang already only adds KCFIPass for targets that don't support back-end lowering, so the check in the pass itself was a bit redundant. The existing CodeGen/kcfi.c test

[PATCH] D135411: Add generic KCFI operand bundle lowering

This revision was automatically updated to reflect the committed changes. Closed by commit rGeb2a57ebc7aa: Add generic KCFI operand bundle lowering (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

This revision was automatically updated to reflect the committed changes. Closed by commit rG41ce74e6e983: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 473348. samitolvanen added a comment. Addressed more feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/ https://reviews.llvm.org/D135411 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 473340. samitolvanen marked 4 inline comments as done. samitolvanen added a comment. Added `KCFIDiagnosticInfo` and switched from `report_fatal_error` to `LLVMContext::diagnose`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:61 + if (F.hasFnAttribute("patchable-function-prefix")) +report_fatal_error("-fpatchable-function-entry=N,M, where M>0 is not " + "compatible with

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 473321. samitolvanen marked 6 inline comments as done. samitolvanen added a comment. Addressed Fangrui's feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen updated this revision to Diff 473306. samitolvanen added a comment. Added release notes, grouped together with `-Wcast-function-type-strict`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136790/new/ https://reviews.llvm.org/D136790

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8217 + err_typecheck_convert_incompatible_function_pointer.Text>, + InGroup>, DefaultIgnore; def ext_typecheck_convert_discards_qualifiers : ExtWarn<

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8217 + err_typecheck_convert_incompatible_function_pointer.Text>, + InGroup>, DefaultIgnore; def ext_typecheck_convert_discards_qualifiers : ExtWarn<

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8217 + err_typecheck_convert_incompatible_function_pointer.Text>, + InGroup>, DefaultIgnore; def ext_typecheck_convert_discards_qualifiers : ExtWarn<

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen added a comment. In D136790#3897184 , @aaron.ballman wrote: > please be sure to add a release note so users know there's a new warning. I'll add a release note. Thanks for the reminder! Comment at:

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen added reviewers: aaron.ballman, nickdesaulniers, pcc, joaomoreira, kees, nathanchance. samitolvanen added a comment. Similarly to D134831 , `-Wincompatible-function-pointer-types` also seems to miss int <-> enum mismatches in function pointer

[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

samitolvanen created this revision. Herald added a project: All. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang supports indirect call Control-Flow Integrity (CFI) sanitizers (e.g. -fsanitize=cfi-icall), which enforce

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1aad641c7930: [Clang][Sema] Add -Wcast-function-type-strict (authored by samitolvanen). Changed prior to commit:

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added a comment. The patch to disable this warning in Linux without W=1 is now in all supported stable kernels (https://lwn.net/Articles/912498/), so I'll commit this later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 466635. samitolvanen added a comment. Switched to `instructions(F)`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/ https://reviews.llvm.org/D135411 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:48-49 + SmallVector KCFICalls; + for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I) { +if (auto *CI = dyn_cast(&*I)) + if

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen updated this revision to Diff 466612. samitolvanen marked 2 inline comments as done. samitolvanen added a comment. Addressed feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135411/new/ https://reviews.llvm.org/D135411 Files:

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen added a comment. In D135411#3841693 , @kees wrote: > What's the best way to test this in the kernel? I assume add > `ARCH_SUPPORTS_CFI_CLANG` to an arch, and see what blows up? :) Yes, I think that's the best way. > Have you tried this on

[PATCH] D135411: Add generic KCFI operand bundle lowering

samitolvanen created this revision. Herald added subscribers: Enna1, ormris, pengfei, hiraditya, kristof.beyls. Herald added a project: All. samitolvanen requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, MaskRay. Herald added projects: clang, LLVM. The KCFI

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added a comment. In D134831#3838373 , @nickdesaulniers wrote: > Please consider waiting until the warning flag has started propagating to > branches of stable, at least to whatever branch -Werror is first enabled in. Yes, I'm planning to

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added a comment. In D134831#3827861 , @nathanchance wrote: > If you want to move on it quicker, feel free to draft a commit message and > slap my Suggested-by on it.

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added a comment. In D134831#3827730 , @nickdesaulniers wrote: > Please get the patch disabling this warning for the kernel in flight before > landing this. Agreed. @nathanchance do you want to send the patch above to LKML? Repository:

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen updated this revision to Diff 464326. samitolvanen marked 3 inline comments as done. samitolvanen added a comment. Addressed feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134831/new/ https://reviews.llvm.org/D134831 Files:

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8686 +def warn_cast_function_type_strict : Warning, + InGroup, DefaultIgnore; def err_cast_pointer_to_non_pointer_int : Error< aaron.ballman wrote: > nathanchance

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen updated this revision to Diff 464067. samitolvanen marked 8 inline comments as done. samitolvanen added a comment. Moved `CastFunctionTypeStrict` to a subgroup of `CastFunctionType`, addressed other feedback, and updated tests. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen updated this revision to Diff 463702. samitolvanen added a comment. Added a release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134831/new/ https://reviews.llvm.org/D134831 Files: clang/docs/ReleaseNotes.rst

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8686 +def warn_cast_function_type_strict : Warning, + InGroup, DefaultIgnore; def err_cast_pointer_to_non_pointer_int : Error< nickdesaulniers wrote: > I don't

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen added reviewers: pcc, nickdesaulniers, kees. samitolvanen added a comment. Any thoughts about adding a stricter version of -Wcast-function-type to make it easier to catch potential CFI issues? I also considered also gating this behind `-fsanitize=cfi-icall/kcfi`, but having a

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

samitolvanen created this revision. Herald added a project: All. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang supports indirect call Control-Flow Integrity (CFI) sanitizers (e.g. -fsanitize=cfi-icall), which enforce

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:265 + PointerSumTypeMember, + PointerSumTypeMember>, + PointerSumTypeMember> samitolvanen wrote: > This fails on 32-bit architectures as `PointerEmbeddedInt`

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:265 + PointerSumTypeMember, + PointerSumTypeMember>, + PointerSumTypeMember> This fails on 32-bit architectures as `PointerEmbeddedInt` doesn't allow storing

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/docs/ControlFlowIntegrity.rst:319 +cross-DSO function address equality. These properties make KCFI easier to +adopt in low-level software. KCFI is limited to indirect call checking only, +and isn't compatible with

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3733753 , @MaskRay wrote: > For `STB_WEAK SHN_ABS` `__kcfi_typeid_*`, there is no duplicate definition > error. Is this behavior intentional? Yes, this is by design. Multiple translation units must be able to

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } joaomoreira wrote: > Can we use another constant blinding scheme, such as a Value++ or anything > else? This way, we would

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3684022 , @joaomoreira wrote: > Some minor suggestions/not so relevant: > > - Change the name of CFIType to CFIHash, as it is probably more descriptive. My thinking here was that Type is more generic than a

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3673297 , @MaskRay wrote: > It seems that we can miss the branch to have more time mature the feature and > have it for 16.0. Sure, that sounds reasonable. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added a comment. Based on the recent LKML discussions about X86 retbleed mitigations (https://lore.kernel.org/lkml/20220716230344.239749...@linutronix.de/), we're going to need some changes to the code generated for X86. Repository:

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:332 + + Register AddrReg = MI.getOperand(0).getReg(); + MaskRay wrote: > Add const. Delete blank line after the declaration. Deleted the blank line, but this can't be

[PATCH] D119296: KCFI sanitizer

samitolvanen marked an inline comment as done. samitolvanen added inline comments. Comment at: clang/lib/Driver/SanitizerArgs.cpp:63 SanitizerKind::Unreachable | SanitizerKind::Return; -static const SanitizerMask AlwaysRecoverable = -SanitizerKind::KernelAddress |

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In 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

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3623059 , @nickdesaulniers wrote: > I see you modified the mir parser & printer; consider adding a .mir test. I added a .mir test for parsing the cfi-type. The machine instructions generated for the various

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + samitolvanen wrote: > pcc wrote: > > samitolvanen wrote: > > > joaomoreira wrote: > > > > samitolvanen wrote: > > > > > joaomoreira

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6694 - if (isExternallyVisible(T->getLinkage())) { + if (isExternallyVisible(T->getLinkage()) || !OnlyExternal) { std::string OutName;

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 439137. samitolvanen added a comment. Fixed the debug output in InstCombine to use metadata as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files:

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 439099. samitolvanen added a comment. Switched from prefix data + attribute to a metadata node based on previous discussion. This seems to be a cleaner solution overall. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 438010. samitolvanen added a comment. Rebased after ToT member function name changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files:

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2257 + + F->setPrefixData(CreateKCFITypeId(FD->getType())); + F->addFnAttr("kcfi-target"); ychen wrote: > FYI: using prefix data may not work for the C++ coroutine. >

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 435685. samitolvanen added a comment. - Per Fangrui's request, added comments explaining the X86 preamble format, and a note about it to the patch summary. - Switched back to `.weak` for the `__kcfi_typeid_` symbols to fix compatibility with LTO.

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3562409 , @MaskRay wrote: > 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

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) samitolvanen wrote: > MaskRay

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 434974. samitolvanen marked 9 inline comments as done. samitolvanen added a comment. - Addressed Fangrui's feedback. - Renamed `KCFI_*` DAG nodes and pseudo instructions to `CFI_*` for now based on Joao's feedback. Still looking for more feedback on the

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3562409 , @MaskRay wrote: > 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 >

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + joaomoreira wrote: > samitolvanen wrote: > > joaomoreira wrote: > > > I did not revise the entire patch yet. With this said, IMHO,

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + joaomoreira wrote: > I did not revise the entire patch yet. With this said, IMHO, this looks like > an overcomplication of a

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) MaskRay wrote: > samitolvanen

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) MaskRay wrote: > This change

  1   2   >