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

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

tkoeppe marked 3 inline comments as done.
tkoeppe added a comment.

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

> Could you add some tests please?

Could you please advise how to add an objcpy test that only runs under x86_64?



================
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");
----------------
jhenderson wrote:
> 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.
Yes, makes sense, changed!


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


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:86
+  if (AllFlags & SectionFlag::SecLarge)
+    NewFlags |= ELF::SHF_X86_64_LARGE;
   return NewFlags;
----------------
jhenderson wrote:
> 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).
Sure thing, changed to `Expected`.


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

https://reviews.llvm.org/D153262