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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

> In D158004#4616926 <https://reviews.llvm.org/D158004#4616926>, @DiggerLin wrote:
>
>> 2. I 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. not sure what @hubert.reinterpretcast  and @MaskRay's opinion
>
> I think that @MaskRay's concern about the larger-scope effects of adding `#!` magic supports going with the targeted direction of special handling for `--export`.

I agree that https://reviews.llvm.org/D158004?id=552000 feels reasonable. `--export-symbols` seems to be an AIX-specific option.

  static std::vector<NMSymbol> dumpSymbolNamesFromFile(StringRef Filename) {
  ...
  
    // 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;



In D158004#4616434 <https://reviews.llvm.org/D158004#4616434>, @jhenderson wrote:

> In D158004#4616402 <https://reviews.llvm.org/D158004#4616402>, @MaskRay wrote:
>
>> In D158004#4616388 <https://reviews.llvm.org/D158004#4616388>, @jhenderson wrote:
>>
>>> In D158004#4616384 <https://reviews.llvm.org/D158004#4616384>, @MaskRay wrote:
>>>
>>>> Changing `llvm::identify_magic` to recognize all files starting with `#!` as `aix_linker_import_file` is not correct. Many can well be used as shebang for shell or other scripts.
>>>>
>>>>   case '#':
>>>>     if (Magic[1] == '!')
>>>>       return file_magic::aix_linker_import_file;
>>>>     break;
>>>
>>> How would you suggest that @DiggerLin proceeds then? The impact of misidentifying a file with a shebang as an AIX import file is that tools will no longer report "invalid object file" or something to that effect for such files, but will otherwise do nothing with them, so it seems relatively harmless to me.
>>
>> Sorry, I have analyzed the AIX use cases... I just noticed this patch and am concerned that `identify_magic` mischaracterizing files with shebang as AIX-specific would likely cause beyond-negligible negative impacts...
>> `identify_magic` in LLVMBinaryFormat is somewhat commonly used outside of llvm-project.
>
> Could you elaborate please what these "beyond-negligible negative impacts" are? I've highlighted the one behaviour impact that I'm aware of, which I think we can ignore as not really an issue.
>
> It seems to me like you're blocking this patch because the AIX import file magic happens to clash with some general text file contents. How else can such a file be identified?
>
> @DiggerLin/@hubert.reinterpretcast et al, what is preventing the change to clang to not call llvm-nm with import files?

(Sorry that I made a typo in a previous message. I mean I haven't carefully analyzed the AIX use cases.)

My reasoning is this: someone may call `llvm::identify_magic` to implement an emulator: check whether an file is an ELF/XCOFF/etc executable file or a script file with shebang.

  switch (llvm::identify_magic(...)) {
  case file_magic::elf_executable:
  case file_magic::elf_shared_object:
    // run
  case file_magic::xcoff_object_64:
    // run
  case file_magic::unknown:
    // check shebang
  }

It would be quite confusing that they now need to change `case file_magic::unknown:` to `case file_magic::aix_linker_import_file`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004