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

Mon Dec 19 04:16:27 PST 2022

mstorsjo requested changes to this revision.
mstorsjo added a comment.
This revision now requires changes to proceed.

> The first thing we do in the for loop is call getSymbol() which checks if the index is greater than or equal to getRawNumberOfSymbols() and returns an error if that's the case. Given the above, it makes sense to loop over getRawNumberOfSymbols() instead of over getNumberOfSymbols() since the value of the former may differ from the latter if the symbol table could not be found.

This sounds like an argument saying the existing code is correct, no?

> This patch allows using llvm-objcopy to add sections to a aarch64 PE executable on x86-64. Before, it failed with an error indicating it failed to parse the PE binary.

What architecture you're running the tools on should really not matter at all here.

This patch is both lacking a proper explanation of what is going wrong currently and how this patch fixes it, and a testcase that could verify that the testcase doesn't regress later. Or even a link to an original bug report with the original issue so that someone else can verify and figure out what's going on.

>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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140291