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