[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.

yes, your understand is correct.  but I need to clarify following.

> The question I have though is why should we special-case import files specifically for --export-symbols?



  since the Import file is input  in the clang command, there is another solution to  identify the Import File in the clang driver and not pass the Import File to `llvm-nm --export-symbols` , after compare with the solution in the patch. we prefer the solution in the patch, if user input the random File for  `llvm-nm --export-symbols` , we  put an error message to "invalid object file`, but for the Import symbol file, we just output none of export symbol. 
  
   

> I'm thinking that they should actually be another kind of Binary file, like COFFImportFile.  It would then fit more naturally into the framework that already exists, whereas the proposed patch feels like a sticking plaster to cover a specific edge-case. If this is causing a major issue, I'm okay with this patch landing, but I'd like a TODO added to the code indicating that it is temporary and should be  replaced with something more appropriate like AIXImportFile or similar in the near future.



  "The import file is not another kind of binary file (refer to: https://www.ibm.com/docs/en/aix/7.3?topic=l-ld-command#ld__a3119106d__title__1, section Import and Export File Format (-bI: and -bE: Flags)). I don't think we will obtain any information from the AIX import file in the future. Therefore, there's no need to add a class and API for it. If, in the future, we do want to retrieve information from the AIX import file, it might be a good idea to implement a class like 'AIXImportFile,' as per your suggestion.

For now, we can simply disregard the AIX import file instead of outputting the error 'invalid object file.'  'llvm-nm --export-symbols.'  I agree with your perspective that this is somewhat akin to a 'sticking plaster' solution to cover a specific edge case."



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2267-2270
+  const char *BuffStart = (*BufferOrErr)->getBufferStart();
+  if (ExportSymbols && (*BufferOrErr)->getBufferSize() >= 2 &&
+      BuffStart[0] == '#' && BuffStart[1] == '!')
+    return SymbolList;
----------------
jhenderson wrote:
> Should this be limited to AIX or similar in some way? Certainly, the comment should highlight that these import files are specific to AIX, as the format is not a generic format that all systems follow. Something like "Ignore AIX import symbol files (these files start with "#!"), when exporting symbols."
> 
> I'd also strongly consider moving the identification code into the `createBinary` tree. See my out-of-line comment for more detail.
it only limit to AIX Import symbol File. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004