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

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

tkoeppe added a comment.

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

> One more thing: you need to add the new flag value to the list of supported flags and their meaning in the llvm-objcopy command guide (llvm/docs/CommandGuide).

Done.



================
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))
----------------
tkoeppe wrote:
> 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!
I've added a new file with a set of tests now. Notes:

* When the target is non-EM_X86_64, it is an error to set the "large" flag (this is tested).
* When not setting a target, the preservation or lack thereof of the "large" flag is tested by rename-section-flag-osproc-mask.test.
* When converting from EM_X86_64 to non-EM_X86_64, any present "large" flags are reinterpreted to the target's interpretation of that flag value.
* When converting from non-EM_X86_64 to EM_X86_64 and `--set-section-flag` is used, the "large" flag is set based on whether it is specified on the command line. When `--set-section-flag` is not used, existing flags are reinterpreted.


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

https://reviews.llvm.org/D153262