[PATCH] D116769: [ifs] Allow llvm-ifs to generate text stub from elf stub

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:529
+  }
+  // Fallback to use offsets from .dynamic as .dynsym was not found
+  // from the section headers.
----------------
haowei wrote:
> haowei wrote:
> > mcgrathr wrote:
> > > I would actually fall back here only if there are no section headers at all.  If there are any section headers at all but no SHT_DYNSYM section, then don't look further.  i.e. return failure after the `for` loop above inside the `if`.
> > > 
> > If it fail when .dynsym is absent, test https://github.com/llvm/llvm-project/blob/ebd8eee62a431a6744c3f187fcda58e5dea08499/llvm/test/tools/llvm-ifs/binary-read-add-soname.test will fail (there are 7 similar tests) as there is no .dynsym at all, .dynstr (though empty) is pointed by .dynamic . I believe either fallback strategy won't cause any issues in the field.
> > 
> > If an ELF has no symbols (which is unlikely), should it have no .dynsym section or should it have an empty .dynsym?
> I uploaded a patch using your suggested fallback logic and modified affected tests (and deleted one as it not achievable).
No SHT_DYNSYM section could ever be empty, because index 0 is reserved and must contain a stub entry.

I've never seen a linker that produces a .dynamic at all without having a .dynsym.  `clang -nostdlib -shared -o /tmp/foo.so -xassembler /dev/null` with a toolchain that uses current LLD by default produces a .dynsym that has nothing but the null entry.  With some current or past GNU linkers, that degenerate case would still produce either some local STT_SECTION symbols in .dynsym (which really never had a reason to be there), or always export some symbols like `_end` or `_edata` in .dynsym so it was never empty of symbols even when it probably should have been.

So I think basically, if there are section headers at all and this is a real-world ET_DYN file of any plausible normal sort of provenance that development tools were ever really expected to handle, then finding no SHT_DYNSYM at all is more likely to indicate a corrupted file or tool bug than a degenerate ELF file that's actually harmless.  Likewise, if you're looking at PT_DYNAMIC data via phdrs instead, then once you've found the dynamic section then if it doesn't include all of DT_{SYMTAB,SYMENT,STRTAB,STRSZ} then it's more likely to be a corrupt image than a valid degenerate ELF file with no dynamic symbol table.  Even a static PIE that has no dynamic relocs that require referring to a symbol table at all (only simple fixups) will in practice still have a symbol table with the null entry at index 0 and an empty string table but with an address and headers as if it had a length other than zero.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:535
+  }
+  if (PHdrs.get().size() > 0) {
+    Expected<const uint8_t *> DynStrPtr =
----------------
haowei wrote:
> mcgrathr wrote:
> > This is inconsistent with the Shdrs case above wrt whether or not to use bool coercion for  a zero check.
> > I personally like avoiding the implicit coercion and spelling it out like you did here.
> > But I think it's most important to be consistent in style for all the integer cases (using implicit bool coercion for pointer nullptr checks is canonical style in C++, but for integers it's less clear).
> > 
> Thanks for pointing this out. I will delete this check since toMappedAddr will fail with an empty program headers. I can re-add the check if you think it is necessary to add a more detailed error message when the program header is empty.
I think whatever form of "the memory access didn't work" error it produces is fine without being any more specific about why it failed for this case than it is for any other case.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769