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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
