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

Wed Jun 28 16:36:31 PDT 2023

MaskRay added a comment.

The GNU objcopy feature request is https://sourceware.org/bugzilla/show_bug.cgi?id=30592

================
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);
----------------
jhenderson wrote:
> tkoeppe wrote:
> > 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?
> First up, and most importantly, we should aim for compatibility with GNU objcopy. As this is a new feature (at least, I'm assuming it's not available in GNU objcopy yet), you should discuss with them a) the flag name in the command-line (i.e. "large") and b) how this should/should not be preserved.
> 
> Asssuming we implement this feature as you are planning, I think it's reasonable to do the following:
> 1) Flags that are processor specific should be "preserved" when the machine doesn't have that numeric flag value as a known OS flag. It's unclear what should happen if you are trying to change the machine type in this case (in other words, is it the new or old OS that's important?).
> 2) Flags that are processor specific that are known for the relevant machine should NOT be preserved. However, we would need to consider backwards compatibility here: if a user is currently modifying the flags of a section that has the SHF_X86_64_LARGE flag, if we no longer preserve it by default, they will suddenly find the flag is lost when they update their version of llvm-objcopy. This seems a little unfriendly. The alternative is to preserve the flag if it exists, or add it if it doesn't and is requested, but this means users won't have the ability to drop the flag. Again, it may be worth discussing with the GNU developers. A migration path might be to introduce a new command-line option to specifically remove flags, and then leave some legacy behaviour in --set-section-flags to maintain the current behaviour there (whilst still allowing that option to add the flag, as suggested).
> 
> @MaskRay, any thoughts on this?
I think GNU objcopy's current behavior is to preserve all `SHF_MASKOS | SHF_MASKPROC` bits. For `EM_X86_64` objects, it will make sense to not preserve `SHF_X86_64_LARGE` (if "large" is not specified with --set-section-flags, drop the section flag).

I think our behavior should behave like this as well. If a non-x86-64 object has the flag, we should preserve it, e.g. `SHF_MIPS_GPREL == SHF_X86_64_LARGE`.

I don't worry about compatibility here. The default `-mlarge-data-threshold=` is 65536 and it's difficult to get SHF_X86_64_LARGE sections with GCC (Clang doesn't emit this yet). I do not find real use cases of `-mlarge-data-threshold=`  on https://sourcegraph.com/search .

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test:97
     Type:            SHT_RELA
     Info:            .baz

----------------
Consider adding a section with `SHF_X86_64_FLAG` and calling objcopy --set-section-flags on it but without specifying "large". Test that the flag is cleared.

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

https://reviews.llvm.org/D153262