[PATCH] D119296: KCFI sanitizer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:695
 
+def KCFIUnchecked : Attr {
+  let Spellings = [Clang<"kcfi_unchecked">];
----------------
Is there a reason the attribute isn't inheritable? e.g.,
```
[[clang::kcfi_unchecked]] extern int i;
int i; // Should this definition have the attribute?
```
https://godbolt.org/z/5Pxjd4nMK (note line 5 of the AST dump, using the `common` attribute as an example)


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5405
+  let Content = [{
+The ``kcfi_unchecked`` attribute causes Clang to not emit KCFI checks for indirect
+calls made through annotated function pointer variables or types.
----------------
Can you link to something that tells the user what KCFI is?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5406
+The ``kcfi_unchecked`` attribute causes Clang to not emit KCFI checks for indirect
+calls made through annotated function pointer variables or types.
+
----------------
There's some confusion here -- if it applies to a typedef to a function pointer type, why does it not apply to a function type directly? e.g.,
```
void func(void) {}
typedef void (*fp)(void) __attribute__((kfci_unchecked));

int main(void) {
  fp Func = func;
  Func(); // Okay, unchecked

  ((void (* __attribute__((kcfi_unchecked)))(void))func)(); // Error, but is equivalent to above
}
```
I can't tell what kind of attribute this wants to be, type or decl, but this matters because the attribute is exposed with the `[[]]` spelling where this detail is important.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5424
+
+``kcfi_unchecked`` is  only supported for function pointers and function pointer types.
+}];
----------------
Does this attribute make sense in C++? If so, there's more questions -- like, can the attribute be applied to pointer to member function objects? What happens to calls through a template type, as in:
```
template <typename Fn>
void CallIt(Fn F) { F(); } // Is this call checked or not?

void func() {}

int main() {
  using unchecked_t = decltype((func)) [[clang::kcfi_unchecked]]; // I may have messed the syntax up
  unchecked_t F = func;
  CallIt(F);
}
```


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3059
   InGroup<IgnoredAttributes>;
+def err_attribute_function_pointers_only : Error<
+  "%0 attribute argument only applies to a function pointer or a function "
----------------
This diagnostic is not required, it's already covered by the automatic attribute checking that comes from tablegen. (There are other issues with the diagnostic as well -- not clear what the distinction is between a function pointer and a function pointer type given that they're both types, but we can likely skip that discussion.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4703-4712
+  if (auto *TD = dyn_cast<TypedefNameDecl>(D))
+    Type = TD->getUnderlyingType();
+  else if (auto *VD = dyn_cast<VarDecl>(D))
+    if (auto *TSI = VD->getTypeSourceInfo())
+      Type = TSI->getType();
+
+  if (Type.isNull() || !Type->isFunctionPointerType()) {
----------------
None of this should be required -- the attribute has a subject list and no custom parsing, so the common attribute feature handler should do all of this. I think you can replace the entire handler with `handleSimpleAttribute<KCFIUncheckedAttr>(S, D, AL)` in the `switch` below unless there's other checking required.


================
Comment at: clang/test/CodeGen/kcfi.c:55
+
+int test() {
+  return call(f1) +
----------------



================
Comment at: clang/test/Sema/attr-kcfi_unchecked.c:12
+void f2(void *p __attribute__((kcfi_unchecked))) {}         // expected-error {{'kcfi_unchecked' attribute argument only applies to a function pointer or a function pointer type}}
+void f3(void (*p)(void) __attribute__((kcfi_unchecked))) {}
----------------
Missing other tests: attribute accepts no arguments, use of the C++ spelling on C++ constructs (like `using` declarations or calling through templates), etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296