[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 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 call

[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 t

[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 | Saniti

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 418933. samitolvanen added a comment. Herald added subscribers: llvm-commits, pengfei, MaskRay, hiraditya. Herald added projects: LLVM, All. - Split the `kcfi_unchecked` attribute into a separate patch. - Based on feedback from kernel developers, switched

[PATCH] D122673: Add kcfi_unchecked attribute

samitolvanen created this revision. samitolvanen added reviewers: aaron.ballman, pcc, nickdesaulniers, ardb, kees, joaomoreira. Herald added subscribers: jdoerfert, martong. Herald added a project: All. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a s

[PATCH] D122673: Add kcfi_unchecked attribute

samitolvanen added a comment. Note that this was split from D119296 . In the previous discussion, @joaomoreira pointed out that this is very similar to `nocf_check` and proposed reusing that attribute. In an offline discussion, @pcc was concerned that an attr

[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 Hash,

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2883804 , @nickdesaulniers wrote: > In D104058#2878083 , @samitolvanen > wrote: > >> In D104058#2877631 , >> @nickdesaulniers w

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8e3b5cb39eef: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen reopened this revision. samitolvanen added a comment. This revision is now accepted and ready to land. The tests that now specify a target triple also need `; REQUIRES: x86-registered-target` or they will obviously fail. I'll upload another revision. Repository: rG LLVM Github M

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 359463. samitolvanen added a comment. Added `REQUIRES: x86-registered-target` to tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Transfo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG700d07f8ce6f: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2891551 , @morehouse wrote: > This patch broke the sanitizer-windows bot: > https://lab.llvm.org/buildbot/#/builders/127/builds/14257 > > Failed Tests (4): > cfi-devirt-lld-thinlto-x86_64 :: anon-namespace.

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 360267. samitolvanen added a comment. Folded in D106392 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Tra

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias); ---

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias); ---

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 360615. samitolvanen added a comment. This revision is now accepted and ready to land. As we only care about fixing inline assembly references, mangled names are not that important in the first place. This version skips any functions that have unusual cha

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3437604 , @pcc wrote: >> Note that if additional data has been injected between the KCFI >> type identifier and the start of the function, e.g. by using >> -fpatchable-function-entry, the offset in bytes must be sp

[PATCH] D122673: Add kcfi_unchecked attribute

samitolvanen added a comment. In D122673#3443498 , @aaron.ballman wrote: > I tend to be very wary of modifying the type system with attributes -- > questions always arise of what the type system effects are of the attribute. > e.g., does it impact over

[PATCH] D122673: Add kcfi_unchecked attribute

samitolvanen abandoned this revision. samitolvanen added a comment. Alright, thanks for the feedback everyone! I'll abandon this patch and look into adding a built-in function instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122673/new/ http

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 422687. samitolvanen added a comment. Dropped `-fsanitize-kcfi-offset` and added an error when used with `-fpatchable-function-entry=N,M`, where M > 0. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 423748. samitolvanen marked 12 inline comments as done. samitolvanen added a comment. Addressed Nick's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296

[PATCH] D119296: KCFI sanitizer

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

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 424030. samitolvanen marked 4 inline comments as done. samitolvanen added a comment. Addressed another round of comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2248 + F->setPrefixData(CreateKCFITypeId(FD->getType())); + F->addFnAttr("kcfi"); +} nickdesaulniers wrote: > While string based attributes are easier to work with in LLVM, I wo

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2255-2260 + for (const char &C : Name) { +if (llvm::isAlnum(C) || C == '_' || C == '.') + continue; +return false; + } + return true; nickdesaulniers wrote: > sami

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2268-2272 +if (!AddressTaken && F.hasLocalLinkage()) { + F.setPrefixData(nullptr); + F.removeFnAttr("kcfi"); + continue; +} nickdesaulniers wrote: > Can we

[PATCH] D124211: Add __builtin_call_kcfi_unchecked

samitolvanen created this revision. samitolvanen added reviewers: aaron.ballman, pcc, nickdesaulniers, ardb, kees, joaomoreira. Herald added a project: All. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. With `-fsanitize=kcfi

[PATCH] D119296: KCFI sanitizer

samitolvanen created this revision. Herald added subscribers: dexonsmith, dang, jdoerfert, cryptoad. Herald added a reviewer: aaron.ballman. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The KCFI sanitizer, enabled with -fsan

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3168 + -1); + llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash); + llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont"); pcc wrote: > We considered a

[PATCH] D119296: KCFI sanitizer

samitolvanen marked an inline comment as not done. samitolvanen added inline comments. Comment at: clang/include/clang/Basic/Attr.td:696 +def KCFIUnchecked : Attr { + let Spellings = [Clang<"kcfi_unchecked">]; + let Subjects = SubjectList<[Var, TypedefName]>; j

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added a comment. Thanks for the pointers, Aaron. I'll rework the attribute code and also address the issues Nick pointed out in the next revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3466027 , @joaomoreira wrote: > I played a little bit with kcfi and here are some thoughts: > > - under -Os I saw functions being inlined, regardless of the source code > calling them indirectly. In these scenari

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3466217 , @joaomoreira wrote: > Oh, one other tiny detail I forgot to mention. I noticed that the tag is > pushing the functions 6 bytes forward, regardless of any prepending padding > nops that were added to en

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 425642. samitolvanen marked an inline comment as done. samitolvanen added a comment. Herald added subscribers: ormris, wenlei, steven_wu, mgorny. - Added an LLVM pass to remove unneeded `llvm.kcfi.check` calls. - Switched from zeros to 0xcc for x86 type i

[PATCH] D124211: Add __builtin_call_kcfi_unchecked

samitolvanen added a comment. In D124211#3480652 , @nickdesaulniers wrote: > Can you link to the lore thread on discussions around the builtin? There's no LKML discussion about the built-in, but there were earlier proposals of using an attribute to acc

[PATCH] D124211: Add __builtin_call_kcfi_unchecked

samitolvanen added a comment. In D124211#3480773 , @nickdesaulniers wrote: > Perhaps it's time to start a discussion on LKML then, before landing this? I don't intend to land these patches until there's a consensus on the correct approach from both pro

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 425942. samitolvanen marked an inline comment as done. samitolvanen added a comment. - Moved the KCFI pass to InstCombine Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3480793 , @pcc wrote: >> Yes, I suspect this might be an issue with Clang's existing CFI schemes too, >> and would probably require an additional pass to drop checks before calls >> that were either inlined or op

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3481573 , @joaomoreira wrote: > At first I was on the boat of KCFI being implemented in the IR level because > being architecture agnostic is certainly a great plus. The original plan was to just lower the intri

[PATCH] D124211: Add __builtin_call_kcfi_unchecked

samitolvanen updated this revision to Diff 426116. samitolvanen marked 2 inline comments as done. samitolvanen added a comment. - Renamed the builtin. - Addressed Nick's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124211/new/ https://re

[PATCH] D124211: Add __builtin_kcfi_call_unchecked

samitolvanen added a comment. LKML RFC: https://lore.kernel.org/lkml/20220429203644.2868448-1-samitolva...@google.com/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124211/new/ https://reviews.llvm.org/D124211

[PATCH] D124211: Add __builtin_kcfi_call_unchecked

samitolvanen abandoned this revision. samitolvanen added a comment. OK, I confirmed that we won't need this after all. I'll abandon this patch and revisit if it becomes necessary in future. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124211/new/

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 427764. samitolvanen added a comment. Based on LKML feedback: - Fixed a bug in `Twine` usage. - Changed AArch64 to encode register information into the ESR and dropped `.kcfi_traps` generation for the arch. - Changed X86 to generate valid instructions f

[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 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 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 L

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 428208. samitolvanen added a comment. - Handle FP, LR, and XZR register arguments in the AArch64 `llvm.kcfi.check` lowering. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 428520. samitolvanen added a comment. Added a test for the Clang -O2 pipeline dropping unneeded checks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files:

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen created this revision. Herald added subscribers: ormris, steven_wu, hiraditya, inglorion. samitolvanen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Create an internal alias with the original name for static

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added reviewers: nickdesaulniers, pcc, tejohnson, kees, eugenis. samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:79 + GlobalValue::InternalLinkage, Name, F, &ExportM); + appendToCompil

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 352825. samitolvanen added a comment. Moved the test to llvm/test/Transforms/ThinLTOBitcodeWriter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was automatically updated to reflect the committed changes. Closed by commit rG4474958d3a97: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 353797. samitolvanen added a comment. Fix a use-of-uninitialized-value error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Transforms/IPO/Th

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:58-80 +std::string OldName = Name.str(); std::string NewName = (Name + ModuleId).str(); if (const auto *C = ExportGV.getComdat()) if (C->getName() == Name)

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe3d24b45b8f8: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. Thanks for the revert, I'll take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 ___ cfe-commits mailing list cfe-commits@

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:39 + // Promotion aliases are used only in inline assembly. It's safe to + // simply skip unusual names. Matches MCAsmInfo::isAcceptableChar(). + for (const char &C : Name) { -

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 363166. samitolvanen added a comment. Also skip functions with names incompatible with XCOFF. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/T

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45 + } + return true; +} nickdesaulniers wrote: > samitolvanen wrote: > > nickdesaulniers wrote: > > > Can llvm::any_of or llvm::none_of be used here? > > > llvm/AD

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ce1c4da7726: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D105265: [X86] AVX512FP16 instructions enabling 3/6

samitolvanen added a comment. FYI, this breaks the Linux kernel when built with ThinLTO: https://github.com/ClangBuiltLinux/linux/issues/1440 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105265/new/ https://reviews.llvm.org/D105265 _

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 358364. samitolvanen added a comment. Moved the alias creation to module level inline assembly to avoid issues with LowerTypeTestsModule, based on pcc's suggestion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2877631 , @nickdesaulniers wrote: > Change LGTM, but I don't understand why the following tests are modified: > > - llvm/test/ThinLTO/X86/devirt2.ll This is needed to fix two `missing symbol resolution` errors th

[PATCH] D112761: cfi-icall: Add -fsanitize-cfi-promotion-aliases

samitolvanen created this revision. Herald added subscribers: ormris, jeroen.dobbelaere, dexonsmith, dang, steven_wu, hiraditya. samitolvanen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Inline assembly refererences to

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 383514. samitolvanen added a comment. Addressed comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files: clang/docs/LanguageExtensions.rst clang/incl

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 384897. samitolvanen added a comment. Keep a `void *` return type for the nocfi variant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files: clang/docs/Lang

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen removed subscribers: rsmith, rjmccall. samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3112492 , @rjmccall wrote: > You could also make this Just Work for things like C++ member functions > rather than

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 388237. samitolvanen added a comment. Renamed to `__builtin_function_start`, allowed only FunctionDecls as a parameter, added support for C++ member functions, and disallowed comparisons in integral constant expressions. Repository: rG LLVM Github M

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen marked an inline comment as done. samitolvanen added a comment. In D108479#3133578 , @rjmccall wrote: > If we do need to support constant expressions of this Yes, we need this also in constant expressions. > I think we should have the const

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3140688 , @rjmccall wrote: > In D108479#3140638 , @samitolvanen > wrote: > >> In D108479#3133578

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 389242. samitolvanen marked an inline comment as done. samitolvanen added a comment. Refactored to avoid evaluating the expression into an l-value. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added a comment. In D108479#3140688 , @rjmccall wrote: > In D108479#3140638 , @samitolvanen > wrote: > >> Sure, I can take a look at how that would work. Basically, in >> `PointerExprEvaluator::Visi

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3149297 , @rjmccall wrote: > In D108479#3149228 , @samitolvanen > wrote: > >> I worked around this for now by explicitly all

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 389346. samitolvanen added a comment. Changed the code to evaluate the argument as a constant expression. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files:

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:208 +if (UnaryOp->getOpcode() == UnaryOperator::Opcode::UO_AddrOf) + E = UnaryOp->getSubExpr(); + rjmccall wrote: > samitolvanen wrote: > > rjmccall wrote: > > > samitolvanen

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3150298 , @rjmccall wrote: > Your builtin is using custom type-checking (`t`), which suppresses all the > normal conversions that happen on expressions. Specifically

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 390521. samitolvanen added a comment. Use standard l-value conversions, and add a test case for `constexpr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 File

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

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

[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 https://reviews.llvm.org/D134831/n

[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: https://reviews.llvm.org/D134831?

[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 an

[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 assig

[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: clang/include/clang/Ba

[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 p

[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 tak

[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 executable-o

[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: llvm/include/llvm/CodeGen/MachineInstr.h:265 + PointerSumTypeMember, + PointerSumTypeMember>, + PointerSumTypeMember> samitolvanen wrote: > This fails on 32-bit architectures as `PointerEmbeddedInt` d

[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 an

[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 separa

[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 thin

[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 clang/include/

[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 L

[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 w

[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 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 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. https://lore.kernel.org/all/20220930203310.4010564-1-samitolva...@google.

  1   2   >