[PATCH] D158004: llvm-nm ignore the Import symbol file for the --export-symbol option.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin added a comment.

> what is preventing the change to clang to not call llvm-nm with import files?



1. Opening a file and checking the "#!" before passing the file to `llvm-nm exports` in the clang driver that looks weird.  the clang driver do not have functionality to export symbols from the files, it depend on the `llvm-nm exports`.   `llvm-nm exports` , the output as empty for  `llvm-nm exports AIXLinkerImportFile` is reason for us.
2. for us, we still prefer we previous implement the functionality in the llvm-nm as https://reviews.llvm.org/D158004?id=552000 instead of adding a new class AIXLinkerImportFile, it only effect `llvm-nm --export` , there is no other side-effect as MaskRay worry about.



  // Ignore AIX linker import files (these files start with "#!"), when
   // exporting symbols.
   const char *BuffStart = (*BufferOrErr)->getBufferStart();
   if (ExportSymbols && (*BufferOrErr)->getBufferSize() >= 2 &&
       BuffStart[0] == '#' && BuffStart[1] == '!')
     return SymbolList;



================
Comment at: llvm/include/llvm/Object/AIXLinkerImportFile.h:23-28
+// The class AIXLinkerImportFile, which is not a symbolic file at the moment.
+// The command 'llvm-nm -export-symbols' exports an empty symbol for AIX linker
+// import file that depends on the AIXLinkerImportFile. Furthermore, if we
+// decide to get symbols from AIX linker import file and derive
+// AIXLinkerImportFile from the SymbolicFile class, the llvm-nm tool will also
+// need to be updated accordingly.
----------------
jhenderson wrote:
> I don't really follow this comment. What are you trying to explain here?
> 
> Aside: I'd delete the blank line before and after it.
AIX linker import file contain imported symbols name in the file. 
we look it not symbolic file this moment , if we want to get the symbols from the AIX linker import file in future, we maybe implement the class as derive from  SymbolicFile class in future. not derived from Binary directly.


================
Comment at: llvm/lib/Object/AIXLinkerImportFile.cpp:19
+
+AIXLinkerImportFile::~AIXLinkerImportFile() = default;
+Expected<std::unique_ptr<AIXLinkerImportFile>>
----------------
jhenderson wrote:
> Please add a blank line between method defninitions.
> 
> That being said, why do you need an explicit destructor, rather than just not having the function present?
without the explicit destructor , there will be compiler error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004