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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
