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

I'd just like to make sure I follow what is going on here:

1. On AIX, clang uses llvm-nm to produce a list of exported symbols for its command-line. It does this by calling `llvm-nm --export-symbols` on each input file to clang.
2. Import files can be passed on the command-line like regular object files. They are distinguished from regular objects because they start with `#!`.
3. Because of these two points, clang ends up passing import files to llvm-nm, which should be ignored.

I think I can buy that it should be llvm-nm's responsibility to ignore import files when using `--export-symbols`, rather than preventing clang from passing them to llvm-nm in the first place. If I understand it correctly, import files are essentially a list of symbols to import from shared objects. That means that conceptually by definition they don't have any exports. I can see that this patch implements that functionality. The question I have though is why should we special-case import files specifically for `--export-symbols`? 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.

Regardless of the decision on the above, the description in this patch could do with being reworked. My suggestion follows. I wouldn't include the quote in the final commit message, as it will get a bit verbose:

> On AIX OS, clang may use llvm-nm to export the symbols from all input files (see https://github.com/llvm/llvm-project/blob/515c435e378b243b1be3da1587c9e206055f2c32/clang/lib/Driver/ToolChains/AIX.cpp#L236). However, the clang command-line may include import files (identified by them starting with `#!`). `llvm-nm` previously reported "invalid object file" errors for import files, meaning that the clang driver would fail to link when import files are included this way.
>
> In this patch, llvm-nm is changed to ignore import files when the `--export-symbol` option, meaning that clang will now succeed in this case.
>
> For more information about AIX import files, see https://www.ibm.com/docs/en/aix/7.3?topic=l-ld-command





================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:55
 
+## Test ignoring the parsing an imported symbol file that  begins with #!
+# RUN: echo "#!" > imp.txt
----------------
hubert.reinterpretcast wrote:
> Minor nit: Grammar.
Also, add a "." at the end of the comment, and probably then best to quote the `#!` i.e. "begins with "#!"."

The term in the doc is "Import File", so you should probably use that instead of "imported symbol file". It also should reference AIX, since this isn't a generic thing. Something like:

```
## Test that llvm-nm ignores AIX import symbol files when using --export-symbols. These start with "#!".
```
(Make sure to reflow to an 80 column width).


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004