[llvm-objcopy] Add --add-symbol

[rupprecht]

tools/llvm-objcopy/CopyConfig.cpp

198

I think these would be better as struct defaults, then just declare as "NewSymbolInfo SI;" here

202

Can you elaborate on the bad format, e.g. for this case: missing: '='

207

Here too

215

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.

235

Include the unsupported flag name, also, this should print the supported flag names.

tools/llvm-objcopy/CopyConfig.h

66

uint64_t

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

573

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.

tools/llvm-objcopy/ELF/Object.h

806

SecIt->get()

tools/llvm-objcopy/ObjcopyOpts.td

243

All of these could use some more docs:

• Mention default behavior if section is not provided
• Describe the format of value (number, hex, etc.)
• Include all supported flags

[evgeny777]

tools/llvm-objcopy/CopyConfig.cpp

215

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.

tools/llvm-objcopy/CopyConfig.h

66

Like I said in D58173 this will not work, because uint64_t is not unsigned long long on 64 bit platforms

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

573

The shndx parameter is ignored when DefinedIn is not nullptr. See addSymbol

[evgeny777]

Addressed some of review comments

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

218–222

I'd fold this bit of the comment in with the <flags> bit above, since they are closely linked. Also, I'd remove the word "silently" from it.

243

Could you use a StringSwitch here, using CaseLower to get case insensitivity, perhaps with lambdas, to avoid the if/else list?

tools/llvm-objcopy/CopyConfig.h

66

I don't understand what you mean by this? Symbol values are by definition in the ELF spec up to 64-bit in size, so uint64_t (which is 64-bits in size) is the correct type. unsigned long long is only guaranteed to be at least as big as unsigned long which in turn is only guaranteed to be as big as unisgned int etc.

Related note: The bitness of the platform does not define the size of types by itself. For example, on Windows, unsigned long is always 32 bits, both for 32-bit Windows and 64-bit Windows. On the other hand, unsigned long long is 64 bits on both.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

573

This should probably be commented. It also feels rather fragile though.

[jhenderson]

tools/llvm-objcopy/CopyConfig.h

66

Okay, I now see what the problem is. It's to do with the call to getAsUnsignedInteger, later on right?

I think this issue could be solved by defining an unsigned long version of getAsUnsignedInteger. That way, it should work regardless of the underlying type of uint64_t. Implementing that would be my preference to working around this by using primitive types in llvm-objcopy rather than fixed-width types.

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

239

The return value here is unchecked. What happens if Value is not a valid number?

[evgeny777]

Addressed

[rupprecht]

test/tools/llvm-objcopy/ELF/add-symbol.test

54

nit: add a newline

tools/llvm-objcopy/CopyConfig.cpp

215

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.

252

These noops can be combined with CasesLower("debug", "constructor", ...)

[evgeny777]

Addressed

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

238

See my comment below about returning Error rather than calling error.

296

I think we're generally aiming to move away from error/reportError in favour of passing Error/Expected back up the stack to be handled at the top level. Please do that instead here and elsewhere where you've added new error messages. You might need to rebase on top of D58316.

321

Could this be folded in with the other ones below?

[evgeny777]

tools/llvm-objcopy/CopyConfig.cpp

321

CasesLower supports up to 5 string parameters. Are you suggesting to add another variant which takes 6 ones?

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

321

Ah, I didn't realise that. This is fine then.

[jakehehrlich]

Sorry to be the Grinch again but what is the use case for this? This isn't a feature in GNU objcopy as far as I can tell and the semantics of such a change are quite involved. I think I'd prefer to discuss this in llvm-dev first. I belive this can be given meaningful and consistent semantics and I think it could have a use case but right now I don't see one. This seems like a complicated feature to add without a use case in mind.

[rupprecht]

In D58234#1404801, @jakehehrlich wrote:

Sorry to be the Grinch again but what is the use case for this? This isn't a feature in GNU objcopy as far as I can tell and the semantics of such a change are quite involved.

Not OP (and I don't have a use case), but from man objcopy:

--add-symbol name=[section:]value[,flags]
    Add a new symbol named name while copying the file.  ...

[evgeny777]

Sorry to be the Grinch again but what is the use case for this?

Haha, no that's in fact a good question =)
I'm using it to hack on a stripped binary. You can add symbols here and there and then set breakpoints with gdb.
Makes things a little bit easier

[evgeny777]

Addressed

[rupprecht]

tools/llvm-objcopy/CopyConfig.h

66

uint64_t

tools/llvm-objcopy/ObjcopyOpts.td

242

We should list all all the flags we support here, something like:

"Add new symbol <name> to .symtab. Accepted flags: global, local, weak, default, hidden, file, section, object, function, indirect-function. Accepted but ignored for compatibility: debug, constructor, warning, indirect, synthetic, unique-object, before."

I think string you have is actually fine for --help, but we don't have a man page to put the more verbose info in, so we should include it in --help so we have it somewhere. We should probably create a man page for llvm-objcopy in the llvm-9 release and make these docs simpler.

test/tools/llvm-objcopy/ELF/add-symbol.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --add-symbol='abs1=1' \ 
+# RUN:              --add-symbol='abs2=2,LoCaL,HiDdEn,FuNcTiOn' \
+# RUN:              --add-symbol='abs3=3,global,default,object' \
+# RUN:              --add-symbol='foo.cpp=0,file' \
+# RUN:              --add-symbol='=.text:0,section' \
+# RUN:              --add-symbol='data=.data:0x100,weak' \
+# RUN:              %t %t2
+# RUN: llvm-readelf -s %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_ARM
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000001000
+    Size:            64
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x2000
+    AddressAlign:    0x0000000000001000
+    Size:            64
+ProgramHeaders:
+  - Type: PT_LOAD
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x1000
+    PAddr: 0x1000
+    Align: 0x1000
+    Sections:
+      - Section: .text
+  - Type: PT_LOAD
+    Flags: [ PF_R, PF_W ]
+    VAddr: 0x2000
+    PAddr: 0x2000
+    Align: 0x1000
+    Sections:
+      - Section: .data
+
+# CHECK:      0: 00000000 0 NOTYPE LOCAL  DEFAULT  UND 
+# CHECK-NEXT: 1: 00000001 0 NOTYPE GLOBAL DEFAULT  ABS abs1
+# CHECK-NEXT: 2: 00000002 0 FUNC   LOCAL  HIDDEN   ABS abs2
+# CHECK-NEXT: 3: 00000003 0 OBJECT GLOBAL DEFAULT  ABS abs3
+# CHECK-NEXT: 4: 00000000 0 FILE   GLOBAL DEFAULT  ABS foo.cpp
+# CHECK-NEXT: 5: 00001000 0 SECTION GLOBAL DEFAULT  1 
+# CHECK-NEXT: 6: 00002100 0 NOTYPE WEAK DEFAULT 2 data
+

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

if (!Config.BuildIdLinkDir.empty() || Config.BuildIdLinkInput ||
       !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
       !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
       !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
       !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
       !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty() ||
       Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
       Config.PreserveDates || Config.StripDWO || Config.StripNonAlloc ||
       Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
-      Config.DiscardMode == DiscardType::Locals) {
+      Config.DiscardMode == DiscardType::Locals ||
+      !Config.SymbolsToAdd.empty()) {
     return createStringError(llvm::errc::invalid_argument,
                              "Option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config, COFFObjectFile &In,

tools/llvm-objcopy/CopyConfig.h

class NameOrRegex {
   std::shared_ptr<Regex> R;
 
 public:
   NameOrRegex(StringRef Pattern, bool IsRegex);
   bool operator==(StringRef S) const { return R ? R->match(S) : Name == S; }
   bool operator!=(StringRef S) const { return !operator==(S); }
 };
 
+struct NewSymbolInfo {
+  StringRef SymbolName;
+  StringRef SectionName;
+  unsigned long long Value;
+  uint8_t Type;
+  uint8_t Bind;
+  uint8_t Visibility;
+};
+
 // Configuration for copying/stripping a single file.
 struct CopyConfig {
   // Main input/output options
   StringRef InputFilename;
   StringRef InputFormat;
   StringRef OutputFilename;
   StringRef OutputFormat;
 
struct CopyConfig {
   Optional<StringRef> BuildIdLinkOutput;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
   DiscardType DiscardMode = DiscardType::None;
 
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
+  std::vector<NewSymbolInfo> SymbolsToAdd;
   std::vector<NameOrRegex> KeepSection;
   std::vector<NameOrRegex> OnlySection;
   std::vector<NameOrRegex> SymbolsToGlobalize;
   std::vector<NameOrRegex> SymbolsToKeep;
   std::vector<NameOrRegex> SymbolsToLocalize;
   std::vector<NameOrRegex> SymbolsToRemove;
   std::vector<NameOrRegex> UnneededSymbolsToRemove;
   std::vector<NameOrRegex> SymbolsToWeaken;

tools/llvm-objcopy/CopyConfig.cpp

static SectionFlagsUpdate parseSetSectionFlagValue(StringRef FlagValue) {
   // Flags split: "f1" "f2" ...
   SmallVector<StringRef, 6> SectionFlags;
   Section2Flags.second.split(SectionFlags, ',');
   SFU.NewFlags = parseSectionFlagSet(SectionFlags);
 
   return SFU;
 }
 
+static NewSymbolInfo parseNewSymbolInfo(StringRef FlagValue) {
+  // Format is: <name>=[<section>:]<value>[,<flags>]
+  NewSymbolInfo SI = {
+      "", "", 0, ELF::STT_NOTYPE, ELF::STB_GLOBAL, ELF::STV_DEFAULT};
+  StringRef Value;
+  std::tie(SI.SymbolName, Value) = FlagValue.split('=');
+  if (Value.empty())
+    error("bad format for --add-symbol");
+
+  if (Value.contains(':')) {
+    std::tie(SI.SectionName, Value) = Value.split(':');
+    if (SI.SectionName.empty() || Value.empty())
+      error("bad format for --add-symbol");
+  }
+
+  SmallVector<StringRef, 6> Flags;
+  Value.split(Flags, ',');
+  getAsUnsignedInteger(Flags[0], 0, SI.Value);
+
+  size_t NumFlags = Flags.size();
+  for (size_t I = 1; I < NumFlags; ++I)
+    if (Flags[I].equals_lower("global"))
+      SI.Bind = ELF::STB_GLOBAL;
+    else if (Flags[I].equals_lower("local"))
+      SI.Bind = ELF::STB_LOCAL;
+    else if (Flags[I].equals_lower("weak"))
+      SI.Bind = ELF::STB_WEAK;
+    else if (Flags[I].equals_lower("default"))
+      SI.Visibility = ELF::STV_DEFAULT;
+    else if (Flags[I].equals_lower("hidden"))
+      SI.Visibility = ELF::STV_HIDDEN;
+    else if (Flags[I].equals_lower("file"))
+      SI.Type = ELF::STT_FILE;
+    else if (Flags[I].equals_lower("section"))
+      SI.Type = ELF::STT_SECTION;
+    else if (Flags[I].equals_lower("object"))
+      SI.Type = ELF::STT_OBJECT;
+    else if (Flags[I].equals_lower("function"))
+      SI.Type = ELF::STT_FUNC;
+    else
+      error("unsupported flag for --add-symbol");
+
+  return SI;
+}
+
 static const StringMap<MachineInfo> ArchMap{
     // Name, {EMachine, 64bit, LittleEndian}
     {"aarch64", {ELF::EM_AARCH64, true, true}},
     {"arm", {ELF::EM_ARM, false, true}},
     {"i386", {ELF::EM_386, false, true}},
     {"i386:x86-64", {ELF::EM_X86_64, true, true}},
     {"powerpc:common64", {ELF::EM_PPC64, true, true}},
     {"sparc", {ELF::EM_SPARC, false, true}},
     {"x86-64", {ELF::EM_X86_64, true, true}},
 };
 
 static const MachineInfo &getMachineInfo(StringRef Arch) {
   auto Iter = ArchMap.find(Arch);
   if (Iter == std::end(ArchMap))
     error("Invalid architecture: '" + Arch + "'");
   return Iter->getValue();
 }
 
 static const StringMap<MachineInfo> OutputFormatMap{
     // Name, {EMachine, 64bit, LittleEndian}
     {"elf32-i386", {ELF::EM_386, false, true}},
for (StringRef Line : Lines) {
     if (!TrimmedLine.empty())
       Symbols.emplace_back(Saver.save(TrimmedLine), UseRegex);
   }
 }
 
 NameOrRegex::NameOrRegex(StringRef Pattern, bool IsRegex) {
   if (!IsRegex) {
     Name = Pattern;
     return;
   }
 
   SmallVector<char, 32> Data;
   R = std::make_shared<Regex>(
       ("^" + Pattern.ltrim('^').rtrim('$') + "$").toStringRef(Data));
 }
 
 static Error addSymbolsToRenameFromFile(StringMap<StringRef> &SymbolsToRename,
                                         BumpPtrAllocator &Alloc,
                                         StringRef Filename) {
   StringSaver Saver(Alloc);
   SmallVector<StringRef, 16> Lines;
   auto BufOrErr = MemoryBuffer::getFile(Filename);
   if (!BufOrErr)
     return createFileError(Filename, BufOrErr.getError());
 
   BufOrErr.get()->getBuffer().split(Lines, '\n');
   size_t NumLines = Lines.size();
   for (size_t LineNo = 0; LineNo < NumLines; ++LineNo) {
     StringRef TrimmedLine = Lines[LineNo].split('#').first.trim();
     if (TrimmedLine.empty())
       continue;
 
     std::pair<StringRef, StringRef> Pair = Saver.save(TrimmedLine).split(' ');
     StringRef NewName = Pair.second.trim();
     if (NewName.empty())
       return createStringError(errc::invalid_argument,
                                "%s:%zu: missing new symbol name",
                                Filename.str().c_str(), LineNo + 1);
     SymbolsToRename.insert({Pair.first, NewName});
   }
   return Error::success();
 }
addSymbolsFromFile(Config.SymbolsToRemove, DC.Alloc, Arg->getValue(),
                        UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_unneeded_symbol))
     Config.UnneededSymbolsToRemove.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_unneeded_symbols))
     addSymbolsFromFile(Config.UnneededSymbolsToRemove, DC.Alloc,
                        Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.emplace_back(Arg->getValue(), UseRegex);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_add_symbol))
+    Config.SymbolsToAdd.push_back(parseNewSymbolInfo(Arg->getValue()));
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
   if (Config.DecompressDebugSections &&

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

for (const auto &Flag : Config.DumpSection) {
       if (Error E = dumpSectionToFile(SecName, File, Obj))
         return createFileError(Config.InputFilename, std::move(E));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
 
+  for (const NewSymbolInfo &SI : Config.SymbolsToAdd) {
+    SectionBase *Sec = Obj.findSection(SI.SectionName);
+    uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
+    Obj.SymbolTable->addSymbol(SI.SymbolName, SI.Bind, SI.Type, Sec, Value,
+                               SI.Visibility, SHN_ABS, 0);
+  }
+
   return Error::success();
 }
 
 Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 Buffer &Out) {
   BinaryReader Reader(Config.BinaryArch, &In);
   std::unique_ptr<Object> Obj = Reader.create();
 

tools/llvm-objcopy/ELF/Object.h

public:
   SymbolTableSection *SymbolTable = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
 
   void sortSections();
   SectionTableRef sections() { return SectionTableRef(Sections); }
   ConstRange<SectionBase> sections() const {
     return make_pointee_range(Sections);
   }
+  SectionBase *findSection(StringRef Name) {
+    auto SecIt =
+        find_if(Sections, [&](const SecPtr &Sec) { return Sec->Name == Name; });
+    return SecIt == Sections.end() ? nullptr : (*SecIt).get();
+  }
   Range<Segment> segments() { return make_pointee_range(Segments); }
   ConstRange<Segment> segments() const { return make_pointee_range(Segments); }
 
   Error removeSections(std::function<bool(const SectionBase &)> ToRemove);
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   template <class T, class... Ts> T &addSection(Ts &&... Args) {
     auto Sec = llvm::make_unique<T>(std::forward<Ts>(Args)...);
     auto Ptr = Sec.get();

tools/llvm-objcopy/ObjcopyOpts.td

 defm build_id_link_output
     : Eq<"build-id-link-output", "Hard-link the output to <dir>/xx/xxx<suffix> "
                                  "name derived from hex build ID">,
       MetaVarName<"suffix">;
 
 def regex
     : Flag<["-", "--"], "regex">,
       HelpText<"Permit regular expressions in name comparison">;
+
+defm add_symbol
+     : Eq<"add-symbol", "Add new symbol <name> to .symtab.">,
+       MetaVarName<"name=[section:]value[,flags]">;