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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not really dug into this patch at this point, but one high-level thing I wanted to highlight: it looks like you're doing a lot of reading of the object file to interpret the contents, in ways that  tools like llvm-readelf and llvm-objdump already do. Do you actually need this much additional code as a result? It might need some refactoring, but it seems like we're otherwise reinventing the wheel...!



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:229
     size_t DynStrIndex = DynTab.Content.addAddr(DT_STRTAB, 0);
+    DynTab.Content.addValue(DT_STRSZ, DynSym.Size);
     for (const std::string &Lib : Stub.NeededLibs)
----------------
This feels like it should be its own independent fix, with corresponding testing etc.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:529
+  }
+  // Fallback to use offsets from .dynamic as .dynsym was not found
+  // from the section headers.
----------------
mcgrathr wrote:
> 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.
FWIW, we have a proprietary linker that sticks dynamic data like the dynsym and dynstr inside a combined section, pointed to by dynamic tags contained in a (normal) dynamic section. As such, in our case, we would have a .dynamic, but no .dynstr or .dynsym (in the section header table). We don't use this tool however, and I know our case is unusual, so don't feel a need to handle it necessarily - just highlighting that it is a possible case (just not with, to my knowledge, mainstream linkers).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769