[llvm-readobj] - Teach readobj to recognize SHF_COMPRESSED flag.

[grimar]

Summary

Main problem here was that SHF_COMPRESSED has the same value with XCORE_SHF_CP_SECTION,
which was included as standart (common) flag.
As far I understand xCore is a family of controllers and it that means it's constant should be processed separately,
only if e_machine == EM_XCORE, otherwise llvm-readobj would ouput different constants twice for compressed section:

SHF_COMPRESSED (0x800)
....
XCORE_SHF_CP_SECTION (0x800)

what probably does not make sence if you're not working with xcore file.

[• rafael]

LGTM with a testcase.

[davide]

Add a test case, please. With that, I'm fine.

[grimar]

In D20273#430490, @rafael wrote:

LGTM with a testcase.

In D20273#430522, @davide wrote:

Add a test case, please. With that, I'm fine.

Thank you guys for review, I have a question then.
As far I know llvm-mc do not support currently feature to generate compressed debug sections in
zlib style (afaik, llvm-mc generates it in only zlib-gnu style, so it is not possible to emit SHF_COMPRESSED flag).

So what I want to suggest here:
what if I'll prepare patch for llvm-mc that will allow it to generate zlib style sections and after that update this one ?
I can guess that it is a good feature for llvm-mc.
If you would agree with above then my question would be - should we need to support both ways or can just switch
to zlib style and cut off gnu one ? (afaik zlib-gnu is depricated in binutils).

After implementing change for llvm-mc I`ll be able to prepare testcase for this. Or I can commit this one and prepare
and commit a testcase after changes to llvm-mc, please tell me what is a acceptable way.
(one more way would be to use pre-compiled object file for testcase right now).

[• rafael]

I like the idea of switching llvm-mc first. I think you can just
switch to the new style.

Cheers,
Rafael

[dblaikie]

I think it'd be handy to have both formats supported for reading/dumping - at least I personally still deal with compilers generating the old format (indeed, that's why I implemented the old format - it's what I saw in the version of GCC I was trying to be compatible with).

In any case, pretty sure I implemented this the first time by adding libObject support first, checking in a binary with compressed debug info generated by GCC. If you break libObject's ability to read the old style .zdebug format, you /should/ find some test cases that fail.

[grimar]

In D20273#434504, @dblaikie wrote:

I think it'd be handy to have both formats supported for reading/dumping - at least I personally still deal with compilers generating the old format (indeed, that's why I implemented the old format - it's what I saw in the version of GCC I was trying to be compatible with).

In any case, pretty sure I implemented this the first time by adding libObject support first, checking in a binary with compressed debug info generated by GCC. If you break libObject's ability to read the old style .zdebug format, you /should/ find some test cases that fail.

I added support for dumping new zlib style in llvm-dwarfdump in D20470. I don't think any of my patches breaking ability to read/dump the old style anywhere, D20331 changes only llvm-mc output style.

[grimar]

I committed this using precompiled binary for testcase.
The reasons why I did not use llvm-mc were:
I found that testcases for llvm-readobj mostly uses precompiled objects. I didn't find llvm-mc
usings. Also, generating of compressed sections from llvm-mc depends on zlib. It is disabled
for windows atm and may be also disabled on unix. We probably do not want to have so many
excessive dependencies like asm->llvm-mc->zlib, for tools like readobj just to check the flags.

[davide]

I'm still convinced (for many reasons) that checking in a binary isn't a great option when we can use MC.
I don't think "excessive dependencies like asm->llvm-mc->zlib" is a strong argument as we have them anyway.
OTOH, using our tools instead of a binary allows:

1. dogfooding (which might catch regression)
2. easier changes in case something breaks in the toolchain (we don't need to generate the binary again, we can just change the test)
3. readability (you can read the ASM instead of a description). Maybe it's not the case but from time to time precompiled binaries lack a comment which describes how they've been generated, and comments and binary can get out of sync quickly.

In other words, I'm always for using llvm-mc instead of a binary (unless there's a strong reason not to). I'd rather bit the bullet and implement the missing feature in the assembler instead of going through the binary route which may lead to bigger pain later (I had to generate again ~10 binaries in DebugInfo tests in order to fix a bug in the verifier recently).

[davide]

In D20273#436327, @grimar wrote:

I found that testcases for llvm-readobj mostly uses precompiled objects.

If they do, and they can use MC instead, I think the llvm-readobj tests should be converted and not the other way around.

[grimar]

In D20273#436361, @davide wrote:

I'm still convinced (for many reasons) that checking in a binary isn't a great option when we can use MC.
I don't think "excessive dependencies like asm->llvm-mc->zlib" is a strong argument as we have them anyway.
OTOH, using our tools instead of a binary allows:

1. dogfooding (which might catch regression)

I think in this patch we do not want to test llvm-mc, we want to check that proper flag is dumped from valid object. Since object is one time commited and
therefore always is valid, regression might be only because of changes in readobj code. If we would want to check that llvm-mc
generates proper flags, it should be test for llvm-mc, but not readobj.

2. easier changes in case something breaks in the toolchain (we don't need to generate the binary again, we can just change the test)

We probably never need to gererate binary again except if it was incorrect initially.

3. readability (you can read the ASM instead of a description). Maybe it's not the case but from time to time precompiled binaries lack a comment which describes how they've been generated, and comments and binary can get out of sync quickly.

Sometimes yes, we can read, but speaking of this patch you not only would not find anything about SHF_COMPRESSED flag in asm, but also would need to read llvm-mc command line
to find was compression set or not and also know what style of compression was used, was it zlib (which sets the flag) or zlib-gnu, which don't. So it would be a huge pain here.

In other words, I'm always for using llvm-mc instead of a binary (unless there's a strong reason not to). I'd rather bit the bullet and implement the missing feature in the assembler instead of going through the binary route which may lead to bigger pain later (I had to generate again ~10 binaries in DebugInfo tests in order to fix a bug in the verifier recently).

So my opinion that sometimes it is profitably to use llvm-mc, but sometimes - not.
(Just in case, are you aware that zlib on windows is broken >= 2 years ? https://llvm.org/bugs/show_bug.cgi?id=19403, so use of llvm-mc for this test would just skip
running testcases on whole windows world). That is what I thinkg a good example of excessive dependency on llvm-mc.

[davide]

In D20273#436565, @grimar wrote:

So my opinion that sometimes it is profitably to use llvm-mc, but sometimes - not.
(Just in case, are you aware that zlib on windows is broken >= 2 years ? https://llvm.org/bugs/show_bug.cgi?id=19403, so use of llvm-mc for this test would just skip
running testcases on whole windows world). That is what I thinkg a good example of excessive dependency on llvm-mc.

I respectfully disagree. There's no correlation between zlib broken on windows and llvm-mc.
If zlib is broken on windows, we should spend time fixing it, not silently ignoring the problem.

[davide]

In D20273#436565, @grimar wrote:

So my opinion that sometimes it is profitably to use llvm-mc, but sometimes - not.
(Just in case, are you aware that zlib on windows is broken >= 2 years ? https://llvm.org/bugs/show_bug.cgi?id=19403, so use of llvm-mc for this test would just skip
running testcases on whole windows world). That is what I thinkg a good example of excessive dependency on llvm-mc.

I respectfully disagree. There's no correlation between zlib broken on windows and llvm-mc.
If zlib is broken on windows, we should spend time fixing it, not silently ignoring the problem.

In D20273#436566, @davide wrote:

In D20273#436565, @grimar wrote:

So my opinion that sometimes it is profitably to use llvm-mc, but sometimes - not.
(Just in case, are you aware that zlib on windows is broken >= 2 years ? https://llvm.org/bugs/show_bug.cgi?id=19403, so use of llvm-mc for this test would just skip
running testcases on whole windows world). That is what I thinkg a good example of excessive dependency on llvm-mc.

I respectfully disagree. There's no correlation between zlib broken on windows and llvm-mc.
If zlib is broken on windows, we should spend time fixing it, not silently ignoring the problem.

I went ahead and reassigned PR19403 to myself.

[grimar]

In D20273#436572, @davide wrote:

I went ahead and reassigned PR19403 to myself.

Thank you ! I am not good familar with CMake and llvm build system and was
not sure I can fix that by myself.

llvm/trunk/test/tools/llvm-readobj/elf-sec-compressed.test

+RUN: llvm-readobj -sections \
+RUN:   %p/Inputs/compression.zlib.style.elf-x86-64 | FileCheck %s
+
+CHECK:      Section {
+CHECK:       Name: .debug_info
+CHECK-NEXT:  Type: SHT_PROGBITS
+CHECK-NEXT:  Flags [ 
+CHECK-NEXT:   SHF_COMPRESSED (0x800)
+CHECK-NEXT:  ]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

static const EnumEntry<unsigned> ElfSectionFlags[] = {
   ENUM_ENT(SHF_STRINGS,          "S"),
   ENUM_ENT(SHF_INFO_LINK,        "I"),
   ENUM_ENT(SHF_LINK_ORDER,       "L"),
   ENUM_ENT(SHF_OS_NONCONFORMING, "o"),
   ENUM_ENT(SHF_GROUP,            "G"),
   ENUM_ENT(SHF_TLS,              "T"),
   ENUM_ENT(SHF_MASKOS,           "o"),
   ENUM_ENT(SHF_MASKPROC,         "p"),
-  ENUM_ENT_1(XCORE_SHF_CP_SECTION),
-  ENUM_ENT_1(XCORE_SHF_DP_SECTION),
+  ENUM_ENT_1(SHF_COMPRESSED),
+};
+
+static const EnumEntry<unsigned> ElfXCoreSectionFlags[] = {
+  LLVM_READOBJ_ENUM_ENT(ELF, XCORE_SHF_CP_SECTION),
+  LLVM_READOBJ_ENUM_ENT(ELF, XCORE_SHF_DP_SECTION)
 };
 
 static const EnumEntry<unsigned> ElfAMDGPUSectionFlags[] = {
   LLVM_READOBJ_ENUM_ENT(ELF, SHF_AMDGPU_HSA_GLOBAL),
   LLVM_READOBJ_ENUM_ENT(ELF, SHF_AMDGPU_HSA_READONLY),
   LLVM_READOBJ_ENUM_ENT(ELF, SHF_AMDGPU_HSA_CODE),
   LLVM_READOBJ_ENUM_ENT(ELF, SHF_AMDGPU_HSA_AGENT)
 };
for (const Elf_Shdr &Sec : Obj->sections()) {
     case EM_MIPS:
       SectionFlags.insert(SectionFlags.end(), std::begin(ElfMipsSectionFlags),
                           std::end(ElfMipsSectionFlags));
       break;
     case EM_X86_64:
       SectionFlags.insert(SectionFlags.end(), std::begin(ElfX86_64SectionFlags),
                           std::end(ElfX86_64SectionFlags));
       break;
+    case EM_XCORE:
+      SectionFlags.insert(SectionFlags.end(), std::begin(ElfXCoreSectionFlags),
+                          std::end(ElfXCoreSectionFlags));
+      break;
     default:
       // Nothing to do.
       break;
     }
     W.printFlags("Flags", Sec.sh_flags, makeArrayRef(SectionFlags));
     W.printHex("Address", Sec.sh_addr);
     W.printHex("Offset", Sec.sh_offset);
     W.printNumber("Size", Sec.sh_size);