[PATCH] D23769: [ELF] - Implemented --oformat binary option.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Config.h:122
@@ -119,2 +121,3 @@
   ELFKind EKind = ELFNoneKind;
+  OutputFormat OFormat;
   uint16_t DefaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
----------------
ruiu wrote:
> This can be `bool OFormatBinary = false`.
Done.

================
Comment at: ELF/Driver.cpp:338
@@ +337,3 @@
+static OutputFormat getOutputFormat(opt::InputArgList &Args) {
+  StringRef S = getString(Args, OPT_oformat);
+  if (S == "binary")
----------------
ruiu wrote:
> This is technically not correct. If you pass --oformat "", it ignores the error. Please use `Args.getLastArg(OPT_oformat)` explicitly.
Fixed.

================
Comment at: ELF/Writer.cpp:1071-1072
@@ -1056,5 +1070,4 @@
 template <class ELFT> void Writer<ELFT>::assignAddresses() {
   uintX_t VA = Config->ImageBase + Out<ELFT>::ElfHeader->getSize() +
                Out<ELFT>::ProgramHeaders->getSize();
   uintX_t ThreadBssOffset = 0;
----------------
grimar wrote:
> ruiu wrote:
> > It seems to me that
> > 
> >   uintX_t VA = Config->ImageBase;
> >   if (!OFormatBinary)
> >     VA += Out<ELFT>::ElfHeader->getSize() +
> >                Out<ELFT>::ProgramHeaders->getSize();
> > 
> > is easier to understand.
> It is not only that place unfortunately. These sections are added to output sections as well and that brings more changes. I can update this patch to avoid 
> ```
> if (Config->OFormat == OutputFormat::Elf)
>       fixHeaders();
> ```
> just to show the amount of changes required.
Hmm, I was mistaken, this change should be enough. 
It is definetely possible to live without disabling fixHeaders() and changes at Writer.cpp(131).
Troubles I had were because of another changes I tried during development. And I was incorrect when was saying "These sections are added to output sections as well", I was thinking about PHDRS at that moment, but we do not write them in this patch, so should not care.

================
Comment at: ELF/Writer.cpp:1112
@@ +1111,3 @@
+template <class ELFT, class uintX_t>
+void setOffsets(OutputSectionBase<ELFT> *Sec, uintX_t &Off) {
+  if (Sec->getType() == SHT_NOBITS) {
----------------
ruiu wrote:
> This seems to set only one offset, so setOffset?
Done.


https://reviews.llvm.org/D23769