[PATCH] D153262: [llvm-objcopy] --set-section-flags: allow "large" to add SHF_X86_64_LARGE

[Thomas Köppe via Phabricator via llvm-commits]

tkoeppe marked an inline comment as done.
tkoeppe added a comment.

In D153262#4470305 <https://reviews.llvm.org/D153262#4470305>, @jhenderson wrote:

> clang-format is complaining in the pre-merge checks. Please make sure you have run it on all your new code.

Sorrry, I thought I had done that, but I must have missed a final pass -- fixing in the next revision!



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:706
         const SectionFlagsUpdate &SFU = Iter->second;
-        setSectionFlagsAndType(Sec, SFU.NewFlags);
+        uint16_t Arch = Config.OutputArch.value_or(MachineInfo());
+        if (Error E = setSectionFlagsAndType(Sec, SFU.NewFlags, Arch.EMachine))
----------------
jhenderson wrote:
> jhenderson wrote:
> > tkoeppe wrote:
> > > jhenderson wrote:
> > > > I don't think this is the correct way of finding the machine architecture at this point in the code. If `Config.OutputArch` isn't set, the result will be a value that doesn't support EM_X86_64_LARGE, even though the actual object being created might be an X86_64 object (because it was already one). Take a look at how `OutputElfType` is created in `executeObjcopyOnBinary` for example.
> > > Aha, I'm now using `Config.OutputArch ? Config.OutputArch->EMachine : Obj.Machine`, PTAL.
> > Looks better. Please make sure you have testing that shows the behaviour with and without `Config.OutputArch` set (both to a supporting architecture and not) and with and without an `Obj.Machine` that supports the new flag. I believe that means you need 6 tests?
> > 
> >   - Config.OutputArch not set, Obj.Machine is EM_X86_64.
> >   - Config.OutputArch not set, Obj.Machine is not EM_X86_64.
> >   - Config.OutputArch set to x86-64, Obj.Machine is EM_X86_64.
> >   - Config.OutputArch set to x86-64, Obj.Machine is not EM_X86_64.
> >   - Config.OutputArch set to other machine, Obj.Machine is EM_X86_64.
> >   - Config.OutputArch set to other machine, Obj.Machine is not EM_X86_64.
> > 
> Pinging this comment about testing.
Yes, will do, I just wanted to get agreement on the overall plan first!


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

https://reviews.llvm.org/D153262