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

Mon Jul 19 14:08:45 PDT 2021

nickdesaulniers added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];
----------------
aaron.ballman wrote:
> 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.
Changing this def to:
```
-def Error : Attr {
+def Error : InheritableAttr {
```
doesn't seem to make your test case work; is there some other method I should be using to find the re-declaration and check the attributes against that?

================
Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];
----------------
aaron.ballman wrote:
> ObjC methods as well?
I guess I can add them, but I'm unfamiliar with the language. If there's no GNU implementation of an ObjC compiler, do we need to worry about GNU C extensions in ObjC?

================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];
----------------
aaron.ballman wrote:
> 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.)
> I sort of wonder whether we should have one semantic attribute with two spellings 

We'd need to be able to distinguish between the `Spellings` at runtime, I think. It looks like those are private to `ParsedAttrInfo`s, so I'm not sure how we could check that.

I think we'd want to somehow check the spelling in the code I added to `clang/lib/Sema/SemaDeclAttr.cpp`? Or is there a different approach that might work better?

> are these attributes mutually exclusive?

```
__attribute__((error("err"),warning("warn"),error("err2")))
void foo(void);

void x1(void) { foo(); }
```
in GCC produces a warning with message "warn" and error with message "err2".  In my current implementation, we error once with message "err".  So I probably should check for multiple instances of the attribute, and use the message from the latest instance.  Oh, but I'm just calling `getUserDiagnostic` which was a table-gen'd getter; how do I even specify which instance of the attribute when there are multiple?  For example:
```
__attribute__((error("err"),error("err2")))
```
calls to `getUserDiagnostic` produce `err`...

> We may need some attribute merging logic to catch these situations.

So we don't currently have anything for that? What happens with `__attribute__(alias("")))` when multiple are given? ex. `__attribute__(alias("bar"),alias("foo"))`

================
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.
+  }];
----------------
aaron.ballman wrote:
> 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?
> I think the documentation for these should probably be combined into one documentation blob

I'm having trouble sharing the documentation block between the two. I've tried declaring a `code` or trying to inherit from a base class, but I can't seem to get either to build.  What's the preferred method to do so here?

>   void foo() override; // Does calling this also warn?

Virtual methods calls do not diagnose, in g++ or my implementation here, regardless of optimization level.

>   func(); // Does this diagnose?

Yes, unless the infinite recursion is removed via optimization; both in gcc and my implementation here.

> What should happen and does LTO change the answer?

This reminds me of having non-matching declarations for the same symbol.  I'm not sure we should codify what should happen in such a case.  garbage in, garbage out. Or do we merge definitions as part of LTO?

================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;

----------------
aaron.ballman wrote:
> GCC doesn't seem to support this spelling -- do they have a different one we should reuse?
I think these diagnostics don't have corresponding flags in GCC, so they cannot be disabled.

Without adding this, clang/test/Misc/warning-flags.c would fail, because I was adding a new unnamed warning `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding this line here, and doing something else?

That test says we shouldn't be adding new warnings not controlled by flags, but that's very much my intention.

Though now `-Wno-user-diagnostic` doesn't produce a `-Wunknown-warning-option` diagnostic...

================
Comment at: clang/lib/CodeGen/CGCall.cpp:5308-5310
+  if (TargetDecl)
+    if (TargetDecl->hasAttr<ErrorAttr>() ||
+        TargetDecl->hasAttr<WarningAttr>()) {
----------------
aaron.ballman wrote:
> 
Can't.  If `TargetDecl` is `nullptr`, then adding parenthesis will change the order of operations, which may lead to a NULL ptr deref at runtime.

================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
aaron.ballman wrote:
> 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?
I don't think we ever encounter it when building the kernel sources. Perhaps I should make this an assertion on `LocCookie.isValid()` instead?

While clang should always attach a `srcloc` MetaData node, other frontend might not, so it's optional.  But this TU is strictly Clang.

================
Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:212
+        if (CallBase *CB = dyn_cast<CallBase>(&I)) {
+          if (Function *Callee = CB->getCalledFunction()) {
+            if (Callee->hasFnAttribute("user-diagnostic")) {
----------------
arsenm wrote:
> What's the expected behavior if the function is called indirectly?
Similar to GCC we only diagnose if we can remove the indirection via optimizations:
https://godbolt.org/z/6ETWYc4e3

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030