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

Wed Jan 18 14:18:54 PST 2023

dblaikie added a comment.

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.

> I guess I need to check for `-Wno-attribute-warning`.  If `-Wno-attribute-warning` is //not// set and `-g` (or friends) is not set, then I should set `emissionKind` to `NoDebug` (I think).

Generally you shouldn't be checking the raw flags like this for a feature like this - I'd expect something in Clang that checks to see if the warning is enabled, rather than whether certain flag spellings are used, etc. Though what goes in the driver V frontend, etc complicates things (if the decision is made in the frontend, once warning flags are parsed and there's a diagnostic engine you can query for warning enablement, then it'd need to override the debug info level rather than the command line handling in the driver)

>>> ---
>>>
>>> In D141451#4045214 <https://reviews.llvm.org/D141451#4045214>, @dblaikie wrote:
>>>
>>>> It'd be nice not to invent a new way of tracking inlining separate from the way debug info does this - duplicate systems with separate opportunities for bugs, etc. Any chance we can reuse the debug info inlining descriptions for this?
>>>
>>> So it looks like we have:
>>>
>>> `!28 = !DILocation(line: 14, column: 3, scope: !8, inlinedAt: !29)`
>>>
>>> Let me see if I can create DILocation without line or column values.
>>
>> Not sure I follow - why would you want to drop line/column info? Isn't that relevant to the inlining stack - you might have multiple calls in the same function & it'd be good to know which one the diagnostic is referring to.
>>
>>>   The DISubprogram and DILocation should form a similar chain, even if significantly more complicated to "unwind."
>
> Not dropping it, more like without `-g` (or friends) it never existed in the first place.  If you look at my change to `llvm/lib/Transforms/Utils/InlineFunction.cpp` (in this patch), I'm basically synthesizing metadata during inlining.  If we don't have Debug Info related metadata because the program wasn't compiled with `-g` (or w/e), then the DILocation for the callsites was never produced by the frontend in the first place.  This patch doesn't intentionally drop anything, it's more like the anything might never have existed in the first place.
>
> So this warning could "be improved" if you recompiled with `-g`; I don't think there's really precedent for that and expect it's perhaps controversial.  Hence my custom metadate nodes rather than the existing DILocation.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141451