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

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

tkoeppe added inline comments.


================
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);
----------------
tkoeppe wrote:
> jhenderson wrote:
> > 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.
> Hm, an interesting wrinke:
> 
> According to https://github.com/llvm/llvm-project/commit/d67c1e129bfd6afa756fed7e3988110bf3d70260#diff-38b4803de5adf661f3d9c7e5e1602a1f097bb79a7828cf31b9c629eb3af94fd1R18, it seems that we actually want to _preserve_ the flag, and not allow it to be changed by the user. That seems unfortunate.
> 
> What do you think, can we reverse that decision?
And you're right that the same numeric value is used for other flags in other architectures!

I think I need some overall guidance here:

* We currently _preserve_ some so-called "OS flags".
* Several different archs use the same numeric value for one of their OS flags.
* But now we actually _don't_ want to preserve the "LARGE" flag but make it configurable.

Is that even a workable idea? It'd potentially break existing users who have been silently propagating the flag (even if that's unlikely). But we really need a way to set this flag on our own custom sections that we assemble by other means.

There seems to be a limitation of the current objcpy interface: it doesn't explicitly allow distinguishing "set", "clear", and "leave as is"; rather the set of volatile/preserved flags is somehow nebulously hardcoded, and there doesn't seem an evolutionary path to promote a flag from preserved to volatile.

What do you reckon?


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

https://reviews.llvm.org/D153262