[PATCH] D20331: [llvm-mc] - Teach llvm-mc to generate compressed debug sections in zlib style.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: include/llvm/MC/MCContext.h:388
@@ -387,3 +387,1 @@
 
-    void renameELFSection(MCSectionELF *Section, StringRef Name);
-
----------------
rafael wrote:
> So nice to see this go :-)
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).

================
Comment at: include/llvm/MC/MCSectionELF.h:65
@@ -64,3 +61,3 @@
 
   void setSectionName(StringRef Name) { SectionName = Name; }
 
----------------
rafael wrote:
> You can delete this one now.
Done.

================
Comment at: lib/MC/ELFObjectWriter.cpp:576
@@ -575,4 +575,3 @@
   auto &Sec = cast<MCSectionELF>(Sym->getSection());
-  unsigned Flags = Sec.getFlags();
-  if (Flags & ELF::SHF_MERGE) {
+  if (Sec.Flags & ELF::SHF_MERGE) {
     if (C != 0)
----------------
davide wrote:
> Why do you need to change this?
Because Flags is a public member since now, please see below comment.

================
Comment at: lib/MC/ELFObjectWriter.cpp:1027
@@ +1026,3 @@
+    if (sys::IsLittleEndianHost)
+      sys::swapByteOrder(Size);
+    getStream() << Size;
----------------
rafael wrote:
> 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?
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.

================
Comment at: lib/MC/MCSectionELF.cpp:164
@@ -165,4 +163,2 @@
 
-bool MCSectionELF::isVirtualSection() const {
-  return getType() == ELF::SHT_NOBITS;
-}
+bool MCSectionELF::isVirtualSection() const { return Flags == ELF::SHT_NOBITS; }
----------------
grimar wrote:
> grimar wrote:
> > davide wrote:
> > > Why did you change from `getType()` to `Flags`. Also, why can't you use the getter as they were used before?
> > 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 ?
> ah, I changed getType, but I was thinking I am changing getFlag(). Thank you, I'll revisit this one !
So i reverted this, it was a mistake change.


http://reviews.llvm.org/D20331