Page MenuHomePhabricator

[llvm-mc] - Teach llvm-mc to generate compressed debug sections in zlib style.
ClosedPublic

Authored by grimar on May 17 2016, 8:44 AM.

Details

Summary

Before this patch llvm-mc generated zlib-gnu styled sections.
That means no SHF_COMPRESSED flag was set, magic 'zlib' signature
was used in combination with full size field. Sections were renamed to "*.z*".
This patch reimplements the compression style to zlib one as zlib-gnu looks
to be depricated everywhere atm.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 57486.May 17 2016, 8:44 AM
grimar retitled this revision from to [llvm-mc] - Teach llvm-mc to generate compressed debug sections in zlib style..
grimar updated this object.
grimar added reviewers: dblaikie, rafael, davide.
grimar added subscribers: llvm-commits, grimar.
davide added inline comments.May 17 2016, 9:11 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57486)

Why do you need to change this?

lib/MC/MCSectionELF.cpp
164 ↗(On Diff #57486)

Why did you change from getType() to Flags. Also, why can't you use the getter as they were used before?

grimar added inline comments.May 17 2016, 9:21 AM
lib/MC/MCSectionELF.cpp
164 ↗(On Diff #57486)

I made Flags public because I need to add SHF_COMPRESSED flag to it.
In lld if we use the variable directly like I did, we do not using getters setters anymore, because there is no sense.
getters/setters are only useful if can protect something, but if you need and use both of them, then probably you do not need any of in fact. So public variable is better here.
I am not sure about style in any other project that lld, I had no real expirience in changing anything aside yet, but I guess it should be the same ?

rafael added inline comments.May 17 2016, 9:24 AM
include/llvm/MC/MCContext.h
388 ↗(On Diff #57486)

So nice to see this go :-)

lib/MC/ELFObjectWriter.cpp
1027 ↗(On Diff #57486)

So, the size is always written big endian? That is a bit odd, but if that is the case, can you use support::endian::Writer<support::big> instead of the if?

rafael added inline comments.May 17 2016, 9:39 AM
include/llvm/MC/MCSectionELF.h
65 ↗(On Diff #57486)

You can delete this one now.

grimar added inline comments.May 17 2016, 9:49 AM
lib/MC/MCSectionELF.cpp
164 ↗(On Diff #57486)

ah, I changed getType, but I was thinking I am changing getFlag(). Thank you, I'll revisit this one !

grimar updated this revision to Diff 57611.May 18 2016, 6:40 AM
  • Addressed review comments.
include/llvm/MC/MCContext.h
388 ↗(On Diff #57486)

I fisrt time saw that code, but the fact of renaming something after giving it a name looked like a hack for me, I was also happy to do that :)
(so zlib-gnu style of compressed sections itself looks like a hack in ELF world for me now, it is good that we are able just to get rid of that instead of continue supporting, I think).

include/llvm/MC/MCSectionELF.h
65 ↗(On Diff #57486)

Done.

lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57486)

Because Flags is a public member since now, please see below comment.

1027 ↗(On Diff #57486)

wow, thanks for noticing. At fact the big endian should be used always for zlib-gnu style headers. But for zlib ones (this patch) elf file encoding should be used. I was sure that rule about big endian applies for both, even was not able to imaging a trap here. But there is a difference between styles, so for zlib style - target elf file endianess is a must, I updated the code.

lib/MC/MCSectionELF.cpp
164 ↗(On Diff #57486)

So i reverted this, it was a mistake change.

I`ll update this one again soon. Forgot to change a piece of code :(

grimar updated this revision to Diff 57614.May 18 2016, 7:03 AM
  • Updated patch, to write the forgotten type field.
  • Also use header variables instead of c++ types (NFC, but looks better IMO).
davide added inline comments.May 18 2016, 7:42 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

You can change that separately, if you really like. It's unrelated, and makes the patch slightly less readable.

lib/MC/MCSectionELF.cpp
162 ↗(On Diff #57614)

Unrelated.

grimar added inline comments.May 18 2016, 8:03 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

Please see my comment below.

lib/MC/MCSectionELF.cpp
162 ↗(On Diff #57614)

I am sorry but I find this and above change directly related since that changes are connected to what this patch do.
But if you think i need to make Flags variable public at first in separate commit, then I have no problems with that, I just think if this whole patch be approved, then I`ll do 2 commits, but otherwise there is probably no need to perform changes with Flags member.

davide added inline comments.May 18 2016, 8:11 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

I can review them at the same time but they should be committed separately.

1024 ↗(On Diff #57614)

static_cast<> instead of C-style cast?

davide added inline comments.May 18 2016, 8:15 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

I saw your comment. There's no need to change at the same time the access control and remove the getter. Leave it private, and leave the getter to keep the patch small. You can do that in a separate commit.

grimar added inline comments.May 18 2016, 8:22 AM
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

Sorry but that means I need to intoduce setter to change Flags field ?

// Set the compressed flag. That is zlib style.
  Section.Flags |= ELF::SHF_COMPRESSED;

So do I correctly understand that you want me to create setter for this patch and remove it and getter in following commit ? what is the point for that ? May be let me just make Flags public at first ?

1024 ↗(On Diff #57614)

will do. quick search shows that both way are used, but I would agree here.

rafael added inline comments.May 18 2016, 12:08 PM
lib/MC/MCSectionELF.cpp
162 ↗(On Diff #57614)

Lets please do this first with just a getter and setter to make the patch easy to read.

grimar updated this revision to Diff 57740.May 19 2016, 12:33 AM
  • Addressed review comments.
lib/MC/ELFObjectWriter.cpp
576 ↗(On Diff #57614)

Returned back original code, added setter for Flags instead.

lib/MC/MCSectionELF.cpp
162 ↗(On Diff #57614)

Done.

rafael accepted this revision.May 19 2016, 5:06 AM
rafael edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.May 19 2016, 5:06 AM
davide accepted this revision.May 19 2016, 6:06 AM
davide edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.

Had to temporaliry revert this, but also found another issue with landing, details are here: D20466
Working on that.

emaste added a subscriber: emaste.May 24 2016, 6:34 AM