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

Thu Aug 5 08:54:15 PDT 2021

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:76
+def err_fe_backend_error_attr :
+  Error<"call to %0 declared with attribute error: %1">, BackendInfo;
+def warn_fe_backend_warning_attr :
----------------

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

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

----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > 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?
> > > > huh, that sounds suspicious -- you don't need to do anything special for `-Wno-foo` handling, that should be automatically supported via tablegen. I'm not certain why you're not seeing `-Wno-warning-attributes` silencing the warnings.
> > > Ah! Because I was testing `__attribute__((error("")))` not `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the latter, not the former.  I should add a test for `-Wno-warning-attributes`! Is this something I also need to add documentation for?
> > Given that this behavior surprised you, I certainly wouldn't oppose mentioning it in the documentation, but I also don't think it's strictly required. That said, I do agree about adding some additional test coverage for when the warning is disabled.
> > 
> > Just to double-check: do you think this functionality needs an "escape hatch" for the error case? e.g., should the `error` attribute generate a warning diagnostic that defaults to being an error, so we have `-Wno-warning-attributes` to turn off `warning` attribute diagnostics and `-Wno-error-attributes` to turn off `error` attribute diagnostics?
> Ah, GCC also controls this via `-Wattribute-warning`; let me rename my implementation.
> 
> > do you think this functionality needs an "escape hatch" for the error case?
> 
> No.  If users don't want an error, then they should prefer `__attribute__((warning("")))` to `__attribute__((error("")))`.
Okay, that's fine by me. If we find a need to give API consumers the ability to ignore the `error` attribute, we can always add one later.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11172

+def err_attribute_removed_on_redeclaration : Error<
+  "'%0' attribute removed from redeclared variable">;
----------------
This diagnostic is a bit confusing to me -- should this be a warning about the attribute being ignored in this case, rather than an error? Alternatively, should this be re-worded to say that the attribute must appear on the first declaration? If the latter, we have some diagnostics that could maybe be combined into using a `%select` for this: `err_noreturn_missing_on_first_decl` and `err_internal_linkage_redeclaration` are awfully similar to the same thing.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:2982

+  // If redeclaration removes error or warning attribute...
+  if (auto *EA = New->getAttr<ErrorAttr>())
----------------
The comment doesn't seem to match the code -- this is when the redeclaration *adds* the attribute. And don't we want to silently accept that if the redeclaration adds the same annotation with the same message? e.g.,
```
__attribute__((error("derp"))) void func();
__attribute__((error("derp"))) void func(); // fine
__attribute__((error("derp"))) void func() {} // fine
```

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:955
+
+  if (const auto *EA = D->getAttr<ErrorAttr>()) {
+    unsigned NewAttrSpellingIndex = AL.getAttributeSpellingListIndex();
----------------
This logic will catch the case where the attributes are processed in a group (e.g., two attributes in one specifier or two attributes on the same declaration) but it won't handle the redeclaration merging case. I think you need to follow the attribute merge pattern used in SemaDecl.cpp.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:958-959
+
+    assert(ErrorAttr::GNU_error < ErrorAttr::GNU_warning &&
+           "Spelling order changed unexpectedly");
+    assert(NewAttrSpellingIndex != ErrorAttr::SpellingNotCalculated &&
----------------

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:963
+
+    bool match =
+        (EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
----------------

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+    bool match =
+        (EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+        (EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+    if (!match) {
----------------
It's a bit tricky for me to tell from the patch -- are the enumerators in the correct order that this logic still works for when the [[]] spelling is used?

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:972
+    if (EA->getUserDiagnostic() != Str) {
+      S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << EA->getSpelling();
+      S.Diag(EA->getLoc(), diag::note_previous_attribute);
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030