[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];
----------------
I think this should be an inheritable attribute (same below) so that redeclarations get the marking as well.

However, this does make for a bit of a novel situation with regards to other attributes. The typical behavior for function attributes is:
```
void func(void);

void use1(void) {
  func(); // Func is not marked yet, no attribute behavior
}

void func(void) __attribute__((whatever)));

void use2(void) {
  func(); // Func is marked, attribute does something
}

void func(void) {} // Func is still marked because the attribute is inheritted

void use3(void) {
  func(); // Func is marked, attribute does something
```
but because all of the interesting work is happening in the backend, I believe the unmarked use will still act as though the attribute was marked.


================
Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];
----------------
ObjC methods as well?


================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];
----------------
Given that the only functional difference between these attributes is the diagnostic level, I sort of wonder whether we should have one semantic attribute with two spellings (one for warning, one for error) and an accessor field to distinguish which is which.

Another interesting question is: are these attributes mutually exclusive? Can they be duplicated (perhaps across declarations)? What happens if the messages differ? (We may need some attribute merging logic to catch these situations.)


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+    depend on optimizations, while providing diagnostics pointing to precise
+    locations of the call site in the source.
+  }];
----------------
I think the documentation for these should probably be combined into one documentation blob; the only difference in behavior is whether the diagnostic is a warning or an attribute.

I think the documentation needs to go into more details about how this attribute works in practice. For example, what should I expect from code like:
```
struct Base {
  __attribute__((warning("derp"))) virtual void foo();
};

struct Derived : Base {
  void foo() override; // Does calling this also warn?
};

__attribute__((error("DERP!"))) void func() { // external function symbol!
  func(); // Does this diagnose?
}
```
I suppose another interesting question given the design is to use the optimizer is: what about LTO? Say I have:
```
// TU1.c
__attribute__((error("Derp Derp Derp"))) void func(void);

// TU.c
extern void func(void);
void blerp(void) { func(); }
```
What should happen and does LTO change the answer?


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:75
 
+def err_fe_backend_user_diagnostic :
+  Error<"call to %0 declared with attribute error: %1">, BackendInfo;
----------------
Should probably give these names that relate to the attribute. How about `err_fe_backend_error_attr` (and `warn_fe_backend_warn_attr`) or something along those lines?


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:78
+def warn_fe_backend_user_diagnostic :
+  Warning<"call to %0 declared with attribute warning: %1">, BackendInfo, InGroup<BackendUserDiagnostic>;
+
----------------
80-col limit


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 
----------------
GCC doesn't seem to support this spelling -- do they have a different one we should reuse?


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5308-5310
+  if (TargetDecl)
+    if (TargetDecl->hasAttr<ErrorAttr>() ||
+        TargetDecl->hasAttr<WarningAttr>()) {
----------------



================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:776
+
+      CalleeName = FD->getName();
+    }
----------------
This isn't really safe -- not all functions have names that can be retrieved this way. We should be sure to add some test coverage for functions without "names" (like `operator int()`).

I think it's better to call `getNameForDiagnostic()` here (unless the diagnostic printer does that already for a `DeclarationName`, in which case `getDeclName()` would be better.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
I think the usability in this situation is a concern -- only having a function name but no source location information is pretty tricky. Do you have a sense for how often we would hit this case?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:949-960
+static void handleErrorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+    return;
+  D->addAttr(::new (S.Context) ErrorAttr(S.Context, AL, Str));
+}
+static void handleWarningAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
----------------
Pretty sure you can get rid of the manual handlers and just use `SimpleHandler = 1` in Attr.td, unless we need to add extra diagnostic logic (which we might need to do for attribute merging).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030