[llvm-objcopy] Add --redefine-syms

[rupprecht]

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

7 ↗

(On Diff #185256)

Shouldn't this be an error?

tools/llvm-objcopy/CopyConfig.cpp

258 ↗

(On Diff #185256)

It might be more straightforward (though verbose) to just use the other overload of split that puts thing in a SmallVector, and error if the size != 2.

259 ↗

(On Diff #185256)

This needs StringSaver to retain these lines once the memory buffer has gone out of scope.

I'm able to reproduce a crash w/ asan by removing StringSaver from addGlobalSymbolsFromFile above, and the usage here looks similar.

[evgeny777]

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

7 ↗

(On Diff #185256)

I don't know. GNU objdump treats this a error. However this line above

llvm-objcopy --redefine-sym foo=oof --redefine-sym empty= %t %t2

says that empty symbol names are allowed

[evgeny777]

tools/llvm-objcopy/CopyConfig.cpp

259 ↗

(On Diff #185256)

Yep, good catch

[rupprecht]

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

7 ↗

(On Diff #185256)

Hmm.... that's weird. I guess it's consistent.

I'll defer to @alexshap @jakehehrlich @jhenderson if they feel otherwise, but I think it might be fine to deviate from --redefine-sym and require two names -- I could imagine someone accidentally filling a text file with "foo=bar" instead of "foo bar". The downside is that if someone wants to set all their symbols to empty names, they'd have to do it painfully (add --redefine-sym=foo= for each one), but I think it's worth it for the much more common case of not getting the format right on the first try.

(The flag case is a less error prone, since foo= makes it obvious that you are assigning a symbol to something empty)

[jhenderson]

One question from me: is it possible using GNU objcopy to redefine to/from a name with a space in it this way? Perhaps using quotes or backslashes? Given that spaces in names are likely rare, I think it might be okay to not handle that case, but if GNU objcopy handles it, then we should too.

I think it's probably fine to deviate from GNU in the behaviour of handling lines without pairs in them, assuming that the above is handled in some sensible manner. I'd emit an error in that case.

You need a test for the case where multiple files are specified. Also possibly the case where the same symbol is mentioned multiple times in a file (although this is probably covered enough by the existing tests).

tools/llvm-objcopy/ObjcopyOpts.td

83 ↗

(On Diff #185256)

Nit: symbol -> symbols

[evgeny777]

@jhenderson

One question from me: is it possible using GNU objcopy to redefine to/from a name with a space in it this way?

Yes. I can redefine to a space (or even an empty string) like this

objcopy --redefine-sym=main=' ' a.out b.out

Still GNU objcopy seems to now allow the same to be done with --redefine-syms <file>
May be pop up warning in llvm-objcopy on such occasions?

[jhenderson]

In D57738#1387047, @evgeny777 wrote:

@jhenderson

One question from me: is it possible using GNU objcopy to redefine to/from a name with a space in it this way?

Yes. I can redefine to a space (or even an empty string) like this

objcopy --redefine-sym=main=' ' a.out b.out

Still GNU objcopy seems to now allow the same to be done with --redefine-syms <file>
May be pop up warning in llvm-objcopy on such occasions?

I was actually referring to --redefine-syms, i.e. how it looks in a file. Did you mean "not allow" rather than "now allow" in that other statement? I'm not quite sure what you're proposing re. the warning.

[evgeny777]

I was actually referring to --redefine-syms, i.e. how it looks in a file. Did you mean "not allow" rather than "now allow" in that other statement?

Yes, that's a mistype. I intended to say GNU objcopy does NOT allow single name on a line in a file when --redefine-syms is used. So there seems to be no way to set empty name in this mode.
We can either duplicate this behavior in llvm-objcopy or show warning like this

llvm-objcopy: warning: symbol-list:1: missing new symbol name

[jhenderson]

In D57738#1387113, @evgeny777 wrote:

I was actually referring to --redefine-syms, i.e. how it looks in a file. Did you mean "not allow" rather than "now allow" in that other statement?

Yes, that's a mistype. I intended to say GNU objcopy does NOT allow single name on a line in a file when --redefine-syms is used. So there seems to be no way to set empty name in this mode.
We can either duplicate this behavior in llvm-objcopy or show warning like this

llvm-objcopy: warning: symbol-list:1: missing new symbol name

I like the warning, especially if it has the line number in of the offending line(s).

[evgeny777]

Addressed

[jhenderson]

In D57738#1387137, @jhenderson wrote:

I like the warning, especially if it has the line number in of the offending line(s).

Actually, on second thoughts, I think this should be an error. If a user specifies a line in the file that is a bad format, that's as bad as doing the same on the command-line, so should be treated with the same severity.

[evgeny777]

Actually, on second thoughts, I think this should be an error. If a user specifies a line in the file that is a bad format, that's as bad as doing the same on the command-line, so should be treated with the same severity.

So do you actually want to trigger error in both (cl and file) cases?
If so should there be a way to replace some symbol foo with an empty symbol?

[jhenderson]

In D57738#1388520, @evgeny777 wrote:

Actually, on second thoughts, I think this should be an error. If a user specifies a line in the file that is a bad format, that's as bad as doing the same on the command-line, so should be treated with the same severity.

So do you actually want to trigger error in both (cl and file) cases?

No - the two cases are different, due to the different formats. In the cl case, a missing '=' causes a bad format error. If an '=' is present, the (potentially empty) string after it is the new name. In the file case, the separator is a space character, so the error should occur if there are no spaces (after trimming). That way, if a user writes "foo=bar" in the file, they'll get an error.

If so should there be a way to replace some symbol foo with an empty symbol?

Replacing symbol foo with an empty-name symbol probably is one of those corner cases that we should only support on the command-line.

[evgeny777]

Addressed

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

268 ↗

(On Diff #185744)

Nit: use size_t instead of unsigned to match the return type of .size(). Also, Lines.size() should be calculated out-of-line, I think, based on the coding standards (although it only talks about end() calls explicitly, so I could be wrong on this).

269 ↗

(On Diff #185744)

I don't think this should be auto. The type isn't obvious.

273 ↗

(On Diff #185744)

Ditto.

276–277 ↗

(On Diff #185744)

Rather than call error here, this should probably return an llvm::Error which is handled in the calling code.

tools/llvm-objcopy/ObjcopyOpts.td

83 ↗

(On Diff #185256)

This still needs doing.

[evgeny777]

Addressed

[jhenderson]

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

93 ↗

(On Diff #185756)

This will fail on Windows because "No such file or directory." is Linux specific. You can make it platform independent by replacing it with {{[Nn]}}o such file or directy (note the lack of full-stop and the capitalization).

101 ↗

(On Diff #185756)

Nit: file is missing new line at end.

tools/llvm-objcopy/CopyConfig.cpp

278–281 ↗

(On Diff #185756)

Use createStringError here. That also allows you to use printf-style formatting if you want.

Also, this error isn't an "illegal_byte_sequence" which is to do with character encoding. It should be something like "invalid_argument" or "parse_failed" or similar.

tools/llvm-objcopy/llvm-objcopy.cpp

55–58 ↗

(On Diff #185756)

There's already a createFileError in LLVM. Can you use that?

[evgeny777]

Addressed

[jhenderson]

LGTM, with the noted fix.

tools/llvm-objcopy/CopyConfig.cpp

278–281 ↗

(On Diff #185756)

%lu is the wrong format for a size_t. It should be %zu.

tools/llvm-objcopy/llvm-objcopy.cpp

55–58 ↗

(On Diff #185756)

It doesn't need to be part of this change, but I wonder if it would make sense for a new createFileError overload to be added alongside the existing one that takes a std::error_code?

llvm/trunk/test/tools/llvm-objcopy/ELF/redefine-symbol.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --redefine-sym foo=oof --redefine-sym empty= %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
 # RUN: not llvm-objcopy --redefine-sym barbar %t %t2 2>&1 | FileCheck %s --check-prefix=BAD-FORMAT
 # RUN: not llvm-objcopy --redefine-sym foo=f1 --redefine-sym foo=f2 %t %t2 2>&1 | FileCheck %s --check-prefix=MULTIPLE-REDEFINITION
+# RUN: echo "  foo   oof #rename foo  " > %t.rename.txt
+# RUN: echo "empty" >> %t.rename.txt
+# RUN: not llvm-objcopy --redefine-syms %t.rename.txt %t %t3 2>&1 | FileCheck %s --check-prefix=MISSING-SYM-NAME
+# RUN: not llvm-objcopy --redefine-syms %t.rename-none.txt %t %t-none 2>&1 | FileCheck %s --check-prefix=NO-FILE
+# RUN: cmp %t2 %t3
+# RUN: echo "  bar   rab #rename bar  " > %t.rename2.txt
+# RUN: echo "  foo   oof #rename foo  " > %t.rename3.txt
+# RUN: echo "  empty   ytpme #rename empty  " >> %t.rename3.txt
+# RUN: llvm-objcopy --redefine-syms %t.rename2.txt --redefine-syms %t.rename3.txt %t %t4
+# RUN: llvm-readobj --symbols %t4 | FileCheck %s --check-prefix=MULTIPLE-FILES
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:

 #CHECK-NEXT:    Binding: Global
 #CHECK-NEXT:    Type: Function
 #CHECK-NEXT:    Other: 0
 #CHECK-NEXT:    Section: .text
 #CHECK-NEXT:  }
 
 #BAD-FORMAT: Bad format for --redefine-sym
 #MULTIPLE-REDEFINITION: Multiple redefinition of symbol foo
+#MISSING-SYM-NAME: error: {{.*}}.rename.txt:2: missing new symbol name
+#NO-FILE: error: '{{.*}}.rename-none.txt': {{[Nn]}}o such file or directory
+
+#MULTIPLE-FILES:        Name: oof
+#MULTIPLE-FILES-NEXT:   Value: 0x1004
+#MULTIPLE-FILES:        Name: rab
+#MULTIPLE-FILES-NEXT:   Value: 0x2000
+#MULTIPLE-FILES:        Name: ytpme
+#MULTIPLE-FILES-NEXT:   Value: 0x1008

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
 namespace {
if (!IsRegex) {
     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 createError(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();
+}
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       error("Bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
     if (!Config.SymbolsToRename.insert(Old2New).second)
       error("Multiple redefinition of symbol " + Old2New.first);
   }
 
+  for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbols))
+    if (Error E = addSymbolsToRenameFromFile(Config.SymbolsToRename, DC.Alloc,
+                                             Arg->getValue()))
+      error(std::move(E));
+
   for (auto Arg : InputArgs.filtered(OBJCOPY_rename_section)) {
     SectionRename SR = parseRenameSectionValue(StringRef(Arg->getValue()));
     if (!Config.SectionsToRename.try_emplace(SR.OriginalName, SR).second)
       error("Multiple renames of section " + SR.OriginalName);
   }
   for (auto Arg : InputArgs.filtered(OBJCOPY_set_section_flags)) {
     SectionFlagsUpdate SFU = parseSetSectionFlagValue(Arg->getValue());
     if (!Config.SetSectionFlags.try_emplace(SFU.Name, SFU).second)

llvm/trunk/tools/llvm-objcopy/ObjcopyOpts.td

defm rename_section
     : Eq<"rename-section",
          "Renames a section from old to new, optionally with specified flags. "
          "Flags supported for GNU compatibility: alloc, load, noload, "
          "readonly, debug, code, data, rom, share, contents, merge, strings.">,
       MetaVarName<"old=new[,flag1,...]">;
 defm redefine_symbol
     : Eq<"redefine-sym", "Change the name of a symbol old to new">,
       MetaVarName<"old=new">;
+defm redefine_symbols
+    : Eq<"redefine-syms",
+         "Reads a list of symbol pairs from <filename> and runs as if "
+         "--redefine-sym=<old>=<new> is set for each one. <filename> "
+         "contains two symbols per line separated with whitespace and may "
+         "contain comments beginning with '#'. Leading and trailing "
+         "whitespace is stripped from each line. May be repeated to read "
+         "symbols from many files.">,         
+      MetaVarName<"filename">;
+
 defm keep_section : Eq<"keep-section", "Keep <section>">,
                     MetaVarName<"section">;
 defm only_section : Eq<"only-section", "Remove all but <section>">,
                     MetaVarName<"section">;
 def j : JoinedOrSeparate<["-"], "j">, Alias<only_section>;
 defm add_section
     : Eq<"add-section",
          "Make a section named <section> with the contents of <file>.">,

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.h

 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
 
 namespace llvm {
 namespace objcopy {
 
+Error createError(StringRef File, std::error_code EC);
+
 LLVM_ATTRIBUTE_NORETURN extern void error(Twine Message);
 LLVM_ATTRIBUTE_NORETURN extern void error(Error E);
 LLVM_ATTRIBUTE_NORETURN extern void reportError(StringRef File, Error E);
 LLVM_ATTRIBUTE_NORETURN extern void reportError(StringRef File,
                                                 std::error_code EC);
 
 // This is taken from llvm-readobj.
 // [see here](llvm/tools/llvm-readobj/llvm-readobj.h:38)

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 
 // The name this program was invoked as.
 StringRef ToolName;
 
+Error createError(StringRef File, std::error_code EC) {
+  assert(EC);  
+  return createFileError(File, make_error<StringError>(EC));
+}
+
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
   WithColor::error(errs(), ToolName) << Message << ".\n";
   errs().flush();
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void error(Error E) {
   assert(E);
   std::string Buf;
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();
   WithColor::error(errs(), ToolName) << Buf;
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, std::error_code EC) {
-  assert(EC);
-  WithColor::error(errs(), ToolName)
-      << "'" << File << "': " << EC.message() << ".\n";
-  exit(1);
+  error(createError(File, EC));
 }
 
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
   assert(E);
   std::string Buf;
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();