[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
llvm-commits at lists.llvm.org
Tue Mar 8 21:54:10 PST 2022
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
More information about the llvm-commits
mailing list