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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:198
+  NewSymbolInfo SI = {
+      "", "", 0, ELF::STT_NOTYPE, ELF::STB_GLOBAL, ELF::STV_DEFAULT};
+  StringRef Value;
----------------
I think these would be better as struct defaults, then just declare as "NewSymbolInfo SI;" here


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:202
+  if (Value.empty())
+    error("bad format for --add-symbol");
+
----------------
Can you elaborate on the bad format, e.g. for this case: missing: '='


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:207
+    if (SI.SectionName.empty() || Value.empty())
+      error("bad format for --add-symbol");
+  }
----------------
Here too


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


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:235
+    else
+      error("unsupported flag for --add-symbol");
+
----------------
Include the unsupported flag name, also, this should print the supported flag names.


================
Comment at: tools/llvm-objcopy/CopyConfig.h:66
+  StringRef SectionName;
+  unsigned long long Value;
+  uint8_t Type;
----------------
uint64_t


================
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);
+  }
----------------
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.


================
Comment at: tools/llvm-objcopy/ELF/Object.h:806
+        find_if(Sections, [&](const SecPtr &Sec) { return Sec->Name == Name; });
+    return SecIt == Sections.end() ? nullptr : (*SecIt).get();
+  }
----------------
SecIt->get()


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:243
+     : Eq<"add-symbol", "Add new symbol <name> to .symtab.">,
+       MetaVarName<"name=[section:]value[,flags]">;
----------------
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


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

https://reviews.llvm.org/D58234