1.5% bin size increase and 1.8% dwp file size increase after #166202

[asmok-g]

We noticed a 1.5% bin size increase and 1.8 dwp file size increase after #166202 in clang when building internally in Google after #166202. We also noticed different increases in samples we run around the same size. I don't think this is a small change and the PR mentions a way to reduce the increase. Given the increase, I think we should implement those suggestions.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: None (asmok-g)

We noticed a 1.5% bin size increase and 1.8 dwp file size increase after #166202 in clang when building internally in Google after #166202. We also noticed different increases in samples we run around the same size. I don't think this is a small change and the PR mentions a way to reduce the increase. Given the increase, I think we should implement those suggestions.

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: None (asmok-g)

We noticed a 1.5% bin size increase and 1.8 dwp file size increase after #166202 in clang when building internally in Google after #166202. We also noticed different increases in samples we run around the same size. I don't think this is a small change and the PR mentions a way to reduce the increase. Given the increase, I think we should implement those suggestions.

[jryans]

Thanks for opening this issue. I'll try to nudge further discussion over here after carrying over the points already made:

@jryans said:

A bit more of an increase than #166202 (comment) above, but perhaps further optimisations, LTO modes, etc. are revealing more opportunities to insert this kind of debug info.

Since this is effectively fixing a debug correctness issue, we would want to retain the core fix here.

@OCHyams said:

our (Sony) debugger, which wants to see call site info even in cases without call site parameters (it's not only using it for entry_value computation).

Does google use call site information (does it care about entry_values, or do any extra stuff with the info)? If not, I wonder if it would be useful to add a flag to disable call site info for your builds (if such a flag doesn't already exist)?

@alexfh said:

@dwblaikie or @labath should know the answer [of whether Google uses call site info].

I think regardless of the debug information completeness improvement, the 1.5%+ increase may be problematic in some cases. We probably need a flag to mitigate this increase without disabling debug information altogether. If there's already a flag that controls the information being added by this change (or a reasonable superset of it), we may be able to suggest it as a workaround for teams who have to squeeze their binaries into hard size limits.

[OCHyams]

We probably need a flag to mitigate this increase without disabling debug information altogether. If there's already a flag that controls the information being added by this change (or a reasonable superset of it), we may be able to suggest it as a workaround for teams who have to squeeze their binaries into hard size limits.

There's EmitCallSiteInfo ("-emit-call-site-info" =<bool>) in llvm/lib/CodeGen/CommandFlags.cpp, but I'm not sure that's exposed from Clang (at least, I can't convince it to accept the argument with various -mllvm, -Xclang, etc, but could be missing something - not super familiar with commands in that file).

[jryans]

Given at least some consumers have a use for call site info even without entry value or tail calls, I would prefer to keep that call site info available and correct for them by default.

If Google does not make use of the call site info and a flag to control it would be sufficient, I'm happy to take on the task adding / exposing a (default enabled) flag that can be used to disable call site emission for these binary-size-constrained environments.

[rupprecht]

Does google use call site information

Broadly speaking, better debug info is always a good thing. As for whether we specifically care about call site information -- I have no idea how to answer that. e.g. if we drop call site information and it makes debugging in LLDB difficult/super confusing, we probably do care.

There's EmitCallSiteInfo ("-emit-call-site-info" =<bool>) in llvm/lib/CodeGen/CommandFlags.cpp, but I'm not sure that's exposed from Clang (at least, I can't convince it to accept the argument with various -mllvm, -Xclang, etc, but could be missing something - not super familiar with commands in that file).

Also not familiar w/ flags in that file, but from usage in LLVM tests I can only find it passed to llc, so it's probably a flag specific to that tool.

Given at least some consumers have a use for call site info even without entry value or tail calls, I would prefer to keep that call site info available and correct for them by default.

If Google does not make use of the call site info and a flag to control it would be sufficient, I'm happy to take on the task adding / exposing a (default enabled) flag that can be used to disable call site emission for these binary-size-constrained environments.

Yes, enabled by default seems right, but a flag to disable it seems (unfortunately) necessary, given the size increase. Would this be a flag that drops all callsite information? From a release/rollout perspective, the ideal change is something that only drops callsite info from cases added by #166202, but preserves callsite info for anything that was already there.

[jryans]

Broadly speaking, better debug info is always a good thing. As for whether we specifically care about call site information -- I have no idea how to answer that. e.g. if we drop call site information and it makes debugging in LLDB difficult/super confusing, we probably do care.

In upstream LLDB specifically, I believe the main impacts of disabling call site info would be:

• tail calls are no longer identified, so backtraces may miss these frames (because the debugger reconstructs them using the call site info)
• some source variable values (locations that make use of DW_OP_entry_value) will no longer be accessible and will report an error instead

Other debuggers and tools may put this info to further uses.

Yes, enabled by default seems right, but a flag to disable it seems (unfortunately) necessary, given the size increase. Would this be a flag that drops all callsite information? From a release/rollout perspective, the ideal change is something that only drops callsite info from cases added by #166202, but preserves callsite info for anything that was already there.

Hmm. Guarding only the changes of #166202 feels a bit odd I guess...? That work is essentially enabling call site info to be emitted for more types of calls that were missed when the feature was first added in 2019. It would seem awkward to me to add e.g. a -gmore-call-site-info=false flag that controls only this group of "some calls but not others".

I would suggest instead adding / exposing a flag to control call site info generally for all calls, since that would both help anyone here who needs to control binary size and also seems understandable a flag-controllable-concept to users long term.

That's just my opinion though. I'm happy to implement whatever flags are agreed on here, as long as we can keep full call site info default enabled.

[rupprecht]

Hmm. Guarding only the changes of #166202 feels a bit odd I guess...? That work is essentially enabling call site info to be emitted for more types of calls that were missed when the feature was first added in 2019. It would seem awkward to me to add e.g. a -gmore-call-site-info=false flag that controls only this group of "some calls but not others".

I would suggest instead adding / exposing a flag to control call site info generally for all calls, since that would both help anyone here who needs to control binary size and also seems understandable a flag-controllable-concept to users long term.

I agree in principle that it's odd to have something that only removes some call site info. The problem though is it may be removing too much -- depending on how often LLDB (or other tools) rely on this info. Yes, they would fit within size limits, but now they would be less debuggable. We can already achieve that with -g0 today :)

Using -mllvm allows more leeway with specific flags like this. Maybe we could have both options? e.g. clang -gcall-site-info=false for the more principled flag to remove it from all calls, and clang -g -mllvm -no-extra-callsite-info for the thing that just disables this?

[jryans]

I agree in principle that it's odd to have something that only removes some call site info. The problem though is it may be removing too much -- depending on how often LLDB (or other tools) rely on this info. Yes, they would fit within size limits, but now they would be less debuggable. We can already achieve that with -g0 today :)

Hmm, I think it's hard to know for sure if the initial bucket of calls that were covered since 2019 is "more used" than the feature overall... Perhaps we could start off with the more general, more easily understandable flag that disables all call site info. You can use that to disable all call site info, and the immediate short term code size problem is resolved. Then later, if one of your debugging users notices they aren't able to do some debugging activity that used to work, we could construct a more specific flag.

Using -mllvm allows more leeway with specific flags like this. Maybe we could have both options? e.g. clang -gcall-site-info=false for the more principled flag to remove it from all calls, and clang -g -mllvm -no-extra-callsite-info for the thing that just disables this?

I am a tad hesitant to construct a flag for just the cases added by #166202, because there's not an obvious single attribute that distinguishes only those calls. Weaving together a conditional to achieve this may be somewhat ungainly and awkward. The change essentially moved the call site emission to a more general place in Clang codegen. Anyway, it's certainly possible to do, just a bit awkward.

I also don't really want to add two flags if there's (so far) only one group (Google) with specific need here. Assuming that continues to hold true, it seems best to focus energies on a fix you'd actually use. If other people appear with a different need, we work on that as it arises.