[NFC][llvm-nm] create a new helper function exportSymbolNamesFromFiles for --export-symbols

[DiggerLin]

Summary

create a new helper function exportSymbolNamesFromFiles for --export-symbols

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

2250–2251

Can Filename be const (potentially by modifying other signatures too)? If so, presumably it can be a StringRef?

2280

If you change dumpSymbolNamesFromFile, the type here should change too to match.

2281–2282

This const_cast suggests the function signature is probably wrong. If it isn't, and Filename needs to be mutable in that function, I'd make a local copy of the name and pass that in instead.

Unrelated, but FileSymList is now a reference to a local variable in the calling function, which will likely cause strange behaviour, including crashes. It shouldn't have the const & bit in the type signature.

2288

Use llvm::remove_if(SymbolList, ...)

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

2250–2251

thanks

2281–2282

according to https://en.cppreference.com/w/cpp/language/lifetime

The lifetime of a temporary object may be extended by binding to a const lvalue reference or to an rvalue reference (since C++11), see reference initialization for details.

when 'return SymbolList;" in dumpSymbolNamesFromFile , the compiler will create a temporary object "SymbolList" ,

So it will not crash.

2288

there is no api llvm::remove_if(SymbolList, ...) as https://en.cppreference.com/w/cpp/algorithm/remove
and if you use the llvm::remove_if(SymbolList, ...) it will compile error.

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

1931

Bonus patch, if you want: swap all these const std::string & for StringRef, especially since you're changing them anyway.

2281–2282

Huh, fair enough. Is there a reason not to take it by value though? I suspect it's identical in terms of codegen.

2288

llvm::remove_if is a wrapper around std::remove_if: https://github.com/llvm/llvm-project/blob/eddd94c27df609113af1d1b795d8483aa6dd662c/llvm/include/llvm/ADT/STLExtras.h#L1623

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

2288

thanks

[DiggerLin]

do we still another NFC patch to archive your suggestion as "https://reviews.llvm.org/D112735" ?

struct NMObject {

std::unique_ptr<Binary> Bin; // Possibly any other members that might be necessary to store archive members.
StringRef ArchiveName;
StringRef ArchitectureName;

};

......

static std::vector<NMObject> getObjectsFromFile(StringRef InputFile) {

std::vector<NMObject> Objects;
/*
  Code from dumpSymbolNamesFromFile which retrieves the objects (and archive properties, if appropriate) inside a binary (may be a single object, or many).
  Emplace an NMObject in the vector for each such constructed object, instead of calling dumpSymbolNamesFromObject.
*/
return Objects;

}

I think the logic of the patch most meet your requirement , My suggestion we can stop to do further refactor for your suggestion.

[jhenderson]

LGTM.

do we still another NFC patch

To summarise, yes, we need another patch (at least one, possibly more), but I don't really mind what that patch looks like (i.e. it doesn't have to match what I suggested before).

My key end goal of the suggestions I made in D112735 was the removal of references to ExportSymbols causing early outs in otherwise common functions. I've highlighted the one section of code that has such references. I don't really mind how you achieve that end goal, as long as it leaves the code in a clean structural state (or at least as clean as it was before you started the export symbols work). My suggestion was merely one part of the wider plan, to ensure we had the information needed, and isn't necessarily the only approach, so if you have another approach then that's fine too.

Away from that, there's at least one other minor improvement I'd like to see: rename exportSymbolNamesFromFiles to dumpExportSymbolList, as I think "dump" indicates "print something" more clearly than "export". The "export symbol list" part of the name then is what is being dumped, and nicely lines up with the option name.

llvm/tools/llvm-nm/llvm-nm.cpp

1900–1910

References to ExportSymbols.

[DiggerLin]

My key end goal of the suggestions I made in D112735 was the removal of references to ExportSymbols causing early outs in otherwise common functions.

I do not think we can remove all the references to ExportSymbols , if we want to use the functionality (which extract object file out from archive, MachOUniversalBinary,TapiUniversal) dumpSymbolNamesFromFile. we need at least one references toExportSymbols.

we can  not implement as to get all the object files from input file into a vector(or list) , and then  dump and print the symbol for each object.

The reason: when there are error on get object file from input files.
for example, we have a test case, user has three input files from llvm-nm command to dump the symbol.

1. there is warning on the input file1 as '"sizes with -print-size for Mach-O files are always zero.\n";"
2. get object files OK for input file 2
3. there are some error when get object files for input file3
   
   The current implement will output as:

error info for input file1
obj21 .o (input file2)
   symbol1 , XX ,XX
   symbol2, XX, XX
obj22.o (input file2)
   symbol1 , XX,XX
   symbol2, XX,XX
error info for input file3(can not get obj31.o from input file3)
obj32.o   (input file3)
   symbol1 , XX,XX
   symbol2, XX,XX
error info for input file3(can not get obj33.o from input file3)

if we get all the object file out and put into a list and dump(and print symbols) from each object file of list.(it will print out all the warning or error information first) the output will be changed to

error info for input file1.
error info for input file3(can not get obj31.o from input file3)
error info for input file3(can not get obj33.o from input file3)
obj21 .o (input file2)
    symbol1 , XX
    symbol2, XX
obj22.o (input file2)
   symbol1 , XX
   symbol2, XX
obj32.o   (input file3)
   symbol1 , XX
   symbol2, XX

[jhenderson]

I would say that the exact position of errors and warnings in the output is of lesser concern, and it's okay for them to change as the implementation changes, as long as the error messages have sufficient context in them - if they don't, this is something that could be improved though.

However, it occurs to me that a proposal of "fetch all symbols" and then "dump all symbols" may be costly in terms of memory usage. I'm usually not so fussed about memory usage, but I'd want to see what the impact of this approach would be. I guess the way to do better, without adversely impact the memory footprint of regular usage, is to have a lambda passed down the call stack, which operates on an object when it is encountered. Under regular usage, this lambda would be a function to print all symbols for that object (sorted as needed). Under export symbols usage, it would store the symbols in a global vector, which would then be printed etc at a later point. Outline code:

void forEachFile(function_ref<void(const std::vector<NMSymbol> &, /*other args as needed*/)> DoThis, ArrayRef<StringRef> Filenames) {
  // for each input file, determine file kind, call processArchive/processMachOUniversalBinary/...
}

void process*(... DoThis, ...) {
  // if multiple objects in file, loop over all doing the following, otherwise, do the following on just the one file:
  auto Syms = getSymbolsFromObject(Obj);
  DoThis(Syms, ...);
}

int main() {
...

  if (ExportSymbols) {
    std::vector<NMSymbol> Syms;
    forEachFile([&Syms](const std::vector<NMSymbol> &SymbolList, ...) {
                   Syms.insert(Syms.end(), SymbolList.begin(), SymbolList.end());
                }, InputFilenames);
    // filter, sort, print exports
  } else {
    forEachFile([](const std::vector<NMSymbol> &SymbolList){
                   // sort then print symbols
                }, InputFilenames);
  }
}

Hopefully you see what I'm getting at. This isn't really a high priority for me though, so don't feel obliged to look at this further, but if you do get a chance to look, I think it is worth considering. Hopefully the recent refactoring should put the code in a decent place for adopting something like this.

[DiggerLin]

In D120913#3371889, @jhenderson wrote:

I would say that the exact position of errors and warnings in the output is of lesser concern, and it's okay for them to change as the implementation changes, as long as the error messages have sufficient context in them - if they don't, this is something that could be improved though.

However, it occurs to me that a proposal of "fetch all symbols" and then "dump all symbols" may be costly in terms of memory usage. I'm usually not so fussed about memory usage, but I'd want to see what the impact of this approach would be. I guess the way to do better, without adversely impact the memory footprint of regular usage, is to have a lambda passed down the call stack, which operates on an object when it is encountered. Under regular usage, this lambda would be a function to print all symbols for that object (sorted as needed). Under export symbols usage, it would store the symbols in a global vector, which would then be printed etc at a later point. Outline code:

void forEachFile(function_ref<void(const std::vector<NMSymbol> &, /*other args as needed*/)> DoThis, ArrayRef<StringRef> Filenames) {
  // for each input file, determine file kind, call processArchive/processMachOUniversalBinary/...
}

void process*(... DoThis, ...) {
  // if multiple objects in file, loop over all doing the following, otherwise, do the following on just the one file:
  auto Syms = getSymbolsFromObject(Obj);
  DoThis(Syms, ...);
}

int main() {
...

  if (ExportSymbols) {
    std::vector<NMSymbol> Syms;
    forEachFile([&Syms](const std::vector<NMSymbol> &SymbolList, ...) {
                   Syms.insert(Syms.end(), SymbolList.begin(), SymbolList.end());
                }, InputFilenames);
    // filter, sort, print exports
  } else {
    forEachFile([](const std::vector<NMSymbol> &SymbolList){
                   // sort then print symbols
                }, InputFilenames);
  }
}

Hopefully you see what I'm getting at. This isn't really a high priority for me though, so don't feel obliged to look at this further, but if you do get a chance to look, I think it is worth considering. Hopefully the recent refactoring should put the code in a decent place for adopting something like this.

I think I got your solution. and I am prefer to delay the patch, and  I will do my high priority work first.  Thanks again for your time and your professional and useful comments in all the patches. and I learned a lot of from your comments.

llvm/tools/llvm-nm/llvm-nm.cpp

 static void dumpSymbolNamesFromObject(
     SymbolicFile &Obj, std::vector<NMSymbol> &SymbolList,
     bool PrintSymbolObject, bool PrintObjectLabel, StringRef ArchiveName = {},
     StringRef ArchitectureName = {}, StringRef ObjectName = {},
     bool PrintArchiveName = true) {
   if (!shouldDump(Obj))
     return;
 
   if (ExportSymbols && Obj.isXCOFF()) {
     XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
     getXCOFFExports(XCOFFObj, SymbolList, ArchiveName);
     return;
   }
 
   if (PrintObjectLabel && !ExportSymbols)
     printObjectLabel(PrintArchiveName, ArchiveName, ArchitectureName,
                      ObjectName.empty() ? Obj.getFileName() : ObjectName);
   if (!getSymbolNamesFromObject(Obj, SymbolList) || ExportSymbols)
     return;
   CurrentFilename = Obj.getFileName();
 
   // If there is an error in hasSymbols(), the error should be encountered in
   // function getSymbolNamesFromObject first.
   if (!cantFail(hasSymbols(Obj)) && SymbolList.empty() && !Quiet) {
     writeFileName(errs(), ArchiveName, ArchitectureName);
     errs() << "no symbols\n";
   }
 
   sortSymbolList(SymbolList);
   printSymbolList(Obj, SymbolList, PrintSymbolObject, ArchiveName,
                   ArchitectureName);
 }
 
 // checkMachOAndArchFlags() checks to see if the SymbolicFile is a Mach-O file
 // and if it is and there is a list of architecture flags is specified then
 // check to make sure this Mach-O file is one of those architectures or all
 // architectures was specificed.  If not then an error is generated and this
 // routine returns false.  Else it returns true.
-static bool checkMachOAndArchFlags(SymbolicFile *O, std::string &Filename) {
+static bool checkMachOAndArchFlags(SymbolicFile *O, StringRef Filename) {
   auto *MachO = dyn_cast<MachOObjectFile>(O);
 
   if (!MachO || ArchAll || ArchFlags.empty())
     return true;
 
   MachO::mach_header H;
   MachO::mach_header_64 H_64;
   Triple T;
   const char *McpuDefault, *ArchFlag;
static bool checkMachOAndArchFlags(SymbolicFile *O, StringRef Filename) {
   const std::string ArchFlagName(ArchFlag);
   if (!llvm::is_contained(ArchFlags, ArchFlagName)) {
     error("No architecture specified", Filename);
     return false;
   }
   return true;
 }
 
-static void dumpArchiveMap(Archive *A, std::string &Filename) {
+static void dumpArchiveMap(Archive *A, StringRef Filename) {
   Archive::symbol_iterator I = A->symbol_begin();
   Archive::symbol_iterator E = A->symbol_end();
   if (I != E) {
     outs() << "Archive map\n";
     for (; I != E; ++I) {
       Expected<Archive::Child> C = I->getMember();
       if (!C) {
         error(C.takeError(), Filename);
         break;
       }
       Expected<StringRef> FileNameOrErr = C->getName();
       if (!FileNameOrErr) {
         error(FileNameOrErr.takeError(), Filename);
         break;
       }
       StringRef SymName = I->getName();
       outs() << SymName << " in " << FileNameOrErr.get() << "\n";
     }
     outs() << "\n";
   }
 }
 
 static void dumpArchive(Archive *A, std::vector<NMSymbol> &SymbolList,
-                        std::string &Filename, LLVMContext *ContextPtr) {
+                        StringRef Filename, LLVMContext *ContextPtr) {
   if (ArchiveMap)
     dumpArchiveMap(A, Filename);
 
   Error Err = Error::success();
   for (auto &C : A->children(Err)) {
     Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary(ContextPtr);
     if (!ChildOrErr) {
       if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
for (auto &C : A->children(Err)) {
     }
   }
   if (Err)
     error(std::move(Err), A->getFileName());
 }
 
 static void dumpMachOUniversalBinaryMatchArchFlags(
     MachOUniversalBinary *UB, std::vector<NMSymbol> &SymbolList,
-    std::string &Filename, LLVMContext *ContextPtr) {
+    StringRef Filename, LLVMContext *ContextPtr) {
   // Look for a slice in the universal binary that matches each ArchFlag.
   bool ArchFound;
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {
     ArchFound = false;
     for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
                                                E = UB->end_objects();
          I != E; ++I) {
       if (ArchFlags[i] == I->getArchFlagName()) {
for (unsigned i = 0; i < ArchFlags.size(); ++i) {
     }
   }
 }
 
 // Returns true If the binary contains a slice that matches the host
 // architecture, or false otherwise.
 static bool dumpMachOUniversalBinaryMatchHost(MachOUniversalBinary *UB,
                                               std::vector<NMSymbol> &SymbolList,
-                                              std::string &Filename,
+                                              StringRef Filename,
                                               LLVMContext *ContextPtr) {
   Triple HostTriple = MachOObjectFile::getHostArch();
   StringRef HostArchName = HostTriple.getArchName();
   for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
                                              E = UB->end_objects();
        I != E; ++I) {
     if (HostArchName == I->getArchFlagName()) {
       Expected<std::unique_ptr<ObjectFile>> ObjOrErr = I->getAsObjectFile();
if (HostArchName == I->getArchFlagName()) {
       return true;
     }
   }
   return false;
 }
 
 static void dumpMachOUniversalBinaryArchAll(MachOUniversalBinary *UB,
                                             std::vector<NMSymbol> &SymbolList,
-                                            std::string &Filename,
+                                            StringRef Filename,
                                             LLVMContext *ContextPtr) {
   bool moreThanOneArch = UB->getNumberOfObjects() > 1;
   for (const MachOUniversalBinary::ObjectForArch &O : UB->objects()) {
     Expected<std::unique_ptr<ObjectFile>> ObjOrErr = O.getAsObjectFile();
     std::string ArchiveName;
     std::string ArchitectureName;
     ArchiveName.clear();
     ArchitectureName.clear();
if (ObjOrErr) {
                 " is not a Mach-O file or an archive file",
             "Mach-O universal file");
     }
   }
 }
 
 static void dumpMachOUniversalBinary(MachOUniversalBinary *UB,
                                      std::vector<NMSymbol> &SymbolList,
-                                     std::string &Filename,
+                                     StringRef Filename,
                                      LLVMContext *ContextPtr) {
   // If we have a list of architecture flags specified dump only those.
   if (!ArchAll && !ArchFlags.empty()) {
     dumpMachOUniversalBinaryMatchArchFlags(UB, SymbolList, Filename,
                                            ContextPtr);
     return;
   }
 
   // No architecture flags were specified so if this contains a slice that
   // matches the host architecture dump only that.
   if (!ArchAll &&
       dumpMachOUniversalBinaryMatchHost(UB, SymbolList, Filename, ContextPtr))
     return;
 
   // Either all architectures have been specified or none have been specified
   // and this does not contain the host architecture so dump all the slices.
   dumpMachOUniversalBinaryArchAll(UB, SymbolList, Filename, ContextPtr);
 }
 
 static void dumpTapiUniversal(TapiUniversal *TU,
                               std::vector<NMSymbol> &SymbolList,
-                              std::string &Filename) {
+                              StringRef Filename) {
   for (const TapiUniversal::ObjectForArch &I : TU->objects()) {
     StringRef ArchName = I.getArchFlagName();
     const bool ShowArch =
         ArchFlags.empty() || llvm::is_contained(ArchFlags, ArchName);
     if (!ShowArch)
       continue;
     if (!AddInlinedInfo && !I.isTopLevelLib())
       continue;
     if (auto ObjOrErr = I.getAsObjectFile())
       dumpSymbolNamesFromObject(
           *ObjOrErr.get(), SymbolList, /*PrintSymbolObject=*/false,
           /*PrintObjectLabel=*/true,
           /*ArchiveName=*/{}, ArchName, I.getInstallName());
     else if (Error E = isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
       error(std::move(E), Filename, ArchName);
     }
   }
 }
 
 static void dumpSymbolicFile(SymbolicFile *O, std::vector<NMSymbol> &SymbolList,
-                             std::string &Filename) {
+                             StringRef Filename) {
   if (!MachOPrintSizeWarning && PrintSize && isa<MachOObjectFile>(O)) {
     WithColor::warning(errs(), ToolName)
         << "sizes with --print-size for Mach-O files are always zero.\n";
     MachOPrintSizeWarning = true;
   }
   if (!checkMachOAndArchFlags(O, Filename))
     return;
   dumpSymbolNamesFromObject(*O, SymbolList, /*PrintSymbolObject=*/true,
                             /*PrintObjectLabel=*/false);
 }
 
-static void dumpSymbolNamesFromFile(std::string &Filename,
-                                    std::vector<NMSymbol> *SymbolList) {
+static std::vector<NMSymbol> dumpSymbolNamesFromFile(StringRef Filename) {
+  std::vector<NMSymbol> SymbolList;
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFileOrSTDIN(Filename);
   if (error(BufferOrErr.getError(), Filename))
-    return;
+    return SymbolList;
 
   LLVMContext Context;
   LLVMContext *ContextPtr = NoLLVMBitcode ? nullptr : &Context;
   Expected<std::unique_ptr<Binary>> BinaryOrErr =
       createBinary(BufferOrErr.get()->getMemBufferRef(), ContextPtr);
   if (!BinaryOrErr) {
     error(BinaryOrErr.takeError(), Filename);
-    return;
+    return SymbolList;
   }
   Binary &Bin = *BinaryOrErr.get();
   if (Archive *A = dyn_cast<Archive>(&Bin))
-    dumpArchive(A, *SymbolList, Filename, ContextPtr);
+    dumpArchive(A, SymbolList, Filename, ContextPtr);
   else if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(&Bin))
-    dumpMachOUniversalBinary(UB, *SymbolList, Filename, ContextPtr);
+    dumpMachOUniversalBinary(UB, SymbolList, Filename, ContextPtr);
   else if (TapiUniversal *TU = dyn_cast<TapiUniversal>(&Bin))
-    dumpTapiUniversal(TU, *SymbolList, Filename);
+    dumpTapiUniversal(TU, SymbolList, Filename);
   else if (SymbolicFile *O = dyn_cast<SymbolicFile>(&Bin))
-    dumpSymbolicFile(O, *SymbolList, Filename);
+    dumpSymbolicFile(O, SymbolList, Filename);
+  return SymbolList;
+}
+
+static void
+exportSymbolNamesFromFiles(const std::vector<std::string> &InputFilenames) {
+  std::vector<NMSymbol> SymbolList;
+  for (const auto &FileName : InputFilenames) {
+    std::vector<NMSymbol> FileSymList = dumpSymbolNamesFromFile(FileName);
+    SymbolList.insert(SymbolList.end(), FileSymList.begin(), FileSymList.end());
+  }
+
+  // Delete symbols which should not be printed from SymolList.
+  SymbolList.erase(
+      llvm::remove_if(SymbolList,
+                      [](const NMSymbol &s) { return !s.shouldPrint(); }),
+      SymbolList.end());
+  sortSymbolList(SymbolList);
+  SymbolList.erase(std::unique(SymbolList.begin(), SymbolList.end()),
+                   SymbolList.end());
+  printExportSymbolList(SymbolList);
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   BumpPtrAllocator A;
   StringSaver Saver(A);
   NmOptTable Tbl;
   ToolName = argv[0];
int main(int argc, char **argv) {
   if (InputFilenames.empty())
     InputFilenames.push_back("a.out");
   if (InputFilenames.size() > 1)
     MultipleFiles = true;
 
   if (NoDyldInfo && (AddDyldInfo || DyldInfoOnly))
     error("--no-dyldinfo can't be used with --add-dyldinfo or --dyldinfo-only");
 
-  std::vector<NMSymbol> SymbolList;
-
-  llvm::for_each(InputFilenames, std::bind(dumpSymbolNamesFromFile,
-                                           std::placeholders::_1, &SymbolList));
-
-  if (ExportSymbols) {
-    // Delete symbols which should not be printed from SymolList.
-    SymbolList.erase(
-        std::remove_if(SymbolList.begin(), SymbolList.end(),
-                       [](const NMSymbol &s) { return !s.shouldPrint(); }),
-        SymbolList.end());
-    sortSymbolList(SymbolList);
-    SymbolList.erase(std::unique(SymbolList.begin(), SymbolList.end()),
-                     SymbolList.end());
-    printExportSymbolList(SymbolList);
-  }
+  if (ExportSymbols)
+    exportSymbolNamesFromFiles(InputFilenames);
+  else
+    llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);
 
   if (HadError)
     return 1;
 }