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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:89
+      return createStringError(errc::invalid_argument,
+                               "section flag SHF_X86_64_LARGE can only be used "
+                               "with x86_64 architecture");
----------------
MaskRay wrote:
> This can be tested when you add a non-EM_X86_64 test.
> 
> `section flag 'large' ...` may be more appropriate since for a non-EM_X86_64 machine, the flag is not called `SHF_X86_64_LARGE`.
At the moment, the "large" option is only valid for X86_64, hence there's a one-to-one mapping of the option name to the flag name, and it's therefore safe to use the term on non-X86-64 machines (because "large" means "set the SHF_X86_64_LARGE" flag specifically).

As stated earlier, I don't think mentioning a command-line term in a library is a great idea, because another tool might use another mechanism to configure `SecLarge`. We could pass in a mapping of flag names (e.g. `SecLarge`) to their textual representation (e.g. `large`) so that the calling code could configure it appropriately, but that seems a bit overkill at the moment.


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

https://reviews.llvm.org/D153262