[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);
----------------
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?


================
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);
----------------
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.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:726-727
+        if (SR.NewFlags) {
+          uint16_t EMachine =
+              Config.OutputArch ? Config.OutputArch->EMachine : Obj.Machine;
+          if (Error E = setSectionFlagsAndType(Sec, *SR.NewFlags, EMachine))
----------------
jhenderson wrote:
> Rather than repeat this code, would it make more sense to pass in the final value from outside the function?
Wait, yes, we don't need to compute this at all, since `Obj.Machine` is already set to the right value on L624, so we can just use that directly. PTAL.


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

https://reviews.llvm.org/D153262