[llvm-readelf] - Improve ELF type field dumping.

[grimar]

Summary

This is related to https://bugs.llvm.org/show_bug.cgi?id=40868.

Currently we don't print OS Specific/`Processor Specific/<unknown>
prefixes when dumping the ELF file type. This is not consistent
with GNU readelf. The patch fixes it.

Also, this patch removes the types.test, because we already have
file-types.test, which tests more cases and this patch revealed that
we have such a duplicate.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

3532

I think the value is the main body and Processor Specific is a comment. If something needs to be placed in (), it should not be the value.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

3532

I am printing exactly what GNU readelf prints here.

[jhenderson]

Looks good to me.

[jhenderson]

We've just run into an unexpected side-effect of this change, when this got merged into our downstream branch. We have a number of new ET_* values in the OS-specific range. In our downstream branch, we've added new entries to ElfObjectFileType so that these values are printed nicely as Type: OurCustomType. However, with this change, this has changed to Type: OS Specific: (OurCustomType) which isn't ideal. We can probably live with it, but I wonder if a small refactor might be better to allow us to have a clean downstream patch without really harming upstream code. My initial suggestion would be to change to something like this:

  if (!makeArrayRef(ElfObjectFileType).end() ==
             llvm::find_if(ElfObjectFileType,
                           [&](const EnumEntry<unsigned> &E) {
                             return E.Value == e.e_type;
                           })) {
    if (e.e_type >= ET_LOPROC) {
      Str = "Processor Specific: (" + Str + ")";
    } else if (e.e_type >= ET_LOOS) {
      Str = "OS Specific: (" + Str + ")";
    } else {
      Str = "<unknown>: " + Str;
    }
}

I think the overall code complexity is about the same. What do you think?

[grimar]

In D93096#2451796, @jhenderson wrote:

We've just run into an unexpected side-effect of this change, when this got merged into our downstream branch. We have a number of new ET_* values in the OS-specific range. In our downstream branch, we've added new entries to ElfObjectFileType so that these values are printed nicely as Type: OurCustomType. However, with this change, this has changed to Type: OS Specific: (OurCustomType) which isn't ideal. We can probably live with it, but I wonder if a small refactor might be better to allow us to have a clean downstream patch without really harming upstream code. My initial suggestion would be to change to something like this:

  if (!makeArrayRef(ElfObjectFileType).end() ==
             llvm::find_if(ElfObjectFileType,
                           [&](const EnumEntry<unsigned> &E) {
                             return E.Value == e.e_type;
                           })) {
    if (e.e_type >= ET_LOPROC) {
      Str = "Processor Specific: (" + Str + ")";
    } else if (e.e_type >= ET_LOOS) {
      Str = "OS Specific: (" + Str + ")";
    } else {
      Str = "<unknown>: " + Str;
    }
}

I think the overall code complexity is about the same. What do you think?

I guess it might be OK. It is probably a bit inconistent with GNU which does:

static char *
get_file_type (unsigned e_type)
{
  static char buff[32];

  switch (e_type)
    {
    case ET_NONE: return _("NONE (None)");
    case ET_REL:  return _("REL (Relocatable file)");
    case ET_EXEC: return _("EXEC (Executable file)");
    case ET_DYN:  return _("DYN (Shared object file)");
    case ET_CORE: return _("CORE (Core file)");

    default:
      if ((e_type >= ET_LOPROC) && (e_type <= ET_HIPROC))
	snprintf (buff, sizeof (buff), _("Processor Specific: (%x)"), e_type);
      else if ((e_type >= ET_LOOS) && (e_type <= ET_HIOS))
	snprintf (buff, sizeof (buff), _("OS Specific: (%x)"), e_type);
      else
	snprintf (buff, sizeof (buff), _("<unknown>: %x"), e_type);
      return buff;
    }
}

But at the same time it looks reasonable to print known/named types first.
I can prepare a patch, though seems there is no way to test such change.

Lets see what @MaskRay and/or others think too.

[jhenderson]

In D93096#2451814, @grimar wrote:

I guess it might be OK. It is probably a bit inconistent with GNU which does:

static char *
get_file_type (unsigned e_type)
{
  static char buff[32];

  switch (e_type)
    {
    case ET_NONE: return _("NONE (None)");
    case ET_REL:  return _("REL (Relocatable file)");
    case ET_EXEC: return _("EXEC (Executable file)");
    case ET_DYN:  return _("DYN (Shared object file)");
    case ET_CORE: return _("CORE (Core file)");

    default:
      if ((e_type >= ET_LOPROC) && (e_type <= ET_HIPROC))
	snprintf (buff, sizeof (buff), _("Processor Specific: (%x)"), e_type);
      else if ((e_type >= ET_LOOS) && (e_type <= ET_HIOS))
	snprintf (buff, sizeof (buff), _("OS Specific: (%x)"), e_type);
      else
	snprintf (buff, sizeof (buff), _("<unknown>: %x"), e_type);
      return buff;
    }
}

I'm not sure it is all that consistent actually - in the GNU case, the code immediately returns the print out of known types before falling back to unknown types, so if we had the same patch in GNU readelf, it'd likely be the same as the old output.

But at the same time it looks reasonable to print known/named types first.
I can prepare a patch, though seems there is no way to test such change.

Lets see what @MaskRay and/or others think too.

Thanks for considering it. FWIW, I agree testing might be a possible issue, but on the other hand, I think it could have easily happened that someone wrote my suggestion in the first place, and I don't think I'd have noticed any real lack of testing.

Also a very minor advantage of my approach is that if llvm-readelf were to ever gain support for known OS/Processor specific types, the code would be simpler (just add to the list), and the output possibly more consistent.

llvm/include/llvm/BinaryFormat/ELF.h

struct Elf64_Ehdr {
   bool checkMagic() const {
     return (memcmp(e_ident, ElfMagic, strlen(ElfMagic))) == 0;
   }
 
   unsigned char getFileClass() const { return e_ident[EI_CLASS]; }
   unsigned char getDataEncoding() const { return e_ident[EI_DATA]; }
 };
 
-// File types
+// File types.
+// See current registered ELF types at:
+//    http://www.sco.com/developers/gabi/latest/ch4.eheader.html
 enum {
   ET_NONE = 0,        // No file type
   ET_REL = 1,         // Relocatable file
   ET_EXEC = 2,        // Executable file
   ET_DYN = 3,         // Shared object file
   ET_CORE = 4,        // Core file
+  ET_LOOS = 0xfe00,   // Beginning of operating system-specific codes
+  ET_HIOS = 0xfeff,   // Operating system-specific
   ET_LOPROC = 0xff00, // Beginning of processor-specific codes
   ET_HIPROC = 0xffff  // Processor-specific
 };
 
 // Versioning
 enum { EV_NONE = 0, EV_CURRENT = 1 };
 
 // Machine architectures

llvm/test/tools/llvm-readobj/ELF/file-types.test

 # RUN: llvm-readelf -h %t5 | FileCheck %s --match-full-lines --check-prefix GNU-CORE
 
 # LLVM-CORE: ElfHeader {
 # LLVM-CORE:   Type: Core (0x4)
 
 # GNU-CORE: ELF Header:
 # GNU-CORE:   Type: CORE (Core file)
 
+## Test what we dump for an arbitrary unknown ELF type.
+# RUN: yaml2obj %s -DTYPE=0xFDFF -o %t.unknown
+# RUN: llvm-readobj -h %t.unknown | FileCheck %s --match-full-lines --check-prefix LLVM-UNNKNOWN
+# RUN: llvm-readelf -h %t.unknown | FileCheck %s --match-full-lines --check-prefix GNU-UNNKNOWN
+
+# LLVM-UNNKNOWN: ElfHeader {
+# LLVM-UNNKNOWN:   Type: 0xFDFF
+
+# GNU-UNNKNOWN: ELF Header:
+# GNU-UNNKNOWN:   Type: <unknown>: fdff
+
 # RUN: yaml2obj %s -DTYPE=0xfe00 -o %t6
 # RUN: llvm-readobj -h %t6 | FileCheck %s --match-full-lines --check-prefix LLVM-LOOS
 # RUN: llvm-readelf -h %t6 | FileCheck %s --match-full-lines --check-prefix GNU-LOOS
 
 # LLVM-LOOS: ElfHeader {
 # LLVM-LOOS:   Type: 0xFE00
 
 # GNU-LOOS: ELF Header:
-# GNU-LOOS:   Type: fe00
+# GNU-LOOS:   Type: OS Specific: (fe00)
 
 # RUN: yaml2obj %s -DTYPE=0xfeff -o %t7
 # RUN: llvm-readobj -h %t7 | FileCheck %s --match-full-lines --check-prefix LLVM-HIOS
 # RUN: llvm-readelf -h %t7 | FileCheck %s --match-full-lines --check-prefix GNU-HIOS
 
 # LLVM-HIOS: ElfHeader {
 # LLVM-HIOS:   Type: 0xFEFF
 
 # GNU-HIOS: ELF Header:
-# GNU-HIOS:   Type: feff
+# GNU-HIOS:   Type: OS Specific: (feff)
 
 # RUN: yaml2obj %s -DTYPE=0xff00 -o %t8
 # RUN: llvm-readobj -h %t8 | FileCheck %s --match-full-lines --check-prefix LLVM-LOPROC
 # RUN: llvm-readelf -h %t8 | FileCheck %s --match-full-lines --check-prefix GNU-LOPROC
 
 # LLVM-LOPROC: ElfHeader {
 # LLVM-LOPROC:   Type: 0xFF00
 
 # GNU-LOPROC: ELF Header:
-# GNU-LOPROC:   Type: ff00
+# GNU-LOPROC:   Type: Processor Specific: (ff00)
 
 # RUN: yaml2obj %s -DTYPE=0xffff -o %t9
 # RUN: llvm-readobj -h %t9 | FileCheck %s --match-full-lines --check-prefix LLVM-HIPROC
 # RUN: llvm-readelf -h %t9 | FileCheck %s --match-full-lines --check-prefix GNU-HIPROC
 
 # LLVM-HIPROC: ElfHeader {
 # LLVM-HIPROC:   Type: 0xFFFF
 
 # GNU-HIPROC: ELF Header:
-# GNU-HIPROC:   Type: ffff
+# GNU-HIPROC:   Type: Processor Specific: (ffff)

llvm/test/tools/llvm-readobj/ELF/types.test

-# Show that llvm-readobj can handle all standard ELF types.
-# RUN: yaml2obj %s -DTYPE=ET_NONE -o %t.none
-# RUN: llvm-readobj --file-headers %t.none | FileCheck %s --check-prefix=LLVM-NONE
-# RUN: llvm-readelf --file-headers %t.none | FileCheck %s --check-prefix=GNU-NONE
-
-# LLVM-NONE: Type: None (0x0)
-# GNU-NONE:  Type: NONE (none)
-
---- !ELF
-FileHeader:
-  Class: ELFCLASS64
-  Data:  ELFDATA2LSB
-  Type:  [[TYPE]]
-
-# RUN: yaml2obj %s -DTYPE=ET_REL -o %t.rel
-# RUN: llvm-readobj --file-headers %t.rel | FileCheck %s --check-prefix=LLVM-REL
-# RUN: llvm-readelf --file-headers %t.rel | FileCheck %s --check-prefix=GNU-REL
-
-# LLVM-REL: Type: Relocatable (0x1)
-# GNU-REL:  Type: REL (Relocatable file)
-
-# RUN: yaml2obj %s -DTYPE=ET_EXEC -o %t.exec
-# RUN: llvm-readobj --file-headers %t.exec | FileCheck %s --check-prefix=LLVM-EXEC
-# RUN: llvm-readelf --file-headers %t.exec | FileCheck %s --check-prefix=GNU-EXEC
-
-# LLVM-EXEC: Type: Executable (0x2)
-# GNU-EXEC:  Type: EXEC (Executable file)
-
-# RUN: yaml2obj %s -DTYPE=ET_DYN -o %t.so
-# RUN: llvm-readobj --file-headers %t.so | FileCheck %s --check-prefix=LLVM-SO
-# RUN: llvm-readelf --file-headers %t.so | FileCheck %s --check-prefix=GNU-SO
-
-# LLVM-SO: Type: SharedObject (0x3)
-# GNU-SO:  Type: DYN (Shared object file)
-
-# RUN: yaml2obj %s -DTYPE=ET_CORE -o %t.core
-# RUN: llvm-readobj --file-headers %t.core | FileCheck %s --check-prefix=LLVM-CORE
-# RUN: llvm-readelf --file-headers %t.core | FileCheck %s --check-prefix=GNU-CORE
-
-# LLVM-CORE: Type: Core (0x4)
-# GNU-CORE:  Type: CORE (Core file)
-
-# Show that llvm-readobj can handle an unknown ELF type.
-# RUN: yaml2obj %s -DTYPE=0x42 -o %t.unknown
-# RUN: llvm-readobj --file-headers %t.unknown | FileCheck %s --check-prefix=LLVM-UNKNOWN
-# RUN: llvm-readelf --file-headers %t.unknown | FileCheck %s --check-prefix=GNU-UNKNOWN
-
-# LLVM-UNKNOWN: Type: 0x42
-# GNU-UNKNOWN:  Type: 42
-
-# Show that llvm-readobj can handle an unknown OS-specific ELF type.
-# RUN: yaml2obj %s -DTYPE=0xfe00 -o %t.os
-# RUN: llvm-readobj --file-headers %t.os | FileCheck %s --check-prefix=LLVM-OS
-# RUN: llvm-readelf --file-headers %t.os | FileCheck %s --check-prefix=GNU-OS
-
-# LLVM-OS: Type: 0xFE00
-# GNU-OS:  Type: fe00
-
-# Show that llvm-readobj can handle an unknown machine-specific ELF type.
-# RUN: yaml2obj %s -DTYPE=0xff00 -o %t.proc
-# RUN: llvm-readobj --file-headers %t.proc | FileCheck %s --check-prefix=LLVM-PROC
-# RUN: llvm-readelf --file-headers %t.proc | FileCheck %s --check-prefix=GNU-PROC
-
-# LLVM-PROC: Type: 0xFF00
-# GNU-PROC:  Type: ff00

llvm/tools/llvm-readobj/ELFDumper.cpp

template <class ELFT> void GNUStyle<ELFT>::printFileHeaders() {
   OS << to_hexString(e.e_ident[ELF::EI_VERSION]);
   if (e.e_version == ELF::EV_CURRENT)
     OS << " (current)";
   OS << "\n";
   Str = printEnum(e.e_ident[ELF::EI_OSABI], makeArrayRef(ElfOSABI));
   printFields(OS, "OS/ABI:", Str);
   printFields(OS,
               "ABI Version:", std::to_string(e.e_ident[ELF::EI_ABIVERSION]));
+
   Str = printEnum(e.e_type, makeArrayRef(ElfObjectFileType));
+  if (e.e_type >= ET_LOPROC) {
+    Str = "Processor Specific: (" + Str + ")";
+  } else if (e.e_type >= ET_LOOS) {
+    Str = "OS Specific: (" + Str + ")";
+  } else if (makeArrayRef(ElfObjectFileType).end() ==
+             llvm::find_if(ElfObjectFileType,
+                           [&](const EnumEntry<unsigned> &E) {
+                             return E.Value == e.e_type;
+                           }))
+    Str = "<unknown>: " + Str;
   printFields(OS, "Type:", Str);
+
   Str = printEnum(e.e_machine, makeArrayRef(ElfMachineType));
   printFields(OS, "Machine:", Str);
   Str = "0x" + to_hexString(e.e_version);
   printFields(OS, "Version:", Str);
   Str = "0x" + to_hexString(e.e_entry);
   printFields(OS, "Entry point address:", Str);
   Str = to_string(e.e_phoff) + " (bytes into file)";
   printFields(OS, "Start of program headers:", Str);