[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: JDevlieghere, aprantl.
dblaikie added a comment.

In D141451#4063582 <https://reviews.llvm.org/D141451#4063582>, @nickdesaulniers wrote:

> In D141451#4063519 <https://reviews.llvm.org/D141451#4063519>, @dblaikie wrote:
>
>> In D141451#4063504 <https://reviews.llvm.org/D141451#4063504>, @nickdesaulniers wrote:
>>
>>> In D141451#4063335 <https://reviews.llvm.org/D141451#4063335>, @dblaikie wrote:
>>>
>>>> In D141451#4063151 <https://reviews.llvm.org/D141451#4063151>, @nickdesaulniers wrote:
>>>>
>>>>> In D141451#4045658 <https://reviews.llvm.org/D141451#4045658>, @efriedma wrote:
>>>>>
>>>>>> clang has a "LocTrackingOnly" setting for debug info, which emits DILocation info into the IR, but emits a marker into the DICompileUnit to skip emitting the .debug_info in the backend.  We currently use it for -Rpass.  We don't do this by default, I think to save compile time.
>>>>>
>>>>> Specifically `emissionKind: NoDebug`, example:
>>>>>
>>>>> `!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 16.0.0 (git at github.com:llvm/llvm-project.git 7b433e026498cf4176931b2407baece1d5060e16)", isOptimized: true, runtimeVersion: 0, emissionKind: NoDebug, splitDebugInlining: false, nameTableKind: None)`
>>>>>
>>>>> Though should the frontend be setting codegen options when parsing? Would the idea be to try to re-set `OPT_debug_info_kind_EQ` when clang codegen's IR for a function with such an attribute?
>>>>
>>>> Probably turn on `emissionKind: NoDebug` whenever the warning is enabled?
>>>
>>> So this warning is default enabled, IIRC.
>>
>> Then it might be a broader question about whether this extra info is acceptable to turn on by default, and if not, maybe an extra flag would be needed to say "give me extra info in this diagnostic", basically (or a separate warning flag that's off by default) - some perf metrics might help indicate whether the extra info is cheap enough.
>
> The implementation as it stands is zero cost in the sense that if `__attribute__((warning("")))` or `__attribute__((error("")))` aren't used in the source code, there is ZERO cost in IR.  Additional metadata is only created when inlining occurs (which might not happen if optimizations weren't enabled) where callsites to such functions exist (unlikely), and if the call site is optimized away, the metadata should be deleted (at least, I imagine that's how metadata works in LLVM.  I should actually verify that's the case, then probably add such verification as a test to this patch, perhaps.
>
>> I'm still a bit confused/not following about the "Let me see if I can create DILocation without line or column values." - why do you want to create that?
>
> You're the one that asked <https://reviews.llvm.org/D141451#4045214> if if I can reuse existing metadata nodes, specifically:
>
>>> It'd be nice not to invent a new way of tracking inlining separate from the way debug info does this

Right - I was thinking more, as above, about directly using the existing metadata generation (if it's too expensive to enable by default, then possibly under an off-by-default warning or other flag) that the inliner already knows how to read and write, rather than creating new/different metadata handling.

Perhaps there's already logic for enabling this for remarks (CompilerInvocation.cpp:~2019).

Though it's possibly a path forward to generate new DILocations in the absence of any - it's not the top of my list reach for here, but that's just my 2c. Perhaps other folks have other ideas.

It's possible that this approach might be able to fill a gap we sometimes have in debug info, though - where a call site lacks a DILocation and that makes creating the inline debug info difficult/impossible at the moment (there's a whole long history of this issue, the assertions and verifier checks we've implemented to detect it, and the bugs (frontend missing opportunities to emit useful DILocations) it has found/lead to being fixed, though - so I'm somewhat hesitant to make us more permissive on constructing debug info inlining for call sites without DILocations). (@aprantl @JDevlieghere perhaps you folks have thoughts on this?)

> So I could emit DILocation and DISubprogram metadata rather than the custom metadata nodes as I'm doing in this diff.  I just wouldn't be able to synthesize line or column info for the callsites, since the frontend may not have ever produced it in the first place.  Such an approach would be emitting `DILocation`+`DISubprogram` from the backend, not the frontend; the backend doesn't have line+column info unless debugging was enabled in the first place (which it probably wasn't).
>
> I don't want to pessimistically emit these always from the frontend with precise line+column info. That would be so pessimistic for the common case where you probably never encounter functions with such attributes in a given TU.

It's an awkward situation, to be sure.

Looks like GCC does include the line number in its backtrace? So maybe that's a justification for us to do so as well. Again, might be worth knowing what the cost of the debug info metadata loc tracking mode is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141451