[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

[arsenm]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

208 ↗

(On Diff #358812)

Why not just do in in codegen in IRTranslator/SelectionDAGBuilder?

212 ↗

(On Diff #358812)

What's the expected behavior if the function is called indirectly?

219 ↗

(On Diff #358812)

Just do one getFnAttribute instead of pre-checking it?

[nickdesaulniers]

• remove accidentally committed logging statement

[arsenm]

Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn

[efriedma]

In D106030#2878897, @arsenm wrote:

Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn

In C++, you can do trickery with template arguments to trigger compile errors, for example, the following:

#include <type_traits>
template<int T> struct dependent_false : std::false_type {};
template<int x> inline void f() {
  if constexpr (x == 0) {
    /* do something */
  } else if constexpr (x == 1) {
    /* do something else */
  } else {
    static_assert(dependent_false<x>() && "Invalid argument");
  }
}
void g() { f<1>(); }

Now suppose you're an enterprising Linux kernel developer, and you decide you want this in C. You don't really care about portability to compilers other than gcc, or compiling at any optimization level less than -O2, so instead of using a real language feature, you decide to depend on a combination of unintentional compiler semantics and linker diagnostics. So you write something like the following:

#define BUILD_BUG() undefined_symbol()
extern void undefined_symbol();

inline void f(int x) {
  if (x == 0) {
    /* do something */
  } else if (x == 1) {
    /* do something else */
  } else {
    BUILD_BUG();
  }
}
void g() { f(1); }

Then, after you're doing this for a while, you ask the gcc developers for a simple feature: an attribute you can apply to undefined_symbol that makes the compiler print the error message at compile-time instead of link-time.

---

Actually, in clang, you could probably use inline asm like the following to accomplish roughly the same thing, assuming you aren't building with -fno-integrated-as:

#define BUILD_BUG() asm(".err \"BUILD_BUG\"")
static inline void f(int x) {
  if (x == 0) {
    /* do something */
  } else if (x == 1) {
    /* do something else */
  } else {
    BUILD_BUG();
  }
}
void g() { f(2); }

[george.burgess.iv]

Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn

To add a bit more clarification, the goal of this attribute is specifically to emit diagnostics after optimizations such as inlining have taken place. diagnose_if is clang's hammer if we want frontend diagnostics: https://godbolt.org/z/jbzbqEzbG . diagnose_if is a bit more flexible than "the condition must be an ICE per the standard," but AIUI the kernel has code like what Eli mentioned, which clang currently can't work with in the frontend (since it requires inlining, etc etc).

[nickdesaulniers]

In D106030#2878897, @arsenm wrote:

Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn

I interpreted this differently than @efriedma or @george.burgess.iv I think. Rephrasing the question as:

Do we really need to keep the user provided diagnostic text itself in the IR?

The answer is no; we can keep it strictly in the frontend, and reconstitute there; we have access to the FunctionDecl which has the attribute. So I can rework this instead of "user-diagnostic"="oh no" IR Fn Attr, to be just dontcall (in IR) then have the frontend check the FunctionDecl for the text of the user provided diagnostic.

But if the question was more so:

Why do we need this?

Then I think @efriedma and @george.burgess.iv provided adequate reasoning. Perhaps I can reuse some of their thoughts in the commit message / patch description?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3835

I think this should be an inheritable attribute (same below) so that redeclarations get the marking as well.

However, this does make for a bit of a novel situation with regards to other attributes. The typical behavior for function attributes is:

void func(void);

void use1(void) {
  func(); // Func is not marked yet, no attribute behavior
}

void func(void) __attribute__((whatever)));

void use2(void) {
  func(); // Func is marked, attribute does something
}

void func(void) {} // Func is still marked because the attribute is inheritted

void use3(void) {
  func(); // Func is marked, attribute does something

but because all of the interesting work is happening in the backend, I believe the unmarked use will still act as though the attribute was marked.

3838

ObjC methods as well?

3842

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

clang/include/clang/Basic/AttrDocs.td

6066

I think the documentation for these should probably be combined into one documentation blob; the only difference in behavior is whether the diagnostic is a warning or an attribute.

I think the documentation needs to go into more details about how this attribute works in practice. For example, what should I expect from code like:

struct Base {
  __attribute__((warning("derp"))) virtual void foo();
};

struct Derived : Base {
  void foo() override; // Does calling this also warn?
};

__attribute__((error("DERP!"))) void func() { // external function symbol!
  func(); // Does this diagnose?
}

I suppose another interesting question given the design is to use the optimizer is: what about LTO? Say I have:

// TU1.c
__attribute__((error("Derp Derp Derp"))) void func(void);

// TU.c
extern void func(void);
void blerp(void) { func(); }

What should happen and does LTO change the answer?

clang/include/clang/Basic/DiagnosticFrontendKinds.td

75

Should probably give these names that relate to the attribute. How about err_fe_backend_error_attr (and warn_fe_backend_warn_attr) or something along those lines?

78

80-col limit

clang/include/clang/Basic/DiagnosticGroups.td

1217

GCC doesn't seem to support this spelling -- do they have a different one we should reuse?

clang/lib/CodeGen/CGCall.cpp

5333–5335

clang/lib/CodeGen/CodeGenAction.cpp

776

This isn't really safe -- not all functions have names that can be retrieved this way. We should be sure to add some test coverage for functions without "names" (like operator int()).

I think it's better to call getNameForDiagnostic() here (unless the diagnostic printer does that already for a DeclarationName, in which case getDeclName() would be better.

783

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?

clang/lib/Sema/SemaDeclAttr.cpp

950–961

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

[nickdesaulniers]

• change IR Attr to dontcall
• check during ISel(s)
• rename td diag
• handle operator int

[nickdesaulniers]

clang/include/clang/Basic/Attr.td

3835

Changing this def to:

-def Error : Attr {
+def Error : InheritableAttr {

doesn't seem to make your test case work; is there some other method I should be using to find the re-declaration and check the attributes against that?

3838

I guess I can add them, but I'm unfamiliar with the language. If there's no GNU implementation of an ObjC compiler, do we need to worry about GNU C extensions in ObjC?

3842

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 ParsedAttrInfos, 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"))

clang/include/clang/Basic/AttrDocs.td

6066

I think the documentation for these should probably be combined into one documentation blob

I'm having trouble sharing the documentation block between the two. I've tried declaring a code or trying to inherit from a base class, but I can't seem to get either to build. What's the preferred method to do so here?

void foo() override; // Does calling this also warn?

Virtual methods calls do not diagnose, in g++ or my implementation here, regardless of optimization level.

func(); // Does this diagnose?

Yes, unless the infinite recursion is removed via optimization; both in gcc and my implementation here.

What should happen and does LTO change the answer?

This reminds me of having non-matching declarations for the same symbol. I'm not sure we should codify what should happen in such a case. garbage in, garbage out. Or do we merge definitions as part of LTO?

clang/include/clang/Basic/DiagnosticGroups.td

1217

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

clang/lib/CodeGen/CGCall.cpp

5333–5335

Can't. If TargetDecl is nullptr, then adding parenthesis will change the order of operations, which may lead to a NULL ptr deref at runtime.

clang/lib/CodeGen/CodeGenAction.cpp

783

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.

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

212 ↗

(On Diff #358812)

Similar to GCC we only diagnose if we can remove the indirection via optimizations:
https://godbolt.org/z/6ETWYc4e3

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3835

That's what I meant by this being novel -- I don't think those test cases will work with this design (having the backend decide whether to diagnose), and so this attribute will behave somewhat inconsistently with other function attributes. This may cause confusion for users or for tooling. I think for users, we can document the behavior and that will be fine. For tooling, we may have to get more creative (such as applying the attribute to all redeclarations of the same function), but perhaps we can hold off on that until we see if tooling runs into a problem in practice.

3838

There's nothing platform-specific about the semantics of these attributes, so GNU or not doesn't really matter. I'm fine holding off on ObjC methods until a user requests support for it, but given how these are generally useful attributes, I think it would make sense to support ObjC up front unless there's a burden from supporting it.

3842

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.

clang/include/clang/Basic/AttrDocs.td

6066

What's the preferred method to do so here?

Write the Documentation subclass once in AttrDocs.td and refer to it in both attributes in Attr.td (IIRC, this may require adding a let Heading = "whatever" in AttrDocs.td). However, I think there should only be one attribute in Attr.td and so I think the documentation sharing issue will fall out naturally from there.

Virtual methods calls do not diagnose, in g++ or my implementation here, regardless of optimization level.

Assuming the base method is marked and the derived is not, if this means you don't warn on SomeDerived->foo(), then I think that makes sense but if that means you don't warn on SomeBase->foo(), then I think there's an issue.

This reminds me of having non-matching declarations for the same symbol. I'm not sure we should codify what should happen in such a case. garbage in, garbage out. Or do we merge definitions as part of LTO?

I guess I don't see this as garbage in so much as trying to verify that the behavior under this patch isn't unreasonable. Naively, I would expect no diagnostic from that case because I wouldn't expect the compiler to be able to "see" the annotation in the other TU. But I have no idea if that's actually the behavior.

clang/include/clang/Basic/DiagnosticGroups.td

1217

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.

clang/lib/CodeGen/CGCall.cpp

5333–5335

If TargetDecl is nullptr, then && short-circuits and you never get into the parenthesized part.

clang/lib/CodeGen/CodeGenAction.cpp

783

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.

[nickdesaulniers]

• merge ErrorAttr with WarningAttr
• fix dumb short circuit bug
• rename diag group
• handle multiple instances of the attribute on one fn

Note: I'll be offline the next few days; still plenty of TODOs to work out, but
leaving this in the review queue for now.

clang/include/clang/Basic/Attr.td

3842

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.

3842

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.

clang/include/clang/Basic/DiagnosticGroups.td

1217

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?

clang/lib/CodeGen/CodeGenAction.cpp

783

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.

clang/lib/Sema/SemaDeclAttr.cpp

950–961

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.

[nickdesaulniers]

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

30

because the callee has a warning attribute! duh...TODO: delete comment.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticCommonKinds.td

375–378 ↗

(On Diff #360324)

FWIW, elsewhere we typically use warn_duplicate_attribute and note_previous_attribute as a pair. It probably makes sense to move those to be common diagnostics instead of adding a new one with a new warning group.

clang/include/clang/Basic/DiagnosticGroups.td

1217

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.

clang/lib/Sema/SemaDeclAttr.cpp

950–961

Oh yeah, derp, that's my mistake. Also, it's moot given that we want to check merging logic anyway (often the handler will call the merge function to generate the semantic attribute to add to the declaration.

[nickdesaulniers]

• reusing existing Diag/Notes, remove test comment, add test for indirect call

clang/include/clang/Basic/AttrDocs.td

6066

What should happen and does LTO change the answer?
But I have no idea if that's actually the behavior.

Well for that input, the TU1.c's declaration is not used and the declaration in TU.c does not match. So it's malformed input IMO, just as if in a non-lto case declarations didn't match between TUs. So that is the actual behavior, but I'm not sure whether documenting what we have for bad input ossifies the behavior? I'd be much more open for adding a test for LTO for well formed input (ie. declarations match).

[nickdesaulniers]

clang/lib/CodeGen/CodeGenAction.cpp

783

One thing I was thinking of to support @arsenm's case:

We probably generally could support diagnosing indirect calls if -g was used to emit DILocation MD nodes. I'd need to change this up from a custom !srcloc node with one unsigned that is a SourceLocation to use the the DILocation tuple of line, column, and scope then change the frontend's BackendDiagnosticConsumer to reinstantiate a SourceLocation from those (if possible).

The only thing is: is it bad if we can only diagnose with -g for indirect calls?

Otherwise I don't see how we can support diagnosing indirect calls. GCC can. We'd perhaps need to emit !srcloc or DILocation nodes for EVERY indirect call from the frontend, and add logic to merge these or something during optimizations when we turn indirect calls into direct calls. And I'm sure that would cause lots of churn in the LLVM source tree / in the tests. I would create a parent revision to precommit that. But I think that's maybe too overkill?

The current behavior is that we don't diagnose indirect calls. I'm happy to add documentation, or create child or parent revisions trying to tackle that, but I think if we can't provide accurate debug info, it's perhaps better not to diagnose at all.

I could change that so that we still emit the diagnostic, but don't show _where_ the callsite was in the sources as part of the diagnostic. But I think it's perhaps better to simply not warn in that case when we can't provide line info.

But this can go either way, so I appreciate others' guidance and thoughts here.

[nickdesaulniers]

clang/include/clang/Basic/Attr.td

3835

I'm having a very difficult time getting inheritable attr's to work correctly.

For example:

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(); }

I can get all Decls of foo to have the attribute (via Inherit in the ast dump), but the first call to foo() doesn't diagnose; at that point we haven't encountered a Decl that has the attribute. So it's hard to match GCC's behavior of warning on all three. Is there something I should be doing differently in CodeGenModule::SetFunctionAttributes to check the "final decl" rather than the current/latest Decl?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3835

We spoke about this a bit off-list and I think we're going to try requiring writing the attribute on the first declaration. I think that might give the most intuitive behavior for this attribute.

clang/include/clang/Basic/AttrDocs.td

6066

So that is the actual behavior, but I'm not sure whether documenting what we have for bad input ossifies the behavior? I'd be much more open for adding a test for LTO for well formed input (ie. declarations match).

The intent isn't so much to ossify the behavior but to ensure we don't do something unacceptable (like crash). We can always add comments to the test to explain that so there's no confusion about trying to define (via tests) what is effectively undefined behavior. However, if you think there's not a lot of value in such a test, I don't insist on adding one.

Having a test for well-formed input is also a very good idea though!

clang/lib/CodeGen/CodeGenAction.cpp

783

Perhaps debug info, but this SourceLocation is squirreled away in a Metadata node attached to the call site.

I thought optimization passes frequently drop metadata nodes?

[nickdesaulniers]

• LTO test, diagnose inheritability

[nickdesaulniers]

clang/include/clang/Basic/Attr.td

3838

Looking at the generated IR for ObjC method calls, I don't think we can support them (easily).

It looks like method calls in ObjC are lowered to direct calls to objc_msg_lookup+indirect CALLs, IIUC. This BackendDiagnostic relies on observing direct calls during instruction selection. (I suspect ObjC supports runtime modification of classes?) So during instruction selection, we can't know that we're making a method call to a particular method (easily; maybe there's helpers for this though?). We don't seem to remove the indirection even with optimizations enabled (making me think that methods can be swapped at runtime).

We could try to diagnose in the frontend, but this very very quickly falls apart if there's any control flow involved. ie. maybe we could diagnose:

void foo(void) { dontcall(); }

but this whole BackendDiagnostic relies on the backend to run optimizations; the frontend couldn't diagnose:

void foo(void) { if (myRuntimeAssertion) dontcall(); }

which is the predominate use case for this fn attr.

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

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

clang/lib/CodeGen/CodeGenAction.cpp

783

I thought optimization passes frequently drop metadata nodes?

Perhaps, but I've yet to see Metadata attached to a static call get dropped. This patch emits such Metadata attached to call IR Instructions when the frontend sees a static call of callee whose Decl has these GNU C fn attrs.

Perhaps when we de-indirect an indirect call to a direct call we don't carry over the Metadata (unsure), but it's moot because the frontend didn't know to attach the dontcall IR Fn attr because the frontend _cant_ de-indirect the call.

So it's not a case that de-indirection in the backend drops the Metadata node (though perhaps that's also an issue), moreso that the frontend didn't know to attach the metadata in the first place because it only saw an indirect call.

We could emit such Metadata for _all_ indirect calls from the frontend (overkill), diagnose only when Debug Info is requested (not great), diagnose but not provide line no of indirect call site (not great), simply not diagnose (not great, but also unlikely IMO for there to be indirect calls of such attributed functions), or maybe something else I'm not thinking of. The current implementation chooses to not diagnose. Is that the best choice? Definitely a clothespin vote. Is there something better? I was hoping you all would tell me. :)

[nickdesaulniers]

clang/include/clang/Basic/AttrDocs.td

6066

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

2196

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

3838

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

6067

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

clang/include/clang/Basic/DiagnosticGroups.td

1217

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

6067

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

1217

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

1217

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

76

78

clang/include/clang/Basic/DiagnosticGroups.td

1217

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.

clang/docs/ReleaseNotes.rst

- Attributes loaded as clang plugins which are sensitive to LangOpts must
   (This was changed in order to better support attributes in code completion).
 
 Windows Support
 ---------------
 
 C Language Changes in Clang
 ---------------------------
 
-- ...
+- Support for ``__attribute__((error("")))`` and
+  ``__attribute__((warning("")))`` function attributes have been added.
 
 C++ Language Changes in Clang
 -----------------------------
 
 - ...
 
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^

clang/include/clang/Basic/Attr.td

 
 def EnforceTCBLeaf : InheritableAttr {
   let Spellings = [Clang<"enforce_tcb_leaf">];
   let Subjects = SubjectList<[Function]>;
   let Args = [StringArgument<"TCBName">];
   let Documentation = [EnforceTCBLeafDocs];
   bit InheritEvenIfAlreadyPresent = 1;
 }
+
+def Error : InheritableAttr {
+  let Spellings = [GCC<"error">, GCC<"warning">];
+  let Accessors = [Accessor<"isError", [GCC<"error">]>,
+                   Accessor<"isWarning", [GCC<"warning">]>];
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];
+}

clang/include/clang/Basic/AttrDocs.td

def EnforceTCBLeafDocs : Documentation {
   let Content = [{
   The ``enforce_tcb_leaf`` attribute satisfies the requirement enforced by
   ``enforce_tcb`` for the marked function to be in the named TCB but does not
   continue to check the functions called from within the leaf function.
 
   - ``enforce_tcb_leaf(Name)`` indicates that this function is a part of the TCB named ``Name``
   }];
 }
+
+def ErrorAttrDocs : Documentation {
+  let Category = DocCatFunction;
+  let Heading = "error, warning";
+  let Content = [{
+The ``error`` and ``warning`` function attributes can be used to specify a
+custom diagnostic to be emitted when a call to such a function is not
+eliminated via optimizations. This can be used to create compile time
+assertions that depend on optimizations, while providing diagnostics
+pointing to precise locations of the call site in the source.
+
+.. code-block:: c++
+
+  __attribute__((warning("oh no"))) void dontcall();
+  void foo() {
+    if (someCompileTimeAssertionThatsTrue)
+      dontcall(); // Warning
+
+    dontcall(); // Warning
+
+    if (someCompileTimeAssertionThatsFalse)
+      dontcall(); // No Warning
+    sizeof(dontcall()); // No Warning
+  }
+  }];
+}

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def warn_fe_backend_optimization_failure : Warning<"%0">, BackendInfo,
     InGroup<BackendOptimizationFailure>, DefaultWarn;
 def note_fe_backend_invalid_loc : Note<"could "
   "not determine the original source location for %0:%1:%2">, BackendInfo;
 
 def err_fe_backend_unsupported : Error<"%0">, BackendInfo;
 def warn_fe_backend_unsupported : Warning<"%0">, BackendInfo;
 
+def err_fe_backend_error_attr :
+  Error<"call to %0 declared with 'error' attribute: %1">, BackendInfo;
+def warn_fe_backend_warning_attr :
+  Warning<"call to %0 declared with 'warning' attribute: %1">, BackendInfo,
+  InGroup<BackendWarningAttributes>;
+
 def err_fe_invalid_code_complete_file : Error<
     "cannot locate code-completion file %0">, DefaultFatal;
 def err_fe_dependency_file_requires_MT : Error<
     "-dependency-file requires at least one -MT or -MQ option">;
 def err_fe_invalid_plugin_name : Error<
     "unable to find plugin '%0'">;
 def err_fe_expected_compiler_job : Error<
     "unable to handle compilation, expected exactly one compiler job in '%0'">;

clang/include/clang/Basic/DiagnosticGroups.td

 // Compatibility flag name from old versions of Clang.
 def : DiagGroup<"frame-larger-than=", [BackendFrameLargerThan]>;
 def BackendPlugin : DiagGroup<"backend-plugin">;
 def RemarkBackendPlugin : DiagGroup<"remark-backend-plugin">;
 def BackendOptimizationRemark : DiagGroup<"pass">;
 def BackendOptimizationRemarkMissed : DiagGroup<"pass-missed">;
 def BackendOptimizationRemarkAnalysis : DiagGroup<"pass-analysis">;
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendWarningAttributes : DiagGroup<"attribute-warning">;
 
 // Instrumentation based profiling warnings.
 def ProfileInstrMissing : DiagGroup<"profile-instr-missing">;
 def ProfileInstrOutOfDate : DiagGroup<"profile-instr-out-of-date">;
 def ProfileInstrUnprofiled : DiagGroup<"profile-instr-unprofiled">;
 
 // AddressSanitizer frontend instrumentation remarks.
 def SanitizeAddressRemarks : DiagGroup<"sanitize-address">;

clang/include/clang/Sema/Sema.h

public:
   UuidAttr *mergeUuidAttr(Decl *D, const AttributeCommonInfo &CI,
                           StringRef UuidAsWritten, MSGuidDecl *GuidDecl);
   DLLImportAttr *mergeDLLImportAttr(Decl *D, const AttributeCommonInfo &CI);
   DLLExportAttr *mergeDLLExportAttr(Decl *D, const AttributeCommonInfo &CI);
   MSInheritanceAttr *mergeMSInheritanceAttr(Decl *D,
                                             const AttributeCommonInfo &CI,
                                             bool BestCase,
                                             MSInheritanceModel Model);
+  ErrorAttr *mergeErrorAttr(Decl *D, const AttributeCommonInfo &CI,
+                            StringRef NewUserDiagnostic);
   FormatAttr *mergeFormatAttr(Decl *D, const AttributeCommonInfo &CI,
                               IdentifierInfo *Format, int FormatIdx,
                               int FirstArg);
   SectionAttr *mergeSectionAttr(Decl *D, const AttributeCommonInfo &CI,
                                 StringRef Name);
   CodeSegAttr *mergeCodeSegAttr(Decl *D, const AttributeCommonInfo &CI,
                                 StringRef Name);
   AlwaysInlineAttr *mergeAlwaysInlineAttr(Decl *D,

clang/lib/CodeGen/CGCall.cpp

else if (IsMustTail)
       Call->setTailCallKind(llvm::CallInst::TCK_MustTail);
   }
 
   // Add metadata for calls to MSAllocator functions
   if (getDebugInfo() && TargetDecl &&
       TargetDecl->hasAttr<MSAllocatorAttr>())
     getDebugInfo()->addHeapAllocSiteMetadata(CI, RetTy->getPointeeType(), Loc);
 
+  // Add metadata if calling an __attribute__((error(""))) or warning fn.
+  if (TargetDecl && TargetDecl->hasAttr<ErrorAttr>()) {
+    llvm::ConstantInt *Line =
+        llvm::ConstantInt::get(Int32Ty, Loc.getRawEncoding());
+    llvm::ConstantAsMetadata *MD = llvm::ConstantAsMetadata::get(Line);
+    llvm::MDTuple *MDT = llvm::MDNode::get(getLLVMContext(), {MD});
+    CI->setMetadata("srcloc", MDT);
+  }
+
   // 4. Finish the call.
 
   // If the call doesn't return, finish the basic block and clear the
   // insertion point; this allows the rest of IRGen to discard
   // unreachable code.
   if (CI->doesNotReturn()) {
     if (UnusedReturnSizePtr)
       PopCleanupBlock();

clang/lib/CodeGen/CodeGenAction.cpp

public:
     void
     OptimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &D);
     void OptimizationRemarkHandler(
         const llvm::OptimizationRemarkAnalysisFPCommute &D);
     void OptimizationRemarkHandler(
         const llvm::OptimizationRemarkAnalysisAliasing &D);
     void OptimizationFailureHandler(
         const llvm::DiagnosticInfoOptimizationFailure &D);
+    void DontCallDiagHandler(const DiagnosticInfoDontCall &D);
   };
 
   void BackendConsumer::anchor() {}
 }
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
EmitOptimizationMessage(
         D, diag::remark_fe_backend_optimization_remark_analysis_aliasing);
 }
 
 void BackendConsumer::OptimizationFailureHandler(
     const llvm::DiagnosticInfoOptimizationFailure &D) {
   EmitOptimizationMessage(D, diag::warn_fe_backend_optimization_failure);
 }
 
+void BackendConsumer::DontCallDiagHandler(const DiagnosticInfoDontCall &D) {
+  if (const Decl *DE = Gen->GetDeclForMangledName(D.getFunctionName()))
+    if (const auto *FD = dyn_cast<FunctionDecl>(DE)) {
+      assert(FD->hasAttr<ErrorAttr>() &&
+             "expected error or warning function attribute");
+
+      if (const auto *EA = FD->getAttr<ErrorAttr>()) {
+        assert((EA->isError() || EA->isWarning()) &&
+               "ErrorAttr neither error or warning");
+
+        SourceLocation LocCookie =
+            SourceLocation::getFromRawEncoding(D.getLocCookie());
+
+        // FIXME: we can't yet diagnose indirect calls. When/if we can, we
+        // should instead assert that LocCookie.isValid().
+        if (!LocCookie.isValid())
+          return;
+
+        Diags.Report(LocCookie, EA->isError()
+                                    ? diag::err_fe_backend_error_attr
+                                    : diag::warn_fe_backend_warning_attr)
+            << FD << EA->getUserDiagnostic();
+      }
+    }
+  // TODO: assert if DE or FD were nullptr?
+}
+
 /// This function is invoked when the backend needs
 /// to report something to the user.
 void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {
   unsigned DiagID = diag::err_fe_inline_asm;
   llvm::DiagnosticSeverity Severity = DI.getSeverity();
   // Get the diagnostic ID based.
   switch (DI.getKind()) {
   case llvm::DK_InlineAsm:
void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {
   case llvm::DK_OptimizationFailure:
     // Optimization failures are always handled completely by this
     // handler.
     OptimizationFailureHandler(cast<DiagnosticInfoOptimizationFailure>(DI));
     return;
   case llvm::DK_Unsupported:
     UnsupportedDiagHandler(cast<DiagnosticInfoUnsupported>(DI));
     return;
+  case llvm::DK_DontCall:
+    DontCallDiagHandler(cast<DiagnosticInfoDontCall>(DI));
+    return;
   default:
     // Plugin IDs are not bound to any value as they are set dynamically.
     ComputeDiagRemarkID(Severity, backend_plugin, DiagID);
     break;
   }
   std::string MsgStorage;
   {
     raw_string_ostream Stream(MsgStorage);

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::SetFunctionAttributes(GlobalDecl GD, llvm::Function *F,
   if (!IsIncompleteFunction && F->isDeclaration())
     getTargetCodeGenInfo().setTargetAttributes(FD, F, *this);
 
   if (const auto *CSA = FD->getAttr<CodeSegAttr>())
     F->setSection(CSA->getName());
   else if (const auto *SA = FD->getAttr<SectionAttr>())
      F->setSection(SA->getName());
 
+  if (FD->hasAttr<ErrorAttr>())
+    F->addFnAttr("dontcall");
+
   // If we plan on emitting this inline builtin, we can't treat it as a builtin.
   if (FD->isInlineBuiltinDeclaration()) {
     const FunctionDecl *FDBody;
     bool HasBody = FD->hasBody(FDBody);
     (void)HasBody;
     assert(HasBody && "Inline builtin declarations should always have an "
                       "available body!");
     if (shouldEmitFunction(FDBody))
if (const auto *CB = FD->getAttr<CallbackAttr>()) {
     ArrayRef<int> PayloadIndices(CB->encoding_begin() + 1, CB->encoding_end());
     F->addMetadata(llvm::LLVMContext::MD_callback,
                    *llvm::MDNode::get(Ctx, {MDB.createCallbackEncoding(
                                                CalleeIdx, PayloadIndices,
                                                /* VarArgsArePassed */ false)}));
   }
 }
 
 void CodeGenModule::addUsedGlobal(llvm::GlobalValue *GV) {
   assert((isa<llvm::Function>(GV) || !GV->isDeclaration()) &&
          "Only globals with definition can force usage.");
   LLVMUsed.emplace_back(GV);
 }
 
 void CodeGenModule::addCompilerUsedGlobal(llvm::GlobalValue *GV) {
   assert(!GV->isDeclaration() &&
          "Only globals with definition can force usage.");

clang/lib/Sema/SemaDecl.cpp

static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
   else if (const auto *VA = dyn_cast<VisibilityAttr>(Attr))
     NewAttr = S.mergeVisibilityAttr(D, *VA, VA->getVisibility());
   else if (const auto *VA = dyn_cast<TypeVisibilityAttr>(Attr))
     NewAttr = S.mergeTypeVisibilityAttr(D, *VA, VA->getVisibility());
   else if (const auto *ImportA = dyn_cast<DLLImportAttr>(Attr))
     NewAttr = S.mergeDLLImportAttr(D, *ImportA);
   else if (const auto *ExportA = dyn_cast<DLLExportAttr>(Attr))
     NewAttr = S.mergeDLLExportAttr(D, *ExportA);
-  else if (const auto *FA = dyn_cast<FormatAttr>(Attr))
+  else if (const auto *EA = dyn_cast<ErrorAttr>(Attr)) {
+    NewAttr = S.mergeErrorAttr(D, *EA, EA->getUserDiagnostic());
+  } else if (const auto *FA = dyn_cast<FormatAttr>(Attr))
     NewAttr = S.mergeFormatAttr(D, *FA, FA->getType(), FA->getFormatIdx(),
                                 FA->getFirstArg());
   else if (const auto *SA = dyn_cast<SectionAttr>(Attr))
     NewAttr = S.mergeSectionAttr(D, *SA, SA->getName());
   else if (const auto *CSA = dyn_cast<CodeSegAttr>(Attr))
     NewAttr = S.mergeCodeSegAttr(D, *CSA, CSA->getName());
   else if (const auto *IA = dyn_cast<MSInheritanceAttr>(Attr))
     NewAttr = S.mergeMSInheritanceAttr(D, *IA, IA->getBestCase(),
if (New->hasAttr<SectionAttr>() && !Old->hasAttr<SectionAttr>()) {
     if (auto *VD = dyn_cast<VarDecl>(New)) {
       if (VD->isThisDeclarationADefinition() == VarDecl::DeclarationOnly) {
         Diag(New->getLocation(), diag::warn_attribute_section_on_redeclaration);
         Diag(Old->getLocation(), diag::note_previous_declaration);
       }
     }
   }
 
   // Redeclaration adds code-seg attribute.
   const auto *NewCSA = New->getAttr<CodeSegAttr>();
   if (NewCSA && !Old->hasAttr<CodeSegAttr>() &&
       !NewCSA->isImplicit() && isa<CXXMethodDecl>(New)) {
     Diag(New->getLocation(), diag::warn_mismatched_section)
          << 0 /*codeseg*/;
     Diag(Old->getLocation(), diag::note_previous_declaration);
   }
 
bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
   if (const auto *ILA = New->getAttr<InternalLinkageAttr>())
     if (!Old->hasAttr<InternalLinkageAttr>()) {
       Diag(New->getLocation(), diag::err_attribute_missing_on_first_decl)
           << ILA;
       Diag(Old->getLocation(), diag::note_previous_declaration);
       New->dropAttr<InternalLinkageAttr>();
     }
 
+  if (auto *EA = New->getAttr<ErrorAttr>()) {
+    if (!Old->hasAttr<ErrorAttr>()) {
+      Diag(EA->getLocation(), diag::err_attribute_missing_on_first_decl) << EA;
+      Diag(Old->getLocation(), diag::note_previous_declaration);
+      New->dropAttr<ErrorAttr>();
+    }
+  }
+
   if (CheckRedeclarationModuleOwnership(New, Old))
     return true;
 
   if (!getLangOpts().CPlusPlus) {
     bool OldOvl = Old->hasAttr<OverloadableAttr>();
     if (OldOvl != New->hasAttr<OverloadableAttr>() && !Old->isImplicit()) {
       Diag(New->getLocation(), diag::err_attribute_overloadable_mismatch)
         << New << OldOvl;

clang/lib/Sema/SemaDeclAttr.cpp

static void handleEnableIfAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   S.Diag(AL.getLoc(), diag::ext_clang_enable_if);
 
   Expr *Cond;
   StringRef Msg;
   if (checkFunctionConditionAttr(S, D, AL, Cond, Msg))
     D->addAttr(::new (S.Context) EnableIfAttr(S.Context, AL, Cond, Msg));
 }
 
+static void handleErrorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef NewUserDiagnostic;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, NewUserDiagnostic))
+    return;
+  if (ErrorAttr *EA = S.mergeErrorAttr(D, AL, NewUserDiagnostic))
+    D->addAttr(EA);
+}
+
 namespace {
 /// Determines if a given Expr references any of the given function's
 /// ParmVarDecls, or the function's implicit `this` parameter (if applicable).
 class ArgumentDependenceChecker
     : public RecursiveASTVisitor<ArgumentDependenceChecker> {
 #ifndef NDEBUG
   const CXXRecordDecl *ClassType;
 #endif
   llvm::SmallPtrSet<const ParmVarDecl *, 16> Parms;
   bool Result;
 
 public:
   ArgumentDependenceChecker(const FunctionDecl *FD) {
 #ifndef NDEBUG
     if (const auto *MD = dyn_cast<CXXMethodDecl>(FD))
       ClassType = MD->getParent();
     else
       ClassType = nullptr;
 #endif
     Parms.insert(FD->param_begin(), FD->param_end());
   }
 
   bool referencesArgs(Expr *E) {
if ((prioritynum < 101 || prioritynum > 65535) &&
     S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_range)
         << E->getSourceRange() << AL << 101 << 65535;
     AL.setInvalid();
     return;
   }
   D->addAttr(::new (S.Context) InitPriorityAttr(S.Context, AL, prioritynum));
 }
 
+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(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI << EA;
+      Diag(EA->getLocation(), 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);
+}
+
 FormatAttr *Sema::mergeFormatAttr(Decl *D, const AttributeCommonInfo &CI,
                                   IdentifierInfo *Format, int FormatIdx,
                                   int FirstArg) {
   // Check whether we already have an equivalent format attribute.
   for (auto *F : D->specific_attrs<FormatAttr>()) {
     if (F->getType() == Format &&
         F->getFormatIdx() == FormatIdx &&
         F->getFirstArg() == FirstArg) {
case ParsedAttr::AT_Deprecated:
     handleDeprecatedAttr(S, D, AL);
     break;
   case ParsedAttr::AT_Destructor:
       handleDestructorAttr(S, D, AL);
     break;
   case ParsedAttr::AT_EnableIf:
     handleEnableIfAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_Error:
+    handleErrorAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_DiagnoseIf:
     handleDiagnoseIfAttr(S, D, AL);
     break;
   case ParsedAttr::AT_NoBuiltin:
     handleNoBuiltinAttr(S, D, AL);
     break;
   case ParsedAttr::AT_ExtVectorType:
     handleExtVectorTypeAttr(S, D, AL);

clang/test/CodeGen/attr-error.c

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+__attribute__((error("oh no"))) void foo(void);
+
+void bar(void) {
+  foo();
+}
+
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare{{.*}} void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
+// CHECK: [[SRCLOC]] = !{i32 {{[0-9]+}}}

clang/test/CodeGen/attr-warning.c

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+__attribute__((warning("oh no"))) void foo(void);
+
+void bar(void) {
+  foo();
+}
+
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare{{.*}} void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
+// CHECK: [[SRCLOC]] = !{i32 {{[0-9]+}}}

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

+// RUN: %clang_cc1 -O2 -verify -emit-codegen-only %s
+
+__attribute__((error("oh no foo"))) void foo(void);
+
+__attribute__((error("oh no bar"))) void bar(void);
+
+int x(void) {
+  return 8 % 2 == 1;
+}
+void baz(void) {
+  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+  if (x())
+    bar();
+}
+
+// FIXME: indirect call detection not yet supported.
+void (*quux)(void);
+
+void indirect(void) {
+  quux = foo;
+  quux();
+}

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

+// RUN: %clang_cc1 -verify=expected,enabled -emit-codegen-only %s
+// RUN: %clang_cc1 -verify=expected,enabled -emit-codegen-only %s -x c++
+// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s
+// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s -x c++
+
+__attribute__((error("oh no foo"))) void foo(void);
+
+__attribute__((error("oh no bar"))) void bar(void);
+
+int x(void) {
+  return 8 % 2 == 1;
+}
+
+__attribute__((warning("oh no quux"))) void quux(void);
+
+__attribute__((error("demangle me"))) void __compiletime_assert_455(void);
+
+__attribute__((error("one"), error("two"))) // expected-warning {{attribute 'error' is already applied with different arguments}}
+void                                        // expected-note@-1 {{previous attribute is here}}
+duplicate_errors(void);
+
+__attribute__((warning("one"), warning("two"))) // expected-warning {{attribute 'warning' is already applied with different arguments}}
+void                                            // expected-note@-1 {{previous attribute is here}}
+duplicate_warnings(void);
+
+void baz(void) {
+  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+  if (x())
+    bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
+
+  quux();                     // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
+  __compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
+  duplicate_errors();         // expected-error {{call to 'duplicate_errors' declared with 'error' attribute: two}}
+  duplicate_warnings();       // enabled-warning {{call to 'duplicate_warnings' declared with 'warning' attribute: two}}
+}
+
+#ifdef __cplusplus
+template <typename T>
+__attribute__((error("demangle me, too")))
+T
+nocall(T t);
+
+struct Widget {
+  __attribute__((warning("don't call me!")))
+  operator int() { return 42; }
+};
+
+void baz_cpp(void) {
+  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+  if (x())
+    bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
+  quux();  // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
+
+  // Test that we demangle correctly in the diagnostic for C++.
+  __compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
+  nocall<int>(42);            // expected-error {{call to 'nocall<int>' declared with 'error' attribute: demangle me, too}}
+
+  Widget W;
+  int w = W; // enabled-warning {{'operator int' declared with 'warning' attribute: don't call me!}}
+}
+#endif

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
 // CHECK-NEXT: DisableTailCalls (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: EnableIf (SubjectMatchRule_function)
 // CHECK-NEXT: EnforceTCB (SubjectMatchRule_function)
 // CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function)
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: Error (SubjectMatchRule_function)
 // CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
 // CHECK-NEXT: HIPManaged (SubjectMatchRule_variable)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)

clang/test/Misc/serialized-diags-driver.c

 // Test that the driver correctly combines its own diagnostics with CC1's in the
 // serialized diagnostics. To test this, we need to trigger diagnostics from
 // both processes, so we compile code that has a warning (with an associated
 // note) and then force the driver to crash. We compile stdin so that the crash
 // doesn't litter the user's system with preprocessed output.
 
 // RUN: rm -f %t
 // RUN: %clang -Wx-typoed-warning -Wall -fsyntax-only --serialize-diagnostics %t.diag %s
 // RUN: c-index-test -read-diagnostics %t.diag 2>&1 | FileCheck %s
 
-// CHECK: warning: unknown warning option '-Wx-typoed-warning' [-Wunknown-warning-option] []
+// CHECK: warning: unknown warning option '-Wx-typoed-warning'; did you mean '-Wattribute-warning'? [-Wunknown-warning-option] []
 
 // CHECK: warning: variable 'voodoo' is uninitialized when used here [-Wuninitialized]
 // CHECK: note: initialize the variable 'voodoo' to silence this warning []
 // CHECK: Number of diagnostics: 2
 
 void foo() {
   int voodoo;
   voodoo = voodoo + 1;
 }

clang/test/Sema/attr-error.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+#if !__has_attribute(error)
+#error "error attribute missing"
+#endif
+
+__attribute__((error("don't call me!"))) int good0(void);
+
+__attribute__((error)) // expected-error {{'error' attribute takes one argument}}
+int
+bad0(void);
+
+int bad1(__attribute__((error("bad1"))) int param); // expected-error {{'error' attribute only applies to functions}}
+
+int bad2(void) {
+  __attribute__((error("bad2"))); // expected-error {{'error' attribute cannot be applied to a statement}}
+}
+
+__attribute__((error(3))) // expected-error {{'error' attribute requires a string}}
+int
+bad3(void);
+
+__attribute__((error("foo"), error("foo"))) int good1(void);
+__attribute__((error("foo"))) int good1(void);
+__attribute__((error("foo"))) int good1(void) {}
+
+__attribute__((error("foo"), warning("foo"))) // expected-error {{'warning' and 'error' attributes are not compatible}}
+int
+bad4(void);
+// expected-note@-3 {{conflicting attribute is here}}
+
+__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}}
+
+/*
+ * Note: we differ from GCC here; rather than support redeclarations that add
+ * or remove this fn attr, we diagnose such differences.
+ */
+
+void foo(void);                                     // expected-note {{previous declaration is here}}
+__attribute__((error("oh no foo"))) void foo(void); // expected-error {{'error' attribute does not appear on the first declaration}}

clang/test/Sema/attr-warning.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+#if !__has_attribute(warning)
+#warning "warning attribute missing"
+#endif
+
+__attribute__((warning("don't call me!"))) int good0(void);
+
+__attribute__((warning)) // expected-error {{'warning' attribute takes one argument}}
+int
+bad0(void);
+
+int bad1(__attribute__((warning("bad1"))) int param); // expected-error {{'warning' attribute only applies to functions}}
+
+int bad2(void) {
+  __attribute__((warning("bad2"))); // expected-error {{'warning' attribute cannot be applied to a statement}}
+}
+
+__attribute__((warning(3))) // expected-error {{'warning' attribute requires a string}}
+int
+bad3(void);
+
+__attribute__((warning("foo"), warning("foo"))) int good1(void);
+__attribute__((warning("foo"))) int good1(void);
+__attribute__((warning("foo"))) int good1(void) {}
+
+__attribute__((warning("foo"), error("foo"))) // expected-error {{'error' and 'warning' attributes are not compatible}}
+int
+bad4(void);
+// expected-note@-3 {{conflicting attribute is here}}
+
+/*
+ * Note: we differ from GCC here; rather than support redeclarations that add
+ * or remove this fn attr, we diagnose such differences.
+ */
+
+void foo(void);                                       // expected-note {{previous declaration is here}}
+__attribute__((warning("oh no foo"))) void foo(void); // expected-error {{'warning' attribute does not appear on the first declaration}}

llvm/docs/LangRef.rst

``convergent``
     that we should treat the call as though we're calling a convergent
     function.  This is particularly useful on indirect calls; without this we
     may treat such calls as though the target is non-convergent.
 
     The optimizer may remove the ``convergent`` attribute on functions when it
     can prove that the function does not execute any convergent operations.
     Similarly, the optimizer may remove ``convergent`` on calls/invokes when it
     can prove that the call/invoke cannot call a convergent function.
+``"dontcall"``
+    This attribute denotes that a diagnostic should be emitted when a call of a
+    function with this attribute is not eliminated via optimization. Front ends
+    can provide optional ``srcloc`` metadata nodes on call sites of such
+    callees to attach information about where in the source language such a
+    call came from.
 ``"frame-pointer"``
     This attribute tells the code generator whether the function
     should keep the frame pointer. The code generator may emit the frame pointer
     even if this attribute says the frame pointer can be eliminated.
     The allowed string values are:
 
      * ``"none"`` (default) - the frame pointer can be eliminated.
      * ``"non-leaf"`` - the frame pointer should be kept if the function calls

llvm/include/llvm/IR/DiagnosticInfo.h

enum DiagnosticKind {
   DK_MachineOptimizationRemarkMissed,
   DK_MachineOptimizationRemarkAnalysis,
   DK_FirstMachineRemark = DK_MachineOptimizationRemark,
   DK_LastMachineRemark = DK_MachineOptimizationRemarkAnalysis,
   DK_MIRParser,
   DK_PGOProfile,
   DK_Unsupported,
   DK_SrcMgr,
+  DK_DontCall,
   DK_FirstPluginKind // Must be last value to work with
                      // getNextAvailablePluginDiagnosticKind
 };
 
 /// Get the next available kind ID for a plugin diagnostic.
 /// Each time this function is called, it returns a different number.
 /// Therefore, a plugin that wants to "identify" its own classes
 /// with a dynamic identifier, just have to use this method to get a new ID
public:
   unsigned getLocCookie() const { return LocCookie; }
   void print(DiagnosticPrinter &DP) const override;
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_SrcMgr;
   }
 };
 
+class DiagnosticInfoDontCall : public DiagnosticInfo {
+  StringRef CalleeName;
+  unsigned LocCookie;
+
+public:
+  DiagnosticInfoDontCall(StringRef CalleeName, unsigned LocCookie)
+      : DiagnosticInfo(DK_DontCall, DS_Error), CalleeName(CalleeName),
+        LocCookie(LocCookie) {}
+  StringRef getFunctionName() const { return CalleeName; }
+  unsigned getLocCookie() const { return LocCookie; }
+  void print(DiagnosticPrinter &DP) const override;
+  static bool classof(const DiagnosticInfo *DI) {
+    return DI->getKind() == DK_DontCall;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_IR_DIAGNOSTICINFO_H

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
bool IRTranslator::translateCall(const User &U, MachineIRBuilder &MIRBuilder) {
 
   // FIXME: support control flow guard targets.
   if (CI.countOperandBundlesOfType(LLVMContext::OB_cfguardtarget))
     return false;
 
   if (CI.isInlineAsm())
     return translateInlineAsm(CI, MIRBuilder);
 
+  if (F && F->hasFnAttribute("dontcall")) {
+    unsigned LocCookie = 0;
+    if (MDNode *MD = CI.getMetadata("srcloc"))
+      LocCookie =
+          mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
+    DiagnosticInfoDontCall D(F->getName(), LocCookie);
+    F->getContext().diagnose(D);
+  }
+
   Intrinsic::ID ID = Intrinsic::not_intrinsic;
   if (F && F->isIntrinsic()) {
     ID = F->getIntrinsicID();
     if (TII && ID == Intrinsic::not_intrinsic)
       ID = static_cast<Intrinsic::ID>(TII->getIntrinsicID(F));
   }
 
   if (!F || !F->isIntrinsic() || ID == Intrinsic::not_intrinsic)

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
if (IsTailCall && MF->getFunction()
                             .getFnAttribute("disable-tail-calls")
                             .getValueAsBool())
     IsTailCall = false;
 
   CallLoweringInfo CLI;
   CLI.setCallee(RetTy, FuncTy, CI->getCalledOperand(), std::move(Args), *CI)
       .setTailCall(IsTailCall);
 
+  if (const Function *F = CI->getCalledFunction())
+    if (F->hasFnAttribute("dontcall")) {
+      unsigned LocCookie = 0;
+      if (MDNode *MD = CI->getMetadata("srcloc"))
+        LocCookie =
+            mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
+      DiagnosticInfoDontCall D(F->getName(), LocCookie);
+      F->getContext().diagnose(D);
+    }
+
   return lowerCallTo(CLI);
 }
 
 bool FastISel::selectCall(const User *I) {
   const CallInst *Call = cast<CallInst>(I);
 
   // Handle simple inline asms.
   if (const InlineAsm *IA = dyn_cast<InlineAsm>(Call->getCalledOperand())) {

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 #include "llvm/IR/CFG.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/IntrinsicsAArch64.h"

 void SelectionDAGBuilder::visitCall(const CallInst &I) {
   // Handle inline assembly differently.
   if (I.isInlineAsm()) {
     visitInlineAsm(I);
     return;
   }
 
   if (Function *F = I.getCalledFunction()) {
+    if (F->hasFnAttribute("dontcall")) {
+      unsigned LocCookie = 0;
+      if (MDNode *MD = I.getMetadata("srcloc"))
+        LocCookie =
+            mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
+      DiagnosticInfoDontCall D(F->getName(), LocCookie);
+      DAG.getContext()->diagnose(D);
+    }
+
     if (F->isDeclaration()) {
       // Is this an LLVM intrinsic or a target-specific intrinsic?
       unsigned IID = F->getIntrinsicID();
       if (!IID)
         if (const TargetIntrinsicInfo *II = TM.getIntrinsicInfo())
           IID = II->getIntrinsicID(F);
 
       if (IID) {

llvm/lib/IR/DiagnosticInfo.cpp

for (const DiagnosticInfoOptimizationBase::Argument &Arg :
                                     ? Args.end()
                                     : Args.begin() + FirstExtraArgIndex))
     OS << Arg.Val;
   return OS.str();
 }
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}

llvm/test/CodeGen/X86/attr-dontcall.ll

+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"

llvm/test/ThinLTO/X86/dontcall.ll

+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}