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

Wed Jun 28 01:18:08 PDT 2023

jhenderson 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:
> 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?

================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:118-121
+  if (auto NewFlags = getNewShfFlags(Flags, EMachine); !NewFlags)
+    return NewFlags.takeError();
+  else
+    Sec.Flags = getSectionFlagsPreserveMask(Sec.Flags, *NewFlags);
----------------
tkoeppe wrote:
> jhenderson wrote:
> > I'll be honest, I don't consider this code readable. I'd prefer the `NewFlags` initialization to be outside the if, as per the inline edit. Also, the use of `auto` hdies that this is an `Expected` return, which doesn't seem desirable.
> I can replace the `auto` with the proper type, which is definitely an improvement.
> 
> I'd say keeping the scope small is generally a win, since there's less one has to keep in mind for longer than necessary. But if you strongly prefer your style, we can of course use that.
I'm not convinced you have to keep anything in mind. Here's my rationale:
1) A variable that isn't used has no impact on the later code (obviously), so it doesn't matter if it is still in scope.
2) A variable that is used later needs to be available, so it obviously still needs to be in scope.
3) A variable that is redeclared later on will result in a compiler error. This will suggest that you either need to start using the existing variable (i.e. transition it from 1) to 2)) or to give the new one (or the old one) a different name.

The only cases I can think of where an existing variable could trip you up are:
a) when you create a new variable in the same scope, but forget to give it a type, and therefore accidentally reuse an existing variable.
b) when you create a new variable in an inner scope, and therefore start hiding the existing variable.
In the first of these two cases either it doesn't matter - if the variable wasn't used after this point, then overwriting it isn't an issue, whilst if the variable was used after this point, we're not in a position where limiting scope would be relevant. In the second case, at least with Visual Studio you can have warning levels turned up to warn you about this, but regardless, you don't impact the outer variables behaviour at all.

(Sorry, that ended up being a longer thought process than I intended!)

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

https://reviews.llvm.org/D153262