[PATCH] D58234: [llvm-objcopy] Add --add-symbol

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol.test:76
+# ERR4: error: bad symbol value: 'xyz'
\ No newline at end of file

----------------
nit: add a newline


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:215
+  size_t NumFlags = Flags.size();
+  for (size_t I = 1; I < NumFlags; ++I)
+    if (Flags[I].equals_lower("global"))
----------------
evgeny777 wrote:
> rupprecht wrote:
> > Some things that I think we should consider:
> > * This applies flags in order of seeing them, e.g. "file,section" is different than "section,file". Perhaps we should error on any flags that overlap?
> > * GNU objcopy implements a few more: export, debug, constructor, warning, indirect, synthetic, indirect-function, unique-object, before=<other symbol>. We might want implement those as well, or at least accept them without error but ignore them.
> > 
> > Take a look at parseSectionRenameFlag above and see if some of that pattern helps -- it at least helps for ordering, i.e. do one pass to get parsed flag values, then another pass to apply parsed flags to bind/visibility/type.
> > Perhaps we should error on any flags that overlap?
> 
> I checked `parseSectionRenameFlag` and looks like it also doesn't check for overlaps (you can pass both code and data, load and noload, etc). FYI GNU objcopy also doesn't check for overlaps.
> 
> 
`parseSectionFlagSet` (the caller of `parseSectionRenameFlag`) doesn't check for overlap, but it doesn't look like any active features overlap -- e.g. "load" and "noload" don't do anything (they're just for GNU compatability), so it doesn't error out on details that don't matter.

I don't think GNU objcopy is being helpful if it permits overlaps -- if the user types "hidden,default" (maybe thinking "default" indicates "default binding type"), they might be surprised when the visibility is not hidden.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:251
+                       [&SI] { SI.Type = ELF::STT_GNU_IFUNC; })
+            .CaseLower("debug", [] {})
+            .CaseLower("constructor", [] {})
----------------
These noops can be combined with `CasesLower("debug", "constructor", ...)`


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

https://reviews.llvm.org/D58234