[PATCH] D120357: [llvm-nm]add helper function to print out the object file name, archive name, architecture name

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1774
     if (Nsect == 0)
-      return;
+      return false;
   }
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Are you sure this should be `return false`? (I don't know either way, but we should preserve behaviour).
> > > the patch almost is NFC patch, I do not think it is a good idea to change the preserve 
> > >  behaviour.
> > I said we should preserve behaviour. My question was: does this?
> not sure your question, would  you like to  explain more detail?
> 
> according to the comment
> 
>  
> ```
>  // If a "-s segname sectname" option was specified and this is a Mach-O
>   // file get the section number for that section in this object file.
>   unsigned int Nsect = 0;
>   MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
>   if (!SegSect.empty() && MachO) {
>     Nsect = getNsectForSegSect(MachO);
>     // If this section is not in the object file no symbols are printed.
>     if (Nsect == 0)
>       return false;
>   }
> ```
> 
> if the user specific a section name from llvm-nm command line
> "-s segname sectname", but the section not in the Mach-O object, it can not get the symbol. it should return the false. 
> 
> do I answer your question ? 
> 
Not exactly. Previously, the behaviour of `dumpSymbolNamesFromObject` was to not print anything if the section was not found. Importantly, it also skipped the `no symbols` printing and also didn't print an error.

This function's  comment says that `return false` indicates an error. As not finding a section previously didn't report an error, `return false` here didn't quite line up with that comment. Consequently, I wondered whether it was correct to `return false`, and the important factor was whether this caused a behaviour change or not. Having gone through this patch again, I now think `return false` is correct amd preserves the existing behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357