[Clang] add support for error+warning fn attrs

[nickdesaulniers]

Summary

Add support for the GNU C style attribute((error(""))) and
attribute((warning(""))). These attributes are meant to be put on
declarations of functions whom should not be called.

They are frequently used to provide compile time diagnostics similar to
_Static_assert, but which may rely on non-ICE conditions (ie. relying on
compiler optimizations). This is also similar to diagnose_if function
attribute, but can diagnose after optimizations have been run.

While users may instead simply call undefined functions in such cases to
get a linkage failure from the linker, these provide a much more
ergonomic and actionable diagnostic to users and do so at compile time
rather than at link time. Users instead may be able use inline asm .err
directives.

These are used throughout the Linux kernel in its implementation of
BUILD_BUG and BUILD_BUG_ON macros. These macros generally cannot be
converted to use _Static_assert because many of the parameters are not
ICEs. The Linux kernel still needs to be modified to make use of these
when building with Clang; I have a patch that does so I will send once
this feature is landed.

To do so, we create a new IR level Function attribute, "dontcall" (both
error and warning boil down to one IR Fn Attr). Then, similar to calls
to inline asm, we attach a !srcloc Metadata node to call sites of such
attributed callees.

The backend diagnoses these during instruction selection, while we still
know that a call is a call (vs say a JMP that's a tail call) in an arch
agnostic manner.

The frontend then reconstructs the SourceLocation from that Metadata,
and determines whether to emit an error or warning based on the callee's
attribute.

Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: https://github.com/ClangBuiltLinux/linux/issues/1173

[nickdesaulniers]

clang/include/clang/Basic/AttrDocs.td

6082

I've added a well formed input test. We don't check generally that during LTO fn attrs match (we check the type signatures match).

Marking as done, but please reopen/reply to this thread if there's additional thoughts.

Perhaps a new thread on expanding the documentation would be good?

clang/lib/CodeGen/CodeGenModule.cpp

2194

I think this is a no. I tried moving this there and wound up with various added tests failing. Will remove unless there's additional feedback here.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3845

That said, the patch as is works for functions (not ObjC methods) in objective-c mode already; would you like a test for that?

No thanks, I think the existing C coverage handles that.

Marking as Done, please reopen this thread if I'm mistaken in this analysis.

Thanks for checking this out, I think it's a good reason to say "let's deal with this later (if ever)".

clang/lib/CodeGen/CodeGenAction.cpp

783

The current implementation chooses to not diagnose. Is that the best choice?

I think it's a reasonable choice for the initial implementation. We can improve it later when we have a concrete use case. My primary concern was addressed though -- I'd rather have a false negative than a diagnostic with no source location information. The false negative is unfortunate, but IMO better than a frustrating diagnostic without source location information. The upside to the frustrating diagnostic is that we'd get a bug report about that (it's easier to miss the false negative and file a bug report about that). I suppose one way to address that is to use an assertion before the early return with a comment that the assertion is speculative rather than assertive. So in asserts builds we get a loud report and in other builds, we get the false negatives. WDYT?

[nickdesaulniers]

clang/include/clang/Basic/AttrDocs.td

6083

@aaron.ballman anything I should add to the documentation blurb?

clang/include/clang/Basic/DiagnosticGroups.td

1225

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?

clang/lib/CodeGen/CodeGenAction.cpp

783

I'd rather have a false negative than a diagnostic with no source location information. The false negative is unfortunate, but IMO better than a frustrating diagnostic without source location information.

Agreed.

WDYT?

One part is that I added a test case to clang/test/Frontend/backend-attribute-error-warning-optimize.c with a TODO saying "I know this doesn't work yet" that would trigger such an assertion. I think it would be unacceptable to add a new test that is red only for assertion enabled builds, which would be the case if I added the assertion.

So if I add the assertion, then I should just remove the unit test? I guess it doesn't add much value to have a TODO in the sources AND in a unit test both effectively saying "yes, this doesn't work yet."

clang/lib/Sema/SemaDeclAttr.cpp

957

To check this, I should probably add tests using __attribute__((__error__)) as opposed to just testing __attribute__((error))

llvm/include/llvm/IR/DiagnosticInfo.h

1079

TODO: play with llvm::Optional

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

6083

I think the prose is generally fine, but it might be useful to show some trivial example usages to set expectations. e.g.,

__attribute__((warning("oh no"))) void unused_func() {} // Not diagnosed because it's never called
__attribute__((warning("oh no"))) void used_func() {}
__attribute__((error("oh no"))) int unevaluated_func() { return 0; }
__attribute__((error("oh no"))) constexpr int constexpr_func() { return 12; }

void use() {
  used_func(); // Warning
  sizeof(unevaluated_func()); // No error, function never called
  int array[constexpr_func()]; // No error, function never called
}

Feel free to use better/different/more examples, btw. I'm not tied to mine.

clang/include/clang/Basic/DiagnosticGroups.td

1225

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?

clang/lib/CodeGen/CodeGenAction.cpp

783

So if I add the assertion, then I should just remove the unit test? I guess it doesn't add much value to have a TODO in the sources AND in a unit test both effectively saying "yes, this doesn't work yet."

Agreed that duplicating the test coverage doesn't buy us much. I don't have a strong opinion on which way to go, so whatever seems sensible to you is likely fine by me. (I'd be fine if we left the assertion out and just went with the unit test coverage.)

clang/lib/Sema/SemaDeclAttr.cpp

957

I think a better approach is to use AL.getAttributeSpellingListIndex() instead of doing a string comparison directly. ErrorAttr will have a public Spelling enum that's auto-generated for it with all the different spellings. See the Attrs.inc file that's generated for the full list, but usage would look something like: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L3074

[nickdesaulniers]

• remove stale todos, change warning to -Wwarning-attributes to match GCC, update docs

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticGroups.td

1225

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(""))).

llvm/include/llvm/IR/DiagnosticInfo.h

1079

Ok, that was fun, but I didn't find it made construction sites of DiagnosticInfoDontCall nicer. This is also how DiagnosticInfoInlineAsm works, which is where I got the idea for !srcloc from.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

77

79

clang/include/clang/Basic/DiagnosticGroups.td

1225

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11172 ↗

(On Diff #364280)

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.

clang/lib/Sema/SemaDecl.cpp

2986

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

clang/lib/Sema/SemaDeclAttr.cpp

955

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.

958–959

963

964–965

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?

972

[nickdesaulniers]

• rebase on D107613, reuse diag::err_attribute_missing_on_first_decl
• test matching redeclarations
• remove getSpelling method call for diag
• capitalize Match
• convert an assert to a static_assert
• reword diagnostic

clang/include/clang/Basic/DiagnosticSemaKinds.td

11172 ↗

(On Diff #364280)

Rebased onto D107613; reusing diag::err_attribute_missing_on_first_decl now. done.

clang/lib/Sema/SemaDecl.cpp

2986

Rebased onto D107613, so this block was moved from Sema::mergeDeclAttributes (which should be for Attributes on any subclass of Decl, which is too generic for a FunctionDecl only Attribute) to Sema::MergeFunctionDecl (which is only for FunctionDecl Attributes).

I had a similar test case already in clang/test/Sema/attr-error.c testing __attribute__((error("foo"), error("foo"))) int good1(void);, but I will add a test to that file for redeclarations with matching attributes. done.

clang/lib/Sema/SemaDeclAttr.cpp

955

I'm not sure that I need to change anything here; redeclarations of ErrorAttrs are checked in Sema::MergeFunctionDecl.

But when I was playing around with something else here earlier this week, I noticed a pre-existing inconsistency (or maybe just a difference, described below) of handle*Attr functionns vs merge*Attr functions.

mergeDeclAttribute in clang/lib/Sema/SemaDecl.cpp has a large if/else chain for new attrs, which calls merge*Attr methods of Sema, but it doesn't give your a reference to any "Old"/pre-existing attr that may be in conflict. A comment in mergeDeclAttribute seems to confirm this.

This function copies an attribute Attr from a previous declaration to the
new declaration D if the new declaration doesn't itself have that attribute
// yet or if that attribute allows duplicates.

Because we do not allow this Attribute to be added on subsequent Decls, I _do not think_ we need to follow the attribute merge pattern used in SemaDecl.cpp. But I could be wrong; please triple check.

964–965

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?

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

964–965

(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!

clang/test/Frontend/backend-attribute-error-warning-optimize.c

2

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.

clang/test/Frontend/backend-attribute-error-warning.c

4

A different approach to using the preprocessor is to use give a prefix to -verify. e.g., on the previous RUN line, you can do: -verify=expected, enabled and on this run line you can do -verify. Then, when you have an optional warning, you can use // enabled-warning {{whatever}} and it'll be checked only for the first RUN line due to the -verify arguments.

clang/test/Frontend/backend-attribute-error-warning.cpp

2–3 ↗

(On Diff #364892)

Given how similar this is to the .c test, I'd recommend merging the contents of the tests together with an additional RUN line with -x c++ to compile in C++ mode and using #if defined(__cplusplus) for the very small amount of C++-specific stuff being tested. WDYT?

[nickdesaulniers]

• combine c++ w/ c test
• use -verify= rather than -D
• remove REQUIRES
• go back to string comparison

clang/lib/Sema/SemaDeclAttr.cpp

964–965

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

clang/test/Frontend/backend-attribute-error-warning-optimize.c

2

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.

[aaron.ballman]

clang/lib/CodeGen/CodeGenAction.cpp

783

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).

clang/lib/Sema/SemaDeclAttr.cpp

964–965

Thanks!

clang/test/CodeGen/attr-error.c

10

This is failing the CI pipeline on Windows because the declaration there looks like: declare dso_local void @foo() #1

clang/test/CodeGen/attr-warning.c

10

Similar issue here as above.

clang/test/Frontend/backend-attribute-error-warning-optimize.c

2

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.

clang/test/Sema/attr-error.c

27

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

[nickdesaulniers]

• fix tests on windows, let diag eng format decl, drop correct attr, refactor to handle merging as InheritableAttr

clang/lib/Sema/SemaDecl.cpp

3372

Should be dropping ErrorAttr.

clang/test/Sema/attr-error.c

27

Ah, that exposes the case you discussed above:

I think you need to follow the attribute merge pattern used in SemaDecl.cpp.

So I did a relatively larger refactor to support that. Doing so required me to make this an InheritableAttr. PTAL

[nickdesaulniers]

• remove unnecessary parens

[aaron.ballman]

clang/test/Sema/attr-error.c

32–33

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

[nickdesaulniers]

clang/test/Sema/attr-error.c

32–33

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

[aaron.ballman]

clang/test/Sema/attr-error.c

32–33

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.

[nickdesaulniers]

• reorder diag vs note

[nickdesaulniers]

clang/test/Sema/attr-error.c

32–33

I suspect that DiagnoseMutualExclusions (via tablegen) would be able to catch this, had we distinct Attrs for warning and error; example tablegen rule: def : MutualExclusions<[Hot, Cold]>;.

[aaron.ballman]

Clang parts LGTM, thank you for this!

clang/test/Sema/attr-error.c

32–33

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.

[nickdesaulniers]

I'm getting test failures for the newly added llvm/test/CodeGen/X86/attr-dontcall.ll, thought there's not much info other than

Exit Code: 1

[nickdesaulniers]

Fixup: https://reviews.llvm.org/D108718

[aeubanks]

Would it be ok to split this into two attributes, dontcall-warn and dontcall-error? So we rely less on the Clang AST when determining if a dontcall call should be emitted as a warning or an error.

[nickdesaulniers]

In D106030#3018754, @aeubanks wrote:

Would it be ok to split this into two attributes, dontcall-warn and dontcall-error? So we rely less on the Clang AST when determining if a dontcall call should be emitted as a warning or an error.

Doesn't matter to me; go for it. BackendConsumer::DontCallDiagHandler is what looks at the AST.