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

[nickdesaulniers]

Summary

Due to inlining, descovering which specific call site to a function with
the attribute "warning" or "error" is painful.

In the IR record inlining decisions in metadata when inlining a callee
that itself contains a call to a dontcall-error or dontcall-warn fn.

Print this info so that it's clearer which call site is problematic.

There's still some limitations with this approach; macro expansion is
not recorded.

Fixes: https://github.com/ClangBuiltLinux/linux/issues/1571

[nickdesaulniers]

• update optimize test

[nickdesaulniers]

• use std::string, demangle C++

[nickdesaulniers]

• update tests

[nickdesaulniers]

• remove test comment in commit message

[nickdesaulniers]

• add tests for alwaysinline

[arsenm]

llvm/lib/Transforms/Utils/InlineFunction.cpp

2467–2468

Misses constexpr casts and aliases

2471

metadata usually uses . word separators

llvm/test/Transforms/Inline/dontcall-attributes.ll

9

test alias case?

[nickdesaulniers]

• replace inlined-from w/ inlined.from

[nickdesaulniers]

• mention inlined.from in LangRef

[nickdesaulniers]

llvm/lib/Transforms/Utils/InlineFunction.cpp

2467–2468

The base feature doesn't work with aliases (or ConstantExpr), in GCC or Clang. I should perhaps fix that first...

[dblaikie]

hmm, did we/do we ever motivate backend diagnostics using the debug info locations? I thought we did in some cases/ways - maybe when the frontend isn't available (eg: when compiling from LLVM IR).

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?

[efriedma]

There are two different approaches we use for getting source locations for diagnostics coming out of the backend; either embedding clang SourceLocations into IR, or trying to translate DWARF locations. The advantage of using clang SourceLocations is that we never lose any fidelity, even in complex cases involving the preprocessor, and the compiler doesn't have to spend time generating complete DWARF info. The advantage of DWARF, of course, is that it's existing, mature infrastructure.

We use clang SourceLocations for inline asm and for the warning/error attributes, we use DWARF for optimization remarks.

There's probably some argument for using DWARF here.

---

Am I reading the correctly, that currently the inlining notes don't have source locations? That's not very friendly (particularly for display in IDEs).

[nickdesaulniers]

In D141451#4045414, @efriedma wrote:

There are two different approaches we use for getting source locations for diagnostics coming out of the backend; either embedding clang SourceLocations into IR, or trying to translate DWARF locations. The advantage of using clang SourceLocations is that we never lose any fidelity, even in complex cases involving the preprocessor, and the compiler doesn't have to spend time generating complete DWARF info. The advantage of DWARF, of course, is that it's existing, mature infrastructure.

We use clang SourceLocations for inline asm and for the warning/error attributes, we use DWARF for optimization remarks.

Yep.

There's probably some argument for using DWARF here.

Yeah, my thoughts too. Though if we don't compile with -g, does that mean if I use DILocation (or w/e metadata for "inlining occurred") that .debug_info will get emitted, even if the user didn't ask for it? If not, I can probably switch everything to use DILocation.

---

Am I reading the correctly, that currently the inlining notes don't have source locations? That's not very friendly (particularly for display in IDEs).

Correct; we pessimistically don't record the srcloc of every call ever. We'd need to do that to support line info on the inlining notes. That seems very expensive.

Perhaps we only show these notes if debug info is enabled (-g) where we can then use DiagnosticInfoWithLocationBase? (Do we have precendent for warnings that are better with DebugInfo enabled?)

That said, the information provided is A MASSIVE LEAP FORWARD in debugging these errors (mostly observed in FORTIFY_SOURCE implementations in the Linux kernel) by simply providing the call chain, even without line/col numbers.

[efriedma]

Yeah, my thoughts too. Though if we don't compile with -g, does that mean if I use DILocation (or w/e metadata for "inlining occurred") that .debug_info will get emitted, even if the user didn't ask for it? If not, I can probably switch everything to use DILocation.

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.

[nickdesaulniers]

In 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 ([e-mail]: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?

---

In 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. The DISubprogram and DILocation should form a similar chain, even if significantly more complicated to "unwind."

llvm/lib/Transforms/Utils/InlineFunction.cpp

2467–2468

Perhaps I should use Call.getCalledOperand()->stripPointerCasts() for constantexpr case.

llvm/test/Transforms/Inline/dontcall-attributes.ll

9

I actually disagree about aliases. I don't think aliases should implicitly inherit function attributes.

[dblaikie]

In D141451#4063151, @nickdesaulniers wrote:

In 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 ([e-mail]: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?

---

In 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."

[nickdesaulniers]

In D141451#4063335, @dblaikie wrote:

In D141451#4063151, @nickdesaulniers wrote:

In 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 ([e-mail]: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. 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).

---

In 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.

llvm/lib/Transforms/Utils/InlineFunction.cpp

2467–2468

I've added support for ConstantExpr to the base feature in D142058. I should still fix this in this PR and add a test case, so not yet marking this thread as done.

[dblaikie]

In D141451#4063504, @nickdesaulniers wrote:

In D141451#4063335, @dblaikie wrote:

In D141451#4063151, @nickdesaulniers wrote:

In 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 ([e-mail]: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, @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?

[nickdesaulniers]

In D141451#4063519, @dblaikie wrote:

In D141451#4063504, @nickdesaulniers wrote:

In D141451#4063335, @dblaikie wrote:

In D141451#4063151, @nickdesaulniers wrote:

In 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 ([e-mail]: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 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

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.

[dblaikie]

In D141451#4063582, @nickdesaulniers wrote:

In D141451#4063519, @dblaikie wrote:

In D141451#4063504, @nickdesaulniers wrote:

In D141451#4063335, @dblaikie wrote:

In D141451#4063151, @nickdesaulniers wrote:

In 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 ([e-mail]: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 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.

[nickdesaulniers]

In D141451#4064298, @dblaikie wrote:

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.
Again, might be worth knowing what the cost of the debug info metadata loc tracking mode is.

For a first order approximation, I have this diff applied:

diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 436226c6f178..6d5049803188 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -386,7 +386,7 @@ VALUE_CODEGENOPT(SmallDataLimit, 32, 0)
 VALUE_CODEGENOPT(SSPBufferSize, 32, 0)
 
 /// The kind of generated debug info.
-ENUM_CODEGENOPT(DebugInfo, codegenoptions::DebugInfoKind, 4, codegenoptions::NoDebugInfo)
+ENUM_CODEGENOPT(DebugInfo, codegenoptions::DebugInfoKind, 4, codegenoptions::LocTrackingOnly)
 
 /// Whether to generate macro debug info.
 CODEGENOPT(MacroDebugInfo, 1, 0)

This changes the default value of codegenopts.DebugInfo from codegenoptions::NoDebugInfo to codegenoptions::LocTrackingOnly, which is what the optimization remark emitter infra uses. This emits more debug info than we need (metadata for every statement in IR rather than JUST call instructions). But it would give me analogous metadata I could use to solve this problem addressed by this patch, and it would guarantee that I had precise col/line info.

Comparing 30 Linux kernel x86_64 defconfig (i.e. no debug info) builds with vs without that change:

Without (baseline):

$ hyperfine --prepare 'make LLVM=1 -j128 -s clean' --runs 30 'make LLVM=1 -j128 -s'
Benchmark 1: make LLVM=1 -j128 -s
  Time (mean ± σ):     61.592 s ±  0.156 s    [User: 4378.360 s, System: 312.040 s]
  Range (min … max):   61.283 s … 62.026 s    30 runs

With diff from above:

$ hyperfine --prepare 'make LLVM=1 -j128 -s clean' --runs 30 'make LLVM=1 -j128 -s'  
Benchmark 1: make LLVM=1 -j128 -s
  Time (mean ± σ):     62.228 s ±  0.523 s    [User: 4433.828 s, System: 312.908 s]
  Range (min … max):   61.825 s … 64.912 s    30 runs

So that's a slowdown from the mean of ~1.02% ((1 - 61.592/62.228)*100). We probably could claw some of that back if we had another level of codegenoptions between NoDebugInfo and LocTrackingOnly that only emitted LocTracking info for call insts (stated another way, omit DILocation for all instructions other than call). I'm guessing that would take significant work to add to clang; I wasn't able how to figure out how to do so quickly. I imagine updating the non-debug-info clang tests to also be a treat. Is a 1% compile time performance hit worth more precise backend diagnostics? Probably (IMO). Perhaps worth an RFC?

---

FWIW, here are measurements of D141451 @ Diff 488371 at the same base as the baseline:

$ hyperfine --prepare 'make LLVM=1 -j128 -s clean' --runs 30 'make LLVM=1 -j128 -s'
Benchmark 1: make LLVM=1 -j128 -s
  Time (mean ± σ):     61.702 s ±  0.456 s    [User: 4395.106 s, System: 313.120 s]
  Range (min … max):   61.409 s … 64.000 s    30 runs

That's ~0.17% slower (and my machine is running hot from benchmarking for the past 1.5hrs). Note this patch/approach doesn't have precise line info.

[dblaikie]

In D141451#4095303, @nickdesaulniers wrote:

In D141451#4064298, @dblaikie wrote:

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.
Again, might be worth knowing what the cost of the debug info metadata loc tracking mode is.

For a first order approximation, I have this diff applied:

diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 436226c6f178..6d5049803188 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -386,7 +386,7 @@ VALUE_CODEGENOPT(SmallDataLimit, 32, 0)
 VALUE_CODEGENOPT(SSPBufferSize, 32, 0)
 
 /// The kind of generated debug info.
-ENUM_CODEGENOPT(DebugInfo, codegenoptions::DebugInfoKind, 4, codegenoptions::NoDebugInfo)
+ENUM_CODEGENOPT(DebugInfo, codegenoptions::DebugInfoKind, 4, codegenoptions::LocTrackingOnly)
 
 /// Whether to generate macro debug info.
 CODEGENOPT(MacroDebugInfo, 1, 0)

This changes the default value of codegenopts.DebugInfo from codegenoptions::NoDebugInfo to codegenoptions::LocTrackingOnly, which is what the optimization remark emitter infra uses. This emits more debug info than we need (metadata for every statement in IR rather than JUST call instructions). But it would give me analogous metadata I could use to solve this problem addressed by this patch, and it would guarantee that I had precise col/line info.

Comparing 30 Linux kernel x86_64 defconfig (i.e. no debug info) builds with vs without that change:

Without (baseline):

$ hyperfine --prepare 'make LLVM=1 -j128 -s clean' --runs 30 'make LLVM=1 -j128 -s'
Benchmark 1: make LLVM=1 -j128 -s
  Time (mean ± σ):     61.592 s ±  0.156 s    [User: 4378.360 s, System: 312.040 s]
  Range (min … max):   61.283 s … 62.026 s    30 runs

With diff from above:

$ hyperfine --prepare 'make LLVM=1 -j128 -s clean' --runs 30 'make LLVM=1 -j128 -s'  
Benchmark 1: make LLVM=1 -j128 -s
  Time (mean ± σ):     62.228 s ±  0.523 s    [User: 4433.828 s, System: 312.908 s]
  Range (min … max):   61.825 s … 64.912 s    30 runs

So that's a slowdown from the mean of ~1.02% ((1 - 61.592/62.228)*100). We probably could claw some of that back if we had another level of codegenoptions between NoDebugInfo and LocTrackingOnly that only emitted LocTracking info for call insts (stated another way, omit DILocation for all instructions other than call). I'm guessing that would take significant work to add to clang;

I don't think that should be too costly/is probably worthwhile if the cost is linear on the number of instructions we have to give locations & calls are probably a minority of instructions overall.

This level would be in CodeGenOptions, but wouldn't have to be rendered into the LLVM DICompileUnit level - since it'd still be the same "there's some debug info here but it's not for emitting DWARF" as far as LLVM was concerned.

I wasn't able how to figure out how to do so quickly. I imagine updating the non-debug-info clang tests to also be a treat.

Hopefully non-debug-info tests won't generally care about a bit of extra metadata, but no doubt some'll need updating.

Is a 1% compile time performance hit worth more precise backend diagnostics? Probably (IMO). Perhaps worth an RFC?

Probably

I guess my only other question (worth mentioning in the RFC, perhaps)/direction would be that any of these "things that are expensive but make backend diagnostics better" could be to put them under a different warning flag (if making -Wattribute-{warning|error} off by default isn't feasible - leaving them on-by-default but less than great user experience when optimizing code, and have the warning suggest enabling a different warning flag that's more verbose but off by default and also enables the extra location tracking - so codebases that generally do use these attributes can opt in to that, while codebases that generally don't do that don't entirely lose/ignore the attributes).

[nickdesaulniers]

In D141451#4100432, @dblaikie wrote:

I guess my only other question (worth mentioning in the RFC, perhaps)/direction would be that any of these "things that are expensive but make backend diagnostics better" could be to put them under a different warning flag (if making -Wattribute-{warning|error} off by default isn't feasible - leaving them on-by-default but less than great user experience when optimizing code, and have the warning suggest enabling a different warning flag that's more verbose but off by default and also enables the extra location tracking - so codebases that generally do use these attributes can opt in to that, while codebases that generally don't do that don't entirely lose/ignore the attributes).

I don't think it's worth it to try to make these "zero cost." There are two function attributes here: __attribute__((error(""))) and __attribute__((warning(""))). They are basically implemented precisely the same way except how diagnostics are finally emitted by the frontend:

For __attribute__((error(""))), we always emit an error. There is no -Wno-error=XXX or -Wno-XXX flag to change that. (i.e. no such -Wattribute-error)

For __attribute__((warning(""))), we emit a warning by default, an error if -Werror=attribute-warning is set, and nothing if -Wno-attribute-warning is set (unlikely).

Since -Wno-attribute-warning is unlikely to ever be set, it's not worth making that case "zero cost."

---

Will work on that RFC though!

[nickdesaulniers]

In D141451#4179674, @nickdesaulniers wrote:

Will work on that RFC though!

RFC (PTAL): https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261