[PATCH] D60324: [llvm-objcopy] Add switch to allow removing referenced sections

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:432
     SectionIndexTable = nullptr;
   if (ToRemove(SymbolNames))
+    if (AllowBrokenDependencies)
----------------
MaskRay wrote:
> There is a `-Wdangling-else` warning.
Is that with the latest version of this patch? There's no `else` anywhere around here now, so I assume it can't be.


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:13
+def allow_broken_dependencies
+    : Flag<["-", "--"], "allow-broken-dependencies">,
+      HelpText<"Allow llvm-objcopy to remove sections even if it would leave "
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > Not strong opinion here, but `allow-broken-dependencies` looks like a bit too long option name.
> > > Will `allow-broken-deps`/`allow-broken-links` sound better?
> > I don't like abbreviations in switches, because I don't know what "deps" stands for. However, `allow-broken-links` is reasonable. I was thinking this switch could also be used for other relationships via sh_info, but I don't know of any yet where this is needed.
> I also did not find any valid use case.
> I.e. looking at the table of how sh_info can be used (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html),
> it does not seem that it is fine to allow broken dependencies there perhaps. 
Theoretically it could affect platform/OS-specific sections (which would indicate their special handling with SHF_INFO_LINK), but I don't think llvm-objcopy correctly handles them anyway.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60324