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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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?
> > 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.
> 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 `Decl`s 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`?
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.


================
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.
+  }];
----------------
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!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030