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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry, I've been trying to focus on other things for the majority of the last few days, as LLVM reviewing has been taking up too much of my worktime recently.

In D158004#4599547 <https://reviews.llvm.org/D158004#4599547>, @hubert.reinterpretcast wrote:

> This is causing a major issue for users linking shared objects using Clang on AIX; yes. Thanks for the understanding. I have some doubts about implementing further functionality for an `AIXImportFile` though: It is already human-readable. Also, it seems (to me) inappropriate to model is as a `Binary` because it is text.

Re. the bit about using `Binary`: technically any file on disk is a binary file, since even text is just a series of binary data (it just happens to be understandable as ASCII/UTF-8/... etc). In fact, Binary.h already references things like TAPI files, which are text-based - see the `ID_*` enum in it. The key thing is that the `createBinary` method recognises the file format (via the `identify_magic` function).

Looking at how classes inherit from `Binary`, it looks like you wouldn't need to implement anything special for `AIXImportFile`. I think the steps are simply:

- Add an `ID_` entry in the enum in Binary.h.
- Add a small class that inherits from Binary.h. This class wouldn't actually need to do anything else, as far as I can tell, other than define a constructor/create method that sets the ID type in the base class.
- Update `identify_magic` in Magic.cpp and the corresponding `file_magic` enum in Magic.h to support the `#!` sequence.
- Add an `isAIXImportFile` method to `Binary` that can be used to see if an arbitrary `Binary` object is of the relevant sub-class type. This bit is probably optional.

Looking at the code, if you went with this approach, you wouldn't need to change llvm-nm at all, I think (the testing you've added would still be fine though). In fact, I'd go as far as to say that the code is designed to work like that, rather than special casing other file formats. The `createBinary` call in `dumpSymbolNamesFromFile` will produce an `AIXImportFile` object and then the immediately-following if/else if chain would result in returning an empty `SymbolList`. This would be much cleaner than the proposed patch, I think, as it avoids special-casing the import files. It's also not that much more complex overall. Finally, it would give you a place to enhance in the future, should you wish to (for example you could later turn it into a `SymbolicFile` that lists the imports when requested).



================
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;
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > DiggerLin wrote:
> > > 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. 
> > @jhenderson, do you have a suggestion on how to limit it in such a way? I was under the impression that the tools were designed to work in cross-compilation environments. A host-environment-based or build configuration property could be employed, but I think your input on the choice is needed.
> "Ignore AIX import symbol files (these files start with "#!"), when exporting symbols." 
> 
> 
> since the import symbol files only used by linker
> I am prefer to 
> "Ignore AIX linker import files (these files start with "#!"), when exporting symbols."
I changed my mind on this one, having looked at the `createBinary` and `identify_magic` functions - there's no risk of conflict with other known file formats, so it's safe to not restrict this to AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004