[PATCH] D131309: [ELF] Add ability to get a symbol by name from the hash table section

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:505-516
+  if (Sec.sh_type != ELF::SHT_HASH)
+    return createError("invalid sh_type for hash table, expected SHT_HASH");
+  Expected<Elf_Shdr_Range> SectionsOrError = sections();
+  if (!SectionsOrError)
+    return SectionsOrError.takeError();
+
+  auto SymTabOrErr = object::getSection<ELFT>(*SectionsOrError, Sec.sh_link);
----------------
jhuber6 wrote:
> jhenderson wrote:
> > jhuber6 wrote:
> > > jhenderson wrote:
> > > > jhuber6 wrote:
> > > > > jhenderson wrote:
> > > > > > All these checks still need test cases.
> > > > > Checking these errors explicitly is excessive. This isn't any new logic so it should already be covered by existing tests. I added one for calling this with an incorrect section but the other error paths are already covered as far as I'm aware.
> > > > I'm not suggesting the called code itself needs additional testing, merely testing that shows that if the called code reports an error, the error is correctly handled and returned up the call stack. The new if statements //are// new logic. As things stand, there is zero testing for those statements.
> > > I'll add a test for an invalid `st_link`. That should be good enough to assert that this part functions properly. I think it's safe to assert that getting a section from an index is sufficiently tested.
> > (I assume you meant sh_link). That will cover the `getSection` if statement, but it doesn't cover the `sections()` if statement or the `getStringTableForSymtab`: every conditional should have at least one test case that triggers both the true and false cases, or you haven't fully tested the if.
> > 
> > Putting it in other terms, if things were easier to mock, I'd suggest mocking out the return values of each of those functions, so that in one case they produced an error and in the other case they produced a valid value that can be used later on, and then having test cases that used both mocked versions. In practice, what you have to do instead is craft some invalid ELF to trigger the error. I don't care how you achieve that, as long as the respective functions trigger errors.
> At this point half of the existing ELF runtime test is going to be this one feature. I personally don't see the utility in testing errors that already have coverage somewhere else. If we already know calling some function with an invalid ELF will return the correct error I don't see why repeating that same pattern elsewhere warrants extra tests since none of this logic is unique or new.
Most of ELF.h is tested via lit tests rather than unittests, so it's a bit of a mischaracterisation that this feature will be half the runtime tests.

If you put a code coverage tool on this function, you wouldn't get 100% coverage without testing both halves of the ifs in question. A lot of work has gone into making sure that many/most/all of these sorts of functions have 100% coverage, both for the positive and the negative cases. That means testing needs to trigger every case where an error is handled (not just where it is reported) to show that the error handling is correct and that the error is appropriately propagated up the call stack. Note that I'm not arguing this for the sake of it: bugs can and do happen even in simple looking code around error handling.

I'm not budging on this point, because I am not willing to accept only partial test coverage, when it is relatively straightforward to add said coverage. I think you'll find that this is the same for others who regularly work on and review code in this area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309