This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Emit function types in -gline-tables-only.
ClosedPublic

Authored by akhuang on Jan 19 2021, 1:43 PM.

Details

Summary

This change adds function types to further differentiate between
FUNC_IDs in -gline-tables-only.

Size increase of object files in clang are
Before: 917990 kb
After: 999312 kb

Bug: https://bugs.llvm.org/show_bug.cgi?id=48432

Diff Detail

Event Timeline

akhuang requested review of this revision.Jan 19 2021, 1:43 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2021, 1:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added inline comments.Jan 19 2021, 2:33 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1051–1052

We discussed giving lambdas display names as an alternative to doing this.

akhuang updated this revision to Diff 317701.Jan 19 2021, 2:48 PM

Remove lambda change

dblaikie added inline comments.Jan 19 2021, 2:50 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1051–1052

Should this be only for CodeView (due to CV's need for functions to be distinct in some way)? Adding the linkage name could increase debug info size for DWARF consumers unnecessarily

3740–3743

And here

akhuang updated this revision to Diff 317718.Jan 19 2021, 4:00 PM
akhuang marked an inline comment as done.

Add check for CodeView

clang/lib/CodeGen/CGDebugInfo.cpp
1051–1052

I think the CodeView change is elsewhere - here it just makes sure we only emit linkage names if it's not line tables only. (Now I'm remembering me creating the hasReducedDebugInfo function and the naming is pretty confusing...)

dblaikie added inline comments.Jan 19 2021, 4:56 PM
clang/test/CodeGenCXX/debug-info-gline-tables-only-codeview.cpp
31–33

This test doesn't seem related to the code change. Perhaps it's meant to test the "type:" parameter of the DISubprogram (like the type parameter of the DISubprogram for "m" is tested).

akhuang added inline comments.Jan 19 2021, 5:02 PM
clang/test/CodeGenCXX/debug-info-gline-tables-only-codeview.cpp
31–33

ah, yeah, it's not. I originally had a change for lambdas but took it out. I think I'll remove this part of the test for now.

akhuang updated this revision to Diff 317728.Jan 19 2021, 5:03 PM

Update test

dblaikie accepted this revision.Jan 19 2021, 5:11 PM

Looks good!

Might be worth some comments explaining why it's being done this way (probably good to have some details wherever we're making these changes to cope with CV's need for distinct functions by name/type/etc).

This revision is now accepted and ready to land.Jan 19 2021, 5:11 PM
akhuang updated this revision to Diff 317975.Jan 20 2021, 12:42 PM

Add comments describing reason for emitting types

This revision was landed with ongoing or failed builds.Jan 20 2021, 12:48 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJan 26 2021, 9:22 AM

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.