[PATCH] D88468: [llvm-readobj] Fix error when printing out stabs symbols of type N_OSO

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs.yaml:120
+  filetype:        0x00000002
+  ncmds:           8
+  sizeofcmds:      744
----------------
int3 wrote:
> grimar wrote:
> > I was able to quickly reduce the number of commands here to `2` by simple removing of unused parts:
> > 
> > 
> > ```
> > ncmds:  2
> > ...
> > LoadCommands:
> >   - cmd:             LC_SEGMENT_64
> >     cmdsize:         232
> >     segname:         __TEXT
> >     vmaddr:          4294967296
> >     vmsize:          4096
> >     fileoff:         0
> >     filesize:        4096
> >     maxprot:         5
> >     initprot:        5
> >     nsects:          1
> >     flags:           0
> >     Sections:
> >       - sectname:        __text
> >         segname:         __TEXT
> >         addr:            0x0000000100000FA0
> >         size:            15
> >         offset:          0x00000FA0
> >         align:           4
> >         reloff:          0x00000000
> >         nreloc:          0
> >         flags:           0x80000400
> >         reserved1:       0x00000000
> >         reserved2:       0x00000000
> >         reserved3:       0x00000000
> >         content:         554889E531C0C745FC000000005DC3
> >   - cmd:             LC_SYMTAB
> >     cmdsize:         24
> >     symoff:          4152
> >     nsyms:           11
> >     stroff:          4328
> >     strsize:         104
> > ```
> the resulting binary won't execute successfully though. I kept them because I thought it would be nice to have a somewhat representative input to the linker...
I don't think we do this normally. E.g, usually any`llvm-readobj (ELF)` test cases are as minimal as possible and we don't care about feeding them to a linker and executing. It is significantly easier to read, mantain and expand tests that are little and contain only important parts.  


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:287
+  case MachO::N_ECOML:
+    return true;
+  default:
----------------
int3 wrote:
> grimar wrote:
> > I was able to comment out few cases:
> > 
> > ```
> >   switch (Type) {
> >   case MachO::N_FUN:
> >   //case MachO::N_STSYM:
> >   //case MachO::N_LCSYM:
> >   case MachO::N_BNSYM:
> >   //case MachO::N_SLINE:
> >   case MachO::N_ENSYM:
> >   case MachO::N_SO:
> >   //case MachO::N_SOL:
> >   //case MachO::N_ENTRY:
> >   //case MachO::N_ECOMM:
> >   //case MachO::N_ECOML:
> >     return true;
> >   default:
> >     return false;
> > ```
> > 
> > And the test case passed for me. I think it means these cases are not tested properly.
> I got these cases from reading `stabs.h` in the macOS SDK, but I'm not actually sure how to get the linker to generate them. Would you prefer I not print out the section name for all stabs sections, as was done for llvm-objdump in D71394?
> 
> Another option would be to edit the YAML to create these stabs symbols, but as above, it seemed a bit iffy to be creating something that might not match a realistic input.
> Would you prefer I not print out the section name for all stabs sections, as was done for llvm-objdump in D71394?

I am - not an expert in MachO, so this is probably a question to someone who is more familar with MachO. I.e. what output is expected. Perhaps it is fine, since was accepted for llvm-objdump.

> Another option would be to edit the YAML to create these stabs symbols, but as above, it seemed a bit iffy to be creating something that might not match a realistic input.

For testing - I think this would be a right approach. For ELF dumper part of `llvm-readelf` we handle many unrealistic cases to check we print a reasonable output and/or warnings and don't crash etc. `llvm-readelf` is a tool that can be used to inspect broken inputs, so I believe it is fine to feed it with unusual things and check it shows a resonable output.

Another point is that if you don't want to create "something that might not match a realistic input", then probably you don't need to write an untested code that supports this first of all?

To summarize, I see following solutions here:
1) Either keep only the code for cases that are covered by test and leave the rest for follow-ups.
2) Add all possible types of stab symbols manually to YAML.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88468