[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:
> jhenderson wrote:
> > 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).
> I am slightly lean a bit more with the fallback logic in diff3 (if .dynsym is not found, fallback to use program header and pointers from .dynamic) instead of  exit early when .dynsym is not found.  I bet there will be other elf tools in the wild that may generate ELF files with not common sections arrangements and it may help to avoid hassles in the future. What do you think?
SGTM. The suggestion to bail early was based on the idea that it might be a sign of corrupt input rather than anything else in practice. Even just knowing that there exists any linker that produces output that doesn't match the assumptions I stated is reason enough to be more accepting of any input we can find a way to work with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769