[PATCH] D106030: [Clang] add support for error+warning fn attrs
Aaron Ballman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 13 05:22:26 PDT 2021
aaron.ballman added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+ : diag::warn_fe_backend_warning_attr)
+ << FD->getDeclName() << EA->getUserDiagnostic();
+ }
----------------
The diagnostics engine knows how to properly format a `NamedDecl *` directly (and this will take care of properly quoting the name in the diagnostics as well).
================
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) {
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > 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?
> > > For reference, the generated `enum Spelling` looks like:
> > > ```
> > > 3611 public:
> > > 3612 enum Spelling {
> > > 3613 GNU_error = 0,
> > > 3614 CXX11_gnu_error = 1,
> > > 3615 C2x_gnu_error = 2,
> > > 3616 GNU_warning = 3,
> > > 3617 CXX11_gnu_warning = 4,
> > > 3618 C2x_gnu_warning = 5,
> > > 3619 SpellingNotCalculated = 15
> > > 3620
> > > 3621 };
> > > ```
> > > while using `getAttributeSpellingListIndex` rather than `getNormalizedFullName` allows us to avoid a pair of string comparisons in favor of a pair of `unsigned` comparisons, we now have an issue because there are technically six spellings. (I guess it's not clear to me what's `CXX11_gnu_*` vs `C2x_gnu_*`?) We could be explicit against checking all six rather than two comparisons.
> > >
> > > Or I can use local variables with more helpful identifiers like:
> > >
> > > ```
> > > bool NewAttrIsError = NewAttrSpellingIndex < ErrorAttr::GNU_warning;
> > > bool NewAttrIsWarning = NewAttrSpellingIndex >= ErrorAttr::GNU_warning;
> > > bool Match = (EA->isError() && NewAttrIsError) || (EA->isWarning() && NewAttrIsWarning);
> > > ```
> > >
> > > WDYT?
> > > (I guess it's not clear to me what's CXX11_gnu_* vs C2x_gnu_*?)
> >
> > A bit of a historical thing. C2x `[[]]` and C++11 `[[]]` are modeled as different spellings. This allows us to specify attributes with a `[[]]` spelling in only one of the languages without having to do an awkward dance involving language options. e.g., it makes it easier to support an attribute spelled `__attribute__((foo))` and `[[vendor::foo]]` in C2x and spelled `[[foo]]` in C++. it picks up the `gnu` bit in the spelling by virtue of being an attribute with a `GCC` spelling -- IIRC, we needed to distinguish between GCC and Clang there for some reason I no longer recall.
> >
> > > WDYT?
> >
> > I think the current approach is reasonable, but I think the previous approach (doing the string compare) was more readable. If you have a preference for using the string compare approach as it originally was, I'd be fine with that now that I see how my suggestion is playing out in practice. If you'd prefer to keep the current approach, I'd also be fine with that. Thank you for trying this out, sorry for the hassle!
> > now that I see how my suggestion is playing out in practice
> > Thank you for trying this out, sorry for the hassle!
>
> Ah, that's ok. [[ https://www.youtube.com/watch?v=aPVLyB0Yc6I | Sometimes you eat the bar, sometimes the bar eats you. ]] I've done that myself already in this patch when playing with llvm::Optional (and C++ default parameters).
>
> Changed back to string comparison now. I don't expect any of this code to be hot, ever. Marking as done.
Thanks!
================
Comment at: clang/test/CodeGen/attr-error.c:9
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
----------------
This is failing the CI pipeline on Windows because the declaration there looks like: `declare dso_local void @foo() #1`
================
Comment at: clang/test/CodeGen/attr-warning.c:9
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
----------------
Similar issue here as above.
================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:1
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -O2 -verify -emit-codegen-only %s
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > Out of curiosity, why is this required? I would have assumed this would be backend-independent functionality?
> >
> > Also, I have no idea where these tests should live. They're not really frontend tests, it's more about the integration between the frontend and the backend. We do have `clang/test/Integration` but there's not a lot of content there to be able to say "oh yeah, this is testing the same sort of stuff". No idea if other reviewers have opinions on this.
> Since I got the idea for this feature from -Wframe-larger-than=, I followed clang/test/Frontend/backend-diagnostic.c, both for the REQUIRES line and the test location.
>
> I guess clang/test/Frontend/backend-diagnostic.c uses x86 inline asm, so maybe I can remove the REQUIRES line. But if my new tests move then so should clang/test/Frontend/backend-diagnostic.c (maybe pre-commit). Will leave that for other reviewers, but removing REQUIRES lines.
Given that there's not an obviously correct answer, I'm fine leaving the tests here. We can rearrange the deck chairs a different day if we'd like.
================
Comment at: clang/test/Sema/attr-error.c:26
+
+__attribute__((error("foo"), warning("foo"))) // expected-error {{'warning' and 'error' attributes are not compatible}}
+int
----------------
Can you also add a test for:
```
__attribute__((error("foo"))) int bad5(void); // expected-note {{conflicting attribute is here}}
__attribute__((warning("foo"))) int bad5(void); // expected-error {{'warning' and 'error' attributes are not compatible}}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106030/new/
https://reviews.llvm.org/D106030
More information about the llvm-commits
mailing list