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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];
----------------
nickdesaulniers wrote:
> 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?
That's what I meant by this being novel -- I don't think those test cases will work with this design (having the backend decide whether to diagnose), and so this attribute will behave somewhat inconsistently with other function attributes. This may cause confusion for users or for tooling. I think for users, we can document the behavior and that will be fine. For tooling, we may have to get more creative (such as applying the attribute to all redeclarations of the same function), but perhaps we can hold off on that until we see if tooling runs into a problem in practice.


================
Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];
----------------
nickdesaulniers wrote:
> 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?
There's nothing platform-specific about the semantics of these attributes, so GNU or not doesn't really matter. I'm fine holding off on ObjC methods until a user requests support for it, but given how these are generally useful attributes, I think it would make sense to support ObjC up front unless there's a burden from supporting it.


================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];
----------------
nickdesaulniers wrote:
> 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"))`
> We'd need to be able to distinguish between the Spellings at runtime, I think. It looks like those are private to ParsedAttrInfos, so I'm not sure how we could check that.

That's the "accessor" thing I was talking about.
```
def Diagnostic : InheritableAttr {
  let Spellings = [GCC<"warning">, GCC<"error">];
  ...
  let Accessors = [Accessor<"isError", [GCC<"error">]>,
    Accessor<"isWarning", [GCC<"warning">]>];
  ...
}
```
Would let you do:
```
DiagnosticAttr *DA = ...;
if (DA->isError()) ...
...
if (DA->isWarning()) ...
```

> So I probably should check for multiple instances of the attribute, and use the message from the latest instance.

I don't think we should follow GCC's lead and produce both a warning and an error diagnostic. I think if the attributes are consistent (both error or both warning), then we should warn if the text is different but use the last attribute's argument as the message (if the text is the same, we should not diagnose at all); if the attributes are inconsistent, I think we should probably give an error rather than a warning, but a case could be made to warn and let the last attribute "win". I consider different arguments or different attributes to be an indication of programmer confusion that's worth the user dealing with.

> So we don't currently have anything for that? 

We do have a bunch of things for this. See `mergeDeclAttribute()` in SemaDecl.cpp for the typical code pattern.


================
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.
+  }];
----------------
nickdesaulniers wrote:
> 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?
> What's the preferred method to do so here?

Write the `Documentation` subclass once in AttrDocs.td and refer to it in both attributes in Attr.td (IIRC, this may require adding a `let Heading = "whatever"` in AttrDocs.td). However, I think there should only be one attribute in Attr.td and so I think the documentation sharing issue will fall out naturally from there.

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

Assuming the base method is marked and the derived is not, if this means you don't warn on `SomeDerived->foo()`, then I think that makes sense but if that means you don't warn on `SomeBase->foo()`, then I think there's an issue.

> 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?

I guess I don't see this as garbage in so much as trying to verify that the behavior under this patch isn't unreasonable. Naively, I would expect no diagnostic from that case because I wouldn't expect the compiler to be able to "see" the annotation in the other TU. But I have no idea if that's actually the behavior.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 
----------------
nickdesaulniers wrote:
> 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...
Ah! I think the warning attribute should be controllable via a diagnostic flag (we should always be able to disable warnings with some sort of flag) and I don't think the error attribute should be controllable (an error is an error and should stop translation, same as with `#error`).

Normally, I'd say "let's use the same flag that controls `#warning`" but...
```
def PoundWarning : DiagGroup<"#warnings">;
```
That's... not exactly an obvious flag for the warning attribute. So I would probably name it `warning-attributes` similar to how we have `deprecated-attributes` already.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5308-5310
+  if (TargetDecl)
+    if (TargetDecl->hasAttr<ErrorAttr>() ||
+        TargetDecl->hasAttr<WarningAttr>()) {
----------------
nickdesaulniers wrote:
> 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.
If `TargetDecl` is `nullptr`, then `&&` short-circuits and you never get into the parenthesized part.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
nickdesaulniers wrote:
> 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.
I thought backend optimization passes would drop source location information with some regularity, so I'm a bit hesitant to go with an assertion unless it turns out I'm wrong there. However, if the backend shouldn't ever lose *this* source location information, then I think it's good to make it an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030