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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Could you add some tests please?



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:72
+static Error getNewShfFlags(SectionFlag AllFlags, uint64_t &NewFlags,
+                            uint16_t EM) {
+  NewFlags = 0;
----------------
`EM` isn't a particularly self-descriptive name. Let's call it `EMachine` like it's called in other places. Same goes throughout the changes.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:89
+      return createStringError(
+          object_error::invalid_file_type,
+          "section flag 'large' can only be used with x86_64 architecture");
----------------
I'm not convinced this is the right error value - you could argue that the input flag type is invalid, rather than the file type, so this would end up being `errc::invalid_argument`, I think.


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


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


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:86
+  if (AllFlags & SectionFlag::SecLarge)
+    NewFlags |= ELF::SHF_X86_64_LARGE;
   return NewFlags;
----------------
rnk wrote:
> I think the thing to do is to:
> - Pass in the elf machine
> - Make NewFlags an output parameter (out parameters seem more consistent with local code than ErrorOr)
> - Return an Error instead
> - While handling the generic SecLarge flag, check the ELF machine, produce an error for non x86_64 machine values
> - Propagate that up out of setSectionFlagsAndType, whose caller can return Error
I would prefer this to return an `Expected<uint64_t>` with the new flags. It's easier to work with than an output parameter + Error return. Otherwise, I agree with the rest of these comments (which I see you've already addressed).


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

https://reviews.llvm.org/D153262