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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030