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

Tue Jul 4 00:22:49 PDT 2023

jhenderson added a comment.

clang-format is complaining in the pre-merge checks. Please make sure you have run it on all your new code.

================
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))
----------------
jhenderson wrote:
> tkoeppe wrote:
> > jhenderson wrote:
> > > 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.
> > Aha, I'm now using `Config.OutputArch ? Config.OutputArch->EMachine : Obj.Machine`, PTAL.
> Looks better. Please make sure you have testing that shows the behaviour with and without `Config.OutputArch` set (both to a supporting architecture and not) and with and without an `Obj.Machine` that supports the new flag. I believe that means you need 6 tests?
> 
>   - Config.OutputArch not set, Obj.Machine is EM_X86_64.
>   - Config.OutputArch not set, Obj.Machine is not EM_X86_64.
>   - Config.OutputArch set to x86-64, Obj.Machine is EM_X86_64.
>   - Config.OutputArch set to x86-64, Obj.Machine is not EM_X86_64.
>   - Config.OutputArch set to other machine, Obj.Machine is EM_X86_64.
>   - Config.OutputArch set to other machine, Obj.Machine is not EM_X86_64.
> 
Pinging this comment about testing.

================
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);
----------------
MaskRay wrote:
> 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 .
All sounds good to me, thanks @MaskRay.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-flag-osproc-mask.test:30
 # X86_64-NEXT:   ]
+# X86_64-LARGE:        Name: .quz
+# X86_64-LARGE-NEXT:   Type: SHT_PROGBITS
----------------
Probably should have a blank line between the two blocks, for readability.

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

https://reviews.llvm.org/D153262