[PATCH] D13055: [ELF2] Handle -m option

[Denis Protivensky via llvm-commits]

denis-protivensky added a comment.

@rafael, thanks for the patch, but I'd prefer to leave the getElfMachineType method since it allows to simplify the interface of ELFData class and ObjectFile/SharedFile. It removes bunch of methods from the .cpp file as well.


================
Comment at: ELF/Config.h:14
@@ -13,1 +13,3 @@
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Object/ELF.h"
+#include <cstdint>
----------------
rafael wrote:
> You only need Support/ELF.h
Agreed.

================
Comment at: ELF/Config.h:15
@@ -14,1 +14,3 @@
+#include "llvm/Object/ELF.h"
+#include <cstdint>
 
----------------
rafael wrote:
> Not used
> 
I introduced the variable of `uint16_t` type, so this header is used and the style guide doesn't prohibit adding headers to avoid hidden dependencies.

================
Comment at: ELF/Config.h:35
@@ +34,3 @@
+
+  ELFKind ElfKind = ELF64LEKind;
+  uint16_t EMachine = llvm::ELF::EM_X86_64;
----------------
rafael wrote:
> Can these two variables be left uninitialized? If not, please make them an explicit Optional<>. We really should not give EM_X86_64 or ELF64LE any special treatment.
Will make ElfKind optional and set EMachine to EM_NONE.

================
Comment at: ELF/Driver.cpp:18
@@ -17,2 +17,3 @@
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Object/ELF.h"
 #include "llvm/Support/FileSystem.h"
----------------
rafael wrote:
> Unnecessary.
I'll change this to ELFTypes.h because it contains `Elf_Ehdr_Impl` definition.

================
Comment at: ELF/Driver.cpp:44
@@ +43,3 @@
+  const char *Name;
+  ELFKind ElfKind;
+  uint16_t EMachine;
----------------
ruiu wrote:
> ElfKind -> ELFKind
This causes build errors with gcc, that's why I made it like that.

================
Comment at: ELF/Driver.cpp:67
@@ -48,2 +66,3 @@
 
-static std::unique_ptr<InputFile> createFile(MemoryBufferRef MB) {
+static uint16_t getElfMachineType(StringRef Object) {
+  using namespace object;
----------------
ruiu wrote:
> Passing an in-memory object as a StringRef feels a bit strange. Maybe you want to passs MemoryBuffer instead.
Will try.

================
Comment at: ELF/Driver.cpp:76-77
@@ +75,4 @@
+    return ((const Elf_Ehdr_Impl<ELF32BE> *)Object.data())->e_machine;
+  else
+    return ((const Elf_Ehdr_Impl<ELF32LE> *)Object.data())->e_machine;
+}
----------------
ruiu wrote:
> No need for `else` because the other path ends with return.
Ok.

================
Comment at: ELF/Driver.cpp:80
@@ +79,3 @@
+
+static inline void configureTarget(ELFKind ElfKind, uint16_t EMachine) {
+  Config->ElfKind = ElfKind;
----------------
rafael wrote:
> Drop the inline.
Is this something style-guide related or error-prone? Why to drop it?

================
Comment at: ELF/Driver.cpp:81-82
@@ +80,4 @@
+static inline void configureTarget(ELFKind ElfKind, uint16_t EMachine) {
+  Config->ElfKind = ElfKind;
+  Config->EMachine = EMachine;
+}
----------------
ruiu wrote:
> This function is probably too short to be defined as a function. Just write two lines of code where you call this function.
I'm against the code duplication in general. Even for two-liner, so I'd like to leave it here.

================
Comment at: ELF/Driver.cpp:96
@@ +95,3 @@
+    configureTarget(ElfKind, EMachine);
+    TargetDefined = true;
+    TargetSource = MB.getBufferIdentifier().data();
----------------
ruiu wrote:
> I'd remove this TargetDefined variable. Instead, I'd change the type of Config->ELFKind to Optional<>, and check for the existence of the value instead of a separate flag.
Good.

================
Comment at: ELF/Driver.cpp:103
@@ +102,3 @@
+    error(Twine(MB.getBufferIdentifier() + " is incompatible with " +
+                TargetSource));
+  return std::move(File);
----------------
ruiu wrote:
> Looks like TargetSource is used just for printing this error message. Can you remove that? I think a detailed error message is generally good, but this error message is used rarely, and probably that wouldn't match the cost of having another variable here. Then we can keep createELFFile to take only one argument just like before.
That was my original design, but @rafael insisted we need this to avoid test regression. See comments for `incompatible.s` test below.

================
Comment at: ELF/Driver.cpp:116
@@ -56,3 +115,3 @@
   if (Magic == file_magic::elf_shared_object)
-    return createELFFile<SharedFile>(MB);
+    return createTargetELFFile<SharedFile>(MB, TargetDefined, TargetSource);
 
----------------
ruiu wrote:
> Let's keep the original function name.
Ok.

================
Comment at: ELF/Driver.cpp:121
@@ +120,3 @@
+
+static void applyEmulation(StringRef Emul) {
+  for (auto E : Emulations) {
----------------
ruiu wrote:
> Move the definition of Emulations inside here as a static local constant because only this function uses that.
Good point.


http://reviews.llvm.org/D13055