[PATCH] D153262: Add named section flag "large" to objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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).



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:104
        ELF::SHF_INFO_LINK) &
-      ~ELF::SHF_EXCLUDE;
+      ~(ELF::SHF_EXCLUDE | ELF::SHF_X86_64_LARGE);
   return (OldFlags & PreserveMask) | (NewFlags & ~PreserveMask);
----------------
Please make sure there is testing for this change.

There's also a potential issue here, if I'm not mistaken: on other architectures, there might be a flag with the same value as SHF_X86_64_LARGE, but which should be handled independently. You'll want a test case for this to show what should happen.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:118-121
+  if (auto NewFlags = getNewShfFlags(Flags, EMachine); !NewFlags)
+    return NewFlags.takeError();
+  else
+    Sec.Flags = getSectionFlagsPreserveMask(Sec.Flags, *NewFlags);
----------------
I'll be honest, I don't consider this code readable. I'd prefer the `NewFlags` initialization to be outside the if, as per the inline edit. Also, the use of `auto` hdies that this is an `Expected` return, which doesn't seem desirable.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:726-727
+        if (SR.NewFlags) {
+          uint16_t EMachine =
+              Config.OutputArch ? Config.OutputArch->EMachine : Obj.Machine;
+          if (Error E = setSectionFlagsAndType(Sec, *SR.NewFlags, EMachine))
----------------
Rather than repeat this code, would it make more sense to pass in the final value from outside the function?


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:90
+          object_error::invalid_file_type,
+          "section flag 'large' can only be used with x86_64 architecture");
+    NewFlags |= ELF::SHF_X86_64_LARGE;
----------------
tkoeppe wrote:
> jhenderson wrote:
> > I don't have a good solution to this currently, but the use of the command-line term in a libary function is a little concerning. A future tool might use a different name for the `SHF_X86_64_LARGE` flag, which would then make this error non-sensical. We should avoid referring to the command-line term this deep. For now, you could probably get away with referring directly to the flag name "SHF_X86_64_LARGE", and we can then change it to something else (e.g. SHF_*_LARGE) if another machine gains support.
> I see, makes sense. I was thinking of giving the user a clue what they did wrong, and both the command line flag and this code are particular to `objcpy`, but I see your point. Changed.
Yeah, I get what your point is, but as this is a library, we can't assume all future users will have the same interface. You //could// do some more significant changes, if you wanted, which would allow us to get the "textual" version of the flag reported. One idea would be to simply pass the map of `SectionFlag` enum values to their textual versions into this method. Alternatively, a callback that takes a `SectionFlag` and converts it to the relevant form. I don't think it's necessary for this patch, but it's an option, if you want to improve the message.


================
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:
> > 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.



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

https://reviews.llvm.org/D153262