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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030