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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked an inline comment as done.
jhuber6 added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:506
+  if (Sec.sh_type != ELF::SHT_HASH)
+    return createError("invalid sh_type for hash table, expected SHT_HASH");
+  Expected<Elf_Shdr_Range> SectionsOrError = sections();
----------------
jhenderson wrote:
> jhenderson wrote:
> > I'm looking at this error and wondering if it would be useful to print the actual type (possibly just as a number), so that if a user encounters this area, it might be slightly easier to find the problem. Up to you whether you think it's useful or not though.
> Just pinging this point in case you missed it, as you've not responded one way or another (even with just a "no I don't think it's useful).
I don't think it's useful and this error message is very similar to other messages.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309