samitolvanen added a comment. In D119296#3480793 <https://reviews.llvm.org/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 optimized into direct calls. > > IIRC, this wasn't really an issue for -fsanitize=cfi because 99% of the time > indirect calls that were going to resolve to direct calls had already done so > by the time we got to the LowerTypeTests pass (which was only run during LTO). I also confirmed this, LowerTypeTestsPass drops unneeded checks in a similar way. > I probably wouldn't call the pass "KCFI" though as that implies that the pass > itself implements KCFI. Maybe something like KCFIRemoveChecks would be > better. Or maybe this is simple enough to add to another pass like > InstCombine instead of adding a new one. You're right, InstCombine looks like a good place for this. In my tests, this dropped all the checks from direct calls, including the ones that were inlined. > This seems like a reasonable approach, and was also the approach taken for > the PAuth ABI. The PAuth ABI attaches an operand bundle to the call > instruction and arranges for the code for the check to be generated together > with the call. This helps with avoiding spills of the verified function > pointer between the check and the call. Thanks for the link, I'll take a look. ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2287 + const Twine &Asm = ".weak __kcfi_typeid_" + Name + "\n" + + ".set __kcfi_typeid_" + Name + ", " + + Twine(Id->getZExtValue()) + "\n"; ---------------- pcc wrote: > Won't this bloat symbol tables with mostly unused entries? I was thinking you > could make them hidden, but that wouldn't have an effect on kernel modules. > Maybe you should have this be opt-in with an attribute and have the kernel's > asmlinkage expand to the attribute. This adds ~2k symbols to an arm64 defconfig vmlinux, which is a fair amount, but quite minimal compared to the ~38k jump table symbols we emit with the existing CFI scheme. I did consider using an attribute to reduce the bloat, but since the extra symbols aren't causing any actual issues at the moment, I figured this is something we can optimize later. 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
