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

Jordan Rupprecht via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 14 11:11:53 PST 2019

rupprecht added inline comments.

Comment at: tools/llvm-objcopy/CopyConfig.cpp:198
+  NewSymbolInfo SI = {
+  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;

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

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



More information about the llvm-commits mailing list