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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:287
+  case MachO::N_ECOML:
+    return true;
+  default:
----------------
grimar wrote:
> 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.
I think keeping the code only for cases covered by test would make for pretty surprising behavior to someone who hasn't seen the corresponding test. I could see some future user being confused as to why an arbitrary subset of STABS symbols get their section names displayed. I think matching llvm-objdump's behavior (and adding a TODO) is easiest for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88468