[PATCH] D61767: [llvm-elfabi] Emit ELF header and string table section

[Haowei Wu via Phabricator via llvm-commits]

haowei added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:113
+  using Elf_Addr = typename ELFT::Addr;
+  using Elf_Dyn = typename ELFT::Dyn;
+
----------------
grimar wrote:
> You can use `TYPEDEF_ELF_TYPES(ELFT)` instead of adding these usings.
I think this is only place in this change to use so many "using" statements. It is not very cost efficient to define a  marco for this case.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:148
+      if (Sec->NoBits)
+        Align = 1;
+      if (Prev == nullptr)
----------------
grimar wrote:
> Why nobits sections are special?
> 
> Also I don't understand:
> 1) There are just 2 sections currently: `ShStrTab` and `StrTab`. Why do you need the code for `NoBits`?
> 2) Why can't you set `Align = 1` from start for them instead of having `Sec->Align ? Sec->Align : 1;` logic?
Most part of this change was authored by my previous co-workers. I don't know the specific reason why NoBits are handled here, but I could guess the code for NoBits are reserved in case the stub SO generated from elfabi would need any sections that has NoBits attribute. I agree it is not useful now as the following change do not use NoBits sections as well.

For align, the sh_addralign of .shstrtab and .dynstr was set to to 0 in this change. If I recall correctly both 0 and 1 are acceptable values for sh_addralign if the section does not have alignment constraints. Since alignTo function require "Align" to be non-zero, I guess that is why `Sec->Align ? Sec->Align : 1` exists. I have change the sh_addralign to 1 for the string tables to avoid it.





================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:561
+    Stream << FileReadError;
+    Stream << " when trying to open `" << FilePath << "` for writing";
+    consumeError(std::move(FileReadError));
----------------
grimar wrote:
> You don't have a test for this I think.
Emmm, I tried locally on a linux machine and it seems to be difficult to put it in test case.
I have to create a file with same filename, change ownership and the permission so elfabi cannot modify it. And run llvm-elfabi to write output to this file. Is there any good way to trigger error from FileOutputBuffer?



================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:48
+           cl::desc("Manually set the DT_SONAME entry of any emitted files"),
+           cl::value_desc("name"));
+cl::opt<ELFTarget> BinaryOutputTarget(
----------------
grimar wrote:
> All above are unrelated formatting changes I think.
Yes. I just run clang-format on entire file. Can I keep them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767