[PATCH] D140291: [llvm-objcopy] Use getNumberOfSymbols() instead of getRawNumberOfSymbols()

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D140291#4004584 <https://reviews.llvm.org/D140291#4004584>, @DaanDeMeyer wrote:

>> This sounds like an argument saying the existing code is correct, no?
>
> My bad, I switched the order of getRawNumberOfSymbols() and getNumberOfSymbols() around.

The summary still mentions "if the index is greater than or equal to getRawNumberOfSymbols()" which I also presume should reference the other function?

>> From reading COFFObjectFile::getNumberOfSymbols() and COFFObjectFile::getRawNumberOfSymbols(), the only difference I see would be if we're operating on an object file without a symbol table - where the former nicely returns 0 while the latter hits llvm_unreachable? What's the actual situation you've got at hand here?
>
> When running llvm-objcopy built from source on an AARCH64 EFI executable built from source without this patch, I get the following error:
>
>   ➜  llvm-project git:(main) build/bin/llvm-objcopy linuxaa64.efi.stub abc
>   build/bin/llvm-objcopy: error: 'linuxaa64.efi.stub': Invalid data was encountered while parsing the file
>
> I looked into the error, and the root cause of the error is that `getRawNumberOfSymbols()` returns `1` while `getNumberOfSymbols()` returns `0`, presumably due to a missing symbol table. Regardless of what's causing the missing symbol table, it seems prudent to not try to read symbols that don't exist, which is why I'm proposing the following patch.
>
> We know that the loop will only succeed if `getRawNumberOfSymbols() <= getNumberOfSymbols()`, so unless we want the failure to happen, it seems better to loop over `getNumberOfSymbols()` rather than `getRawNumberOfSymbols()`

Well the reasoning is slightly off here. If the symbol table is missing, `getRawNumberOfSymbols()` doesn't return 1, it should hit a `llvm_unreachable`, which means the compiler didn't even necessarily generate code for that case, so it could literally crash, return anything, etc. In a build with asserts enabled, it would be a hard fault directly at that point.

Anyway, the patch probably is correct that this should be changed into `getNumberOfSymbols()`. The current reasoning in the patch is quite hard to follow though - something like this would be more straightforward:

> `getRawNumberOfSymbols()` assumes that a symbol table does exist - while `getNumberOfSymbols()` does handle and tolerate the case when there's no symbol table at all. (For the cases when there is a symbol table, they do return the same value.)

That makes it much clearer - it's not about iterating over a bigger or smaller range of values, it's about the fact that we can't call `getRawNumberOfSymbols()` when there's no symbol table.

But it would be great to have a real live source binary to inspect - can you share one? Also the change _does_ need a testcase added/changed that triggers this case. After building, you can run e.g. `bin/llvm-lit -s path/to/llvm-project/llvm/test/tools/llvm-objcopy` which will run all the testcases. Most of these testcases run with minimal synthetic input generated from yaml files though - we don't generally want to commit full real binaries there.

So to generate a testcase, you need to look into whether a binary can be generated with LLVM's `yaml2obj`, place a minimal such input in `llvm/test/tools/llvm-objcopy/COFF/Inputs` and write a test file in `llvm/test/tools/llvm-objcopy/COFF` which uses that, which would trigger the issue before the fix is applied, and which runs correctly with the fix in place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140291