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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];
----------------
aaron.ballman wrote:
> 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.
> 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.

GCC's behavior is basically the same as our `ArgList::getLastArg()` (but for attributes); the warning and error attributes aren't mutually exclusive, and we simply accept the last instance's string as the message to display. We'd need to then check for the presence of both.

Ah, also, it seems that `mergeDeclAttribute` is useful for inheritable attributes, example test case:
```
void foo(void);

void x0(void) { foo(); }

__attribute__((error("oh no foo")))
void foo(void);

void x1(void) { foo(); }

void foo(void) {}

void x2(void) { foo(); }
```
vs supporting multiple attributes on one decl, example test case:
```
__attribute__((error("err"),warning("warn"),error("err2")))
void foo(void);

void x1(void) { foo(); }
```

So I should create `Sema::mergeErrorAttr` called from `mergeDeclAttribute` to handle the former case, and modify `handleErrorAttr` to handle the latter case.

Marking this thread done, will leave the other one open for inheritability.


================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > 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.
> > 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.
> 
> GCC's behavior is basically the same as our `ArgList::getLastArg()` (but for attributes); the warning and error attributes aren't mutually exclusive, and we simply accept the last instance's string as the message to display. We'd need to then check for the presence of both.
> 
> Ah, also, it seems that `mergeDeclAttribute` is useful for inheritable attributes, example test case:
> ```
> void foo(void);
> 
> void x0(void) { foo(); }
> 
> __attribute__((error("oh no foo")))
> void foo(void);
> 
> void x1(void) { foo(); }
> 
> void foo(void) {}
> 
> void x2(void) { foo(); }
> ```
> vs supporting multiple attributes on one decl, example test case:
> ```
> __attribute__((error("err"),warning("warn"),error("err2")))
> void foo(void);
> 
> void x1(void) { foo(); }
> ```
> 
> So I should create `Sema::mergeErrorAttr` called from `mergeDeclAttribute` to handle the former case, and modify `handleErrorAttr` to handle the latter case.
> 
> Marking this thread done, will leave the other one open for inheritability.
> > 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/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 
----------------
aaron.ballman wrote:
> 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.
Done, now `-Wno-warning-attributes` doesn't produce `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was there something else I needed to add?


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
aaron.ballman wrote:
> 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.
Perhaps debug info, but this SourceLocation is squirreled away in a Metadata node attached to the call site.

Ah, but via further testing of @arsenm 's point ("what about indirect calls?"), this case is tricky:
```
// clang -O2 foo.c
__attribute__((error("oh no foo")))
void foo(void);

void (*bar)(void);

void baz(void) {
  bar = foo;
  bar();
}
```
since we did not emit the Metadata node on the call, but then later during optimizations we replaced the call to `bar` with a call to `foo`. At that point, the front end did not emit a Metadata node with source location information, so we can't reconstruct the call site properly for the diagnostic. Hmm...I'll need to think more about 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) {
----------------
aaron.ballman wrote:
> 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).
It seems that the use of `Args` in the tablegen definitions is incompatible with `SimpleHander`:
```
In file included from /android0/llvm-project/clang/lib/Sema/ParsedAttr.cpp:108:
tools/clang/include/clang/Sema/AttrParsedAttrImpl.inc:4039:32: error: no matching constructor for initialization of 'clang::ErrorAttr'
  D->addAttr(::new (S.Context) ErrorAttr(S.Context, Attr));
                               ^         ~~~~~~~~~~~~~~~
tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
class ErrorAttr : public Attr {
      ^
tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
tools/clang/include/clang/AST/Attrs.inc:3624:3: note: candidate constructor not viable: requires 3 arguments, but 2 were provided
```
where the 3rd argument would be the `llvm::StringRef UserDiagnostic`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030