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

[Rui Ueyama via llvm-commits]

ruiu 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;
----------------
This can be `bool OFormatBinary = false`.

================
Comment at: ELF/Driver.cpp:337
@@ -336,1 +336,3 @@
 
+static OutputFormat getOutputFormat(opt::InputArgList &Args) {
+  StringRef S = getString(Args, OPT_oformat);
----------------
isOutputFormatBinary

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

================
Comment at: ELF/Writer.cpp:267-268
@@ -263,2 +266,4 @@
                                                 : createPhdrs();
-    fixHeaders();
+    if (Config->OFormat == OutputFormat::Elf)
+      fixHeaders();
+
----------------
Do you have to not call this function?  I don't like to sprinkle OFormatBinary check everywhere. If it works with this function call, remove this check.

================
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) {
----------------
This seems to set only one offset, so setOffset?


https://reviews.llvm.org/D23769