Page MenuHomePhabricator

Implement common symbol size calculation in llvm-size
ClosedPublic

Authored by khemant on Feb 2 2016, 1:17 PM.

Details

Summary

Commons do not live in any sections. Their sizes need to be added to total size. This option adds them to total size. In Berkely format, this is shown in bss size. In Sys V format a new row named "*COM*" is added that shows the common totals. Both format will have this added to the grand total if the option is set.

This is similar to GNU size binutils tool.

Diff Detail

Repository
rL LLVM

Event Timeline

khemant updated this revision to Diff 46691.Feb 2 2016, 1:17 PM
khemant retitled this revision from to Implement common symbol size calculation in llvm-size.
khemant updated this object.
khemant added reviewers: llvm-commits, davide.
khemant set the repository for this revision to rL LLVM.
khemant added subscribers: davide, llvm-commits.

This change needs update to test case after Rafael's rework on previous patch.

davide edited edge metadata.Feb 10 2016, 10:12 AM

This looks fine. Update, I'll take another look and sign off.

Thanks!

Davide

rafael added inline comments.
test/tools/llvm-size/basic.test
14 ↗(On Diff #46691)

It is really easy to create common symbols with a given size from assembly. Please use llvm-mc instead of checking in a binary.

Indeed, as Rafael pointed already out, please convert the test to use MC. Thanks.

khemant updated this revision to Diff 49255.Feb 26 2016, 3:21 PM
khemant edited edge metadata.

Changed test case.

rafael added inline comments.Mar 1 2016, 1:56 PM
test/tools/llvm-size/X86/test-common.s
17 ↗(On Diff #49255)

Do you actually need all this? To test just the common size I don't see you need any asm instruction.

tools/llvm-size/llvm-size.cpp
62 ↗(On Diff #49255)

git-clang-format

137 ↗(On Diff #49255)

TotalCommons

370 ↗(On Diff #49255)

CommonSize

khemant added inline comments.Mar 1 2016, 2:38 PM
test/tools/llvm-size/X86/test-common.s
17 ↗(On Diff #49255)

I needed some section with non zero size for case where no common is specified.
Does that matter?

tools/llvm-size/llvm-size.cpp
370 ↗(On Diff #49255)

There are variables with all lower case. Do you want me to converst all locals to upper before merging this patch with upper case variables pertaining to this change?

khemant updated this revision to Diff 50065.Mar 8 2016, 1:22 PM

Addressed comments

khemant marked 4 inline comments as done.Mar 8 2016, 1:23 PM
khemant added a subscriber: khemant.Mar 8 2016, 1:25 PM

I will make the blakent style change after this patch. For now I am making the changes you mentioned.

Thanks.

Hemant Kulkarni
khemant@codeaurora.org
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation

rafael accepted this revision.Mar 28 2016, 9:26 AM
rafael added a reviewer: rafael.

LGTM

This revision is now accepted and ready to land.Mar 28 2016, 9:26 AM
This revision was automatically updated to reflect the committed changes.