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

Tue Aug 3 15:05:09 PDT 2021

nickdesaulniers planned changes to this revision.
nickdesaulniers added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > ObjC methods as well?
> > 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?
> 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.
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.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+    depend on optimizations, while providing diagnostics pointing to precise
+    locations of the call site in the source.
+  }];
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > 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?
> > > > > 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?
> > > > 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.
> > > 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).
> > 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!
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?

================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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?
> > > > > 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.
> > > > 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.
> > > 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.
> > 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.
> > 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?
> 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. :)

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2193
+  // TODO: move to CodeGenModule::ConstructAttributeList() or
+  // CodeGenFunction::EmitCall() ???
+  if (FD->hasAttr<ErrorAttr>())
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030