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

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/Config.h:35
@@ +34,3 @@
+
+  ELFKind ElfKind = ELF64LEKind;
+  uint16_t EMachine = llvm::ELF::EM_X86_64;
----------------
ruiu wrote:
> denis-protivensky wrote:
> > 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.
> Or you could define a new enum, ELFNoneKind and use the value as an indicator of absence of a valid value.
Better to have the optional, since defining one enum value used in only one case as a default looks like bad design.

================
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;
----------------
denis-protivensky wrote:
> ruiu wrote:
> > Passing an in-memory object as a StringRef feels a bit strange. Maybe you want to passs MemoryBuffer instead.
> Will try.
`MemoryBufferRef` doesn't have suitable methods to operate on bytes, so I'll in any case need to construct `StringRef` from `MemoryBufferRef` inside. No profit in changing the signature, especially for non-public method.

================
Comment at: ELF/Driver.cpp:103
@@ +102,3 @@
+    error(Twine(MB.getBufferIdentifier() + " is incompatible with " +
+                TargetSource));
+  return std::move(File);
----------------
ruiu wrote:
> denis-protivensky wrote:
> > 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.
> I feel like there must be some way to remove this parameter. Can we do this check not in this function but at Driver.cpp line 189?
Thought about that, but it would be worse: we'll have two logically equal checks of having ELFKind set in two different places. Moreover, we'll need to return `nullptr` value from the method and then check it in the loop. I don't feel it's worth doing.


http://reviews.llvm.org/D13055