[PATCH] D58234: [llvm-objcopy] Add --add-symbol
Eugene Leviant via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 15 00:35:53 PST 2019
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
More information about the llvm-commits
mailing list