[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:
> > 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.


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


================
Comment at: llvm/include/llvm/Object/ELF.h:527-528
+                       " has invalid sh_offset: " + Twine(Sec.sh_offset));
+  if (Sec.sh_size < sizeof(Elf_Hash) + sizeof(Elf_Word) * HashTab->nbucket +
+                        sizeof(Elf_Word) * HashTab->nchain)
+    return createError("section " + getSecIndexForError(*this, Sec) +
----------------
If HashTab's size is 0, then you can't read the nbucket or nchain field, so this will result in reading outside the section. You'll need to split this into a check to make sure it's big enough for the nchain/nbucket fields, and then a separate one to make sure the arrays fit.


================
Comment at: llvm/include/llvm/Object/ELF.h:543-544
+    if (I * sizeof(Elf_Sym) >= (*SymTabOrErr)->sh_size)
+      return createError("symbol [index " + Twine(I) + "] is out of range: " +
+                         Twine((*SymTabOrErr)->sh_size / sizeof(Elf_Sym)));
+    if (SymTab[I].st_name >= StrTab.size())
----------------
It's not clear in the final message what the bit after the colon means. I'd add some additional words clarifying that it is the number of symbols.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:916
+  Expected<ELFObjectFile<ELF64LE>> ElfOrErr = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
+FileHeader:
----------------
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.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:995
+
+// Test for getSymbolFromHashTable: check that is fails with an invalid symbol.
+TEST(ELFObjectFileTest, SymbolFromHashTableTestSymIndex) {
----------------
I was slightly confused by first reading of this comment, so I'd add the "index" to be specific.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:1035
+
+// Test for getSymbolFromHashTable: check that is fails with an invalid table.
+TEST(ELFObjectFileTest, SymbolFromHashTableTestName) {
----------------
"is" -> "it" here and in all the other comments.

Not sure which "table" is referred to in this comment. Probably should be "check that it fails if a symbol has an invalid st_name."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309