[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

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 wonder if this should be made into an actual keyword.  This involves some boilerplate additions to:
> - llvm/include/llvm/AsmParser/LLToken.h
> - llvm/include/llvm/Bitcode/LLVMBitCodes.h
> - llvm/include/llvm/IR/Attributes.td
> - llvm/lib/AsmParser/LLLexer.cpp
> - llvm/lib/Bitcode/Reader/BitcodeReader.cpp
> - llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
> - llvm/lib/Transforms/Utils/CodeExtractor.cpp
> - llvm/test/Bitcode/attributes.ll
> - llvm/include/llvm/IR/Attributes.td
> 
> I haven't seen guidance as to which is preferred or what the tradeoffs are. These string attrs are way less boilerplate, but not seeing support in CodeExtractor makes me slightly dubious. Let me ask on LLVM's discourse (or open question to other reviewers).
> https://discourse.llvm.org/t/ir-string-vs-tablegend-attributes-and-boilerplate/61914
> 
> 
> Also, this reminds me; isn't there a fn attr we use today in the kernel to blanket say "don't instrument this?" I wonder if that needs to be updated to know about disabling CFI, if I'm remembering correctly and that is necessary. I'm thinking of `noinstr` in the Linux kernel, which looks like it sets a bunch of things.
> Also, this reminds me; isn't there a fn attr we use today in the kernel to blanket say "don't instrument this?" I wonder if that needs to be updated to know about disabling CFI, if I'm remembering correctly and that is necessary. I'm thinking of `noinstr` in the Linux kernel, which looks like it sets a bunch of things.

I don't think we want to disable CFI for everything that has `__noinstr` as this covers a fair amount of code.  For arm64, we disable CFI only for functions that actually shouldn't have it enabled.


================
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:
> Is this more concise using `llvm::all_of` from llvm/ADT/STLExtras.h?
Yes, but also slower. This is the same function that we use in LLVM to check for acceptable promotion alias names (https://reviews.llvm.org/D104058). Should this be moved into a helper function somewhere? If so, where?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296