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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Clang parts LGTM, thank you for this!



================
Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting attribute is here}}
+
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > I think the diagnostic order is backwards here. The first declaration is where I'd expect the note and the second declaration is where I'd expect the error. (The idea is: the first declaration adds an attribute to the decl, so the redeclaration is what introduces the conflict and so that's where the error should live.) As an example: https://godbolt.org/z/bjGTWxYvh
> > > I'm not sure how to reconcile this. Looking at `bad4` above (different case than `bad5`), the diagnostic we get is:
> > > ```
> > > /tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
> > > __attribute__((error("foo"), warning("foo")))
> > >                              ^
> > > /tmp/y.c:1:16: note: conflicting attribute is here
> > > __attribute__((error("foo"), warning("foo")))
> > >                ^
> > > 1 error generated.
> > > ```
> > > which looks correct to me. If I flip the order these are diagnosed in, that fixes `bad5` but if I flip around the ordering:
> > > ```
> > > --- a/clang/lib/Sema/SemaDeclAttr.cpp
> > > +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> > > -      Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI << EA;
> > > -      Diag(EA->getLocation(), diag::note_conflicting_attribute);
> > > +      Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
> > > +          << CI << EA;
> > > +      Diag(CI.getLoc(), diag::note_conflicting_attribute);
> > > ```
> > > Then `bad4` doesn't look quite correct.
> > > ```
> > > /tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
> > > __attribute__((error("foo"), warning("foo")))
> > >                ^
> > > /tmp/y.c:1:30: note: conflicting attribute is here
> > > __attribute__((error("foo"), warning("foo")))
> > >                              ^
> > > ```
> > > Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is the "new" vs the "old?"
> > > which looks correct to me.
> > 
> > That also looks correct to me; the second attribute is the diagnostic and the first attribute is the note.
> > 
> > > Perhaps in the callers of handleErrorAttr I'm confusing which Decl is the "new" vs the "old?"
> > 
> > Huh, this is rather strange. The logic you're using is basically the same as `checkAttrMutualExclusion()`, just with extra work to figure out which kind of attribute you're dealing with. I'm not seeing what's causing the order to be different when I reason my way through the code. Perhaps in the `bad4()` case, are we somehow processing the attributes in reverse order?
> > 
> > FWIW, at the end of the day, we can live with the diagnostic in either situation, even if they're a bit strange. I think the merge behavior (two different decls) is more important to get right because that's going to be the far more common scenario to see the diagnostic in. If it turns out that it's a systemic issue (or just gnarly to figure out), it won't block this review. We can always improve the behavior in a follow-up.
> I suspect that `DiagnoseMutualExclusions` (via tablegen) would be able to catch this, had we distinct `Attr`s for warning and error; example tablegen rule: `def : MutualExclusions<[Hot, Cold]>;`.
I suspect you're correct. Perhaps the logic in ClangAttrEmitter.cpp is already accounting for some oddity here that we're not. The way the generated mutual exclusions look are:
```
  if (const auto *Second = dyn_cast<AlwaysDestroyAttr>(A)) {
    if (const auto *First = D->getAttr<NoDestroyAttr>()) {
      S.Diag(First->getLocation(), diag::err_attributes_are_not_compatible) << First << Second;
      S.Diag(Second->getLocation(), diag::note_conflicting_attribute);
      return false;
    }
    return true;
  }
```
Compared with your code, this is different. In your code, `CI` is analogous to `Second` above, and `EA` is analogous to `First`. You diagnose at the location of `First` but pass in `<< Second (CI) << First (EA)` as the arguments.
```
ErrorAttr *Sema::mergeErrorAttr(Decl *D, const AttributeCommonInfo &CI,
                                StringRef NewUserDiagnostic) {
  if (const auto *EA = D->getAttr<ErrorAttr>()) {
    std::string NewAttr = CI.getNormalizedFullName();
    assert((NewAttr == "error" || NewAttr == "warning") &&
           "unexpected normalized full name");
    bool Match = (EA->isError() && NewAttr == "error") ||
                 (EA->isWarning() && NewAttr == "warning");
    if (!Match) {
      Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
          << CI << EA;
      Diag(CI.getLoc(), diag::note_conflicting_attribute);
      return nullptr;
    }
    if (EA->getUserDiagnostic() != NewUserDiagnostic) {
      Diag(CI.getLoc(), diag::warn_duplicate_attribute) << EA;
      Diag(EA->getLoc(), diag::note_previous_attribute);
    }
    D->dropAttr<ErrorAttr>();
  }
  return ::new (Context) ErrorAttr(Context, CI, NewUserDiagnostic);
}
```
Regardless, I'm content with the behavior of your tests, so I think we can move on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030