Page MenuHomePhabricator

[DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types
ClosedPublic

Authored by akhuang on Wed, Jan 13, 3:23 PM.

Details

Summary

In line-tables-only mode, we used to emit qualified names as the display name for functions when using CodeView.
This patch changes to emitting the parent scopes instead, with forward declarations for class types.
Also remove the linkage name in line-tables-only to save size.
The total object file size ends up being slightly smaller than if we use the full qualified names.

Diff Detail

Event Timeline

akhuang created this revision.Wed, Jan 13, 3:23 PM
akhuang requested review of this revision.Wed, Jan 13, 3:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Jan 13, 3:23 PM

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

This was kind of motivated by the issue in https://bugs.llvm.org/show_bug.cgi?id=48432, even though this patch doesn't solve that particular bug. I guess the general issue is that in CV the only way to make the functions unique is by their display name or type. So previously we were going in the direction of differentiating functions by the display name (like using the qualified name), but it seems like it's more size-efficient to use types instead, which is what this patch does.

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

This was kind of motivated by the issue in https://bugs.llvm.org/show_bug.cgi?id=48432, even though this patch doesn't solve that particular bug. I guess the general issue is that in CV the only way to make the functions unique is by their display name or type. So previously we were going in the direction of differentiating functions by the display name (like using the qualified name), but it seems like it's more size-efficient to use types instead, which is what this patch does.

OK - thanks for walking me through it, that sounds good to me.

Might be worth rephrasing from "emit type information for function scopes" to "emit parent/context scopes for functions, using declarations for any scopes that are types". (my own fault, but reading this quickly, I assumed it was about emitting type information for the parameters (DWARF gmlt drops the parameters/types))

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?

Might be worth rephrasing from "emit type information for function scopes" to "emit parent/context scopes for functions, using declarations for any scopes that are types". (my own fault, but reading this quickly, I assumed it was about emitting type information for the parameters (DWARF gmlt drops the parameters/types))

That sounds good

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

akhuang retitled this revision from [DebugInfo][CodeView] Change in line tables only mode to emit type information for function scopes, rather than using the qualified name. to [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.Wed, Jan 13, 4:46 PM
akhuang edited the summary of this revision. (Show Details)
rnk added a comment.Wed, Jan 13, 5:42 PM

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

Yes, I encouraged Amy to approach this incrementally:

  • emit forward declarations for types used in function scopes (this patch)
  • make lambda type names more unique (next)
  • try discarding the linkage name from our type info to save size
  • add overload type information

In particular, I wanted to measure the size impact of adding function type info in isolation. I suspect that the type info we need to make overload signatures unique is very small. My theory is that most debug info size comes from complete descriptions of class types.

rnk added inline comments.Wed, Jan 13, 5:48 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1050–1052

I see Amy included the linkage name step as part of this patch, so disregard that comment.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
370–373 ↗(On Diff #316525)

Do we still need the LLVM part of this? We needed it before because we were operating on strings like foo::<unnamed-tag>::operator(). Can we leave it as is for now, or handle it as a separate patch?

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

akhuang added inline comments.Thu, Jan 14, 1:59 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1050–1052

oops, didn't realize that I included it here. Anyway, I can commit this separately.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
370–373 ↗(On Diff #316525)

Oh, right. I don't think we need this anymore..

akhuang updated this revision to Diff 316779.Thu, Jan 14, 2:02 PM
akhuang edited the summary of this revision. (Show Details)

Remove llvm change.

akhuang edited the summary of this revision. (Show Details)Thu, Jan 14, 2:49 PM
rnk accepted this revision.Thu, Jan 14, 5:24 PM

lgtm

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

Thanks for checking the direction, David, I felt it was worth getting your input.

This revision is now accepted and ready to land.Thu, Jan 14, 5:24 PM