[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 6 inline comments as done.
jhuber6 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);
----------------
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.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:916
+  Expected<ELFObjectFile<ELF64LE>> ElfOrErr = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
+FileHeader:
----------------
jhenderson wrote:
> You might be able to avoid duplicating this string between some of the test cases, by having a factory function that constructs the string by having a prefix (which ends at the end of the section you want to modify) and a suffix (which starts immediately after it), and then an optional field that can be inserted in the middle. Essentially you could have lines like the following then in the different tests:
> ```
> makeYAMLWithInvalidDynsym("ShOffset: 9999");
> makeYAMLWithInvalidHash("ShSize: 1");
> ```
> or perhaps:
> ```
> makeYAML(DynsymPrefix, "ShOffset: 9999", DynsymSuffix);
> makeYAML(HashPrefix, "ShSize: 1", HashSuffix);
> ```
> Potentially, you could have that function construct the ELF and return the hash table too, to reduce the boiler plate.
The solution I had previously that modified the ELF in-place was more compact because I could iteratively make different field invalid using the same input. I could go back to that method, otherwise I think it's too much work to make some factory generator just for saving a few string constants in a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309