[PATCH] D93096: [llvm-readelf] - Improve ELF type field dumping.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D93096#2451796 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93096/new/

https://reviews.llvm.org/D93096