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

Haowei Wu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 7 17:16:27 PST 2022


haowei marked an inline comment as done.
haowei 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.
----------------
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?


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:535
+  }
+  if (PHdrs.get().size() > 0) {
+    Expected<const uint8_t *> DynStrPtr =
----------------
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.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:573
+  Expected<Elf_Phdr_Range> PHdrs = ElfFile.program_headers();
+  if (!PHdrs) {
+    return PHdrs.takeError();
----------------
mcgrathr wrote:
> What's the purpose of fetching and checking PHdrs here when you just use toMappedAddr (which I presume only works if the phdrs were present to tell it what to do)?
It was a check before toMappedAddr since this function will fail if program headers are empty. I will remove the check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769



More information about the llvm-commits mailing list