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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:72
+  ElfHeader.e_flags = 0;
+  ElfHeader.e_ehsize = sizeof(ElfHeader);
+  ElfHeader.e_phentsize = sizeof(typename ELFT::Phdr);
----------------
I'd use `typename ELFT::Ehdr` for consistency with lines below.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:113
+  using Elf_Addr = typename ELFT::Addr;
+  using Elf_Dyn = typename ELFT::Dyn;
+
----------------
haowei wrote:
> 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.
You are right. I thought we have it defined globally already (like `LLVM_ELF_IMPORT_TYPES_ELFT`), but it turns out it is only defined in `llvm-readobj\ELFDumper.cpp` currently.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:561
+    Stream << FileReadError;
+    Stream << " when trying to open `" << FilePath << "` for writing";
+    consumeError(std::move(FileReadError));
----------------
haowei wrote:
> 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?
> 
Perhaps I am missing something (I am not sure what the problem is).
We have a few tests in LLVM (e.g see LLD tests, like lld\test\COFF\thinlto-emit-imports.ll) that
do something very close to what you describe I think: they create a file, change permission and run tool on it:

```
; RUN: rm -f %t3.obj.imports
; RUN: touch %t3.obj.imports
; RUN: chmod 400 %t3.obj.imports
; RUN: not lld-link -entry:main -thinlto-index-only \
; RUN:     -thinlto-emit-imports-files %t1.obj %t2.obj %t3.obj \
; RUN:     -out:%t4.exe 2>&1 | FileCheck %s --check-prefix=ERR
; ERR: cannot open {{.*}}3.obj.imports: {{P|p}}ermission denied
```

Can you do something like that?


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf64be-ehdr.test:2
+# RUN: llvm-elfabi %s --output-target=elf64-big %t
+# RUN: llvm-readobj --file-headers %t | FileCheck %s
+
----------------
Use `--match-full-lines` too?


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf64le-ehdr.test:2
+# RUN: llvm-elfabi %s --output-target=elf64-little %t
+# RUN: llvm-readobj --file-headers %t | FileCheck %s
+
----------------
Use `--match-full-lines` too?


================
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(
----------------
haowei wrote:
> grimar wrote:
> > All above are unrelated formatting changes I think.
> Yes. I just run clang-format on entire file. Can I keep them?
It is better to avoid unnecessarily changes in patches. These changes are unrealated and the right way
to do this would be to format the entry file first and commit as NFC, that should not need a review in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767