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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 marked 3 inline comments as done.
evgeny777 added inline comments.


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




================
Comment at: tools/llvm-objcopy/CopyConfig.h:66
+  StringRef SectionName;
+  unsigned long long Value;
+  uint8_t Type;
----------------
rupprecht wrote:
> uint64_t
Like I said in D58173 this will not work, because `uint64_t` is not `unsigned long long` on 64 bit platforms


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:573
+    Obj.SymbolTable->addSymbol(SI.SymbolName, SI.Bind, SI.Type, Sec, Value,
+                               SI.Visibility, SHN_ABS, 0);
+  }
----------------
rupprecht wrote:
> GNU docs says: "If the section is given, the symbol will be associated with and relative to that section, otherwise it will be an ABS symbol."
> 
> I think SHN_ABS is wrong if the section name is given.
The shndx parameter is ignored when `DefinedIn` is not nullptr. See `addSymbol`


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

https://reviews.llvm.org/D58234