[llvm-objcopy] Add --set-start, --change-start, --adjust-start

[mstorsjo]

Please add a check for this new option in COFF/COFFObjCopy.cpp for the "Option not supported by llvm-objcopy for COFF" error, to avoid giving the illusion that the option is handled with that backend.

(The MachO backend doesn't yet look at options at all and doesn't have a similar error case for unimplemented options.)

[evgeny777]

Addressed

[rupprecht]

tools/llvm-objcopy/CopyConfig.cpp

486 ↗

(On Diff #186671)

uint64_t

492 ↗

(On Diff #186671)

int64_t

tools/llvm-objcopy/CopyConfig.h

105 ↗

(On Diff #186671)

Can you use a more descriptive name/docs here?

tools/llvm-objcopy/ObjcopyOpts.td

245 ↗

(On Diff #186671)

Does this allow -adjust-start?

[evgeny777]

tools/llvm-objcopy/CopyConfig.cpp

486 ↗

(On Diff #186671)

On 64 bit platforms uint64_t is unsigned long, not unsigned long long. You'll get the compilation error on x64 if you change type (x86 will be ok)

[evgeny777]

Addressed some of review comments

[• ham999221]

This comment was removed by asl.

[jhenderson]

test/tools/llvm-objcopy/ELF/set-start.test

1 ↗

(On Diff #186981)

Maybe worth renaming the test to something like "change-entry-point.test" since set-start implies you are just changing testing the --set-start switch. Alternatively, split the switches into different tests.

2 ↗

(On Diff #186981)

Could you also add test cases that show --set-start takes decimal, please?

What happens if you try to specify a negative number here?

6 ↗

(On Diff #186981)

Haven't checked GNU objcopy to see if this makes sense, but can you specify -0xabcd? If so, please add a test case for that.

Please also add these test-cases:

1. --adjust-start causes an underflow. I think this should be an error.
2. The above switches with numbers that are in the 64-bit range (i.e. too big to fit in a 32-bit number).

24 ↗

(On Diff #186981)

Nit: Put a space between # and the check-prefix, for readability.

tools/llvm-objcopy/CopyConfig.cpp

487 ↗

(On Diff #186981)

You aren't checking the return value for success here.

493 ↗

(On Diff #186981)

Unchecked return here.

486 ↗

(On Diff #186671)

This isn't actually quite true, since it's the standard library which defines the underlying type of uint64_t (i.e. it could be unsigned long, unsigned long long, or some internal compiler type). See my comments in D58234.

[jakehehrlich]

1. What's the use case for this?
2. What's the interaction for program headers?

For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.

[jhenderson]

In D58173#1401364, @jakehehrlich wrote:

1. What's the use case for this?
2. What's the interaction for program headers?

For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.

Re. 2) This will have no interaction with program headers (though we might want to check that the address is within a loadable program header). All it does is change the e_entry field of the ELF header, i.e. the start address of the program. Personally, I don't have an issue with it changing that value in an ET_REL file (normally it would be 0), but we should follow what GNU objcopy does in that case. It doesn't hurt to have the value set in such a file; it's just meaningless.

I don't personally have a use case for this, but I could imagine a program with multiple possible start locations, and these switches allow switching between them. I'd like to here the concrete use case though.

[evgeny777]

I don't personally have a use case for this, but I could imagine a program with multiple possible start locations, and these switches allow switching between them. I'd like to here the concrete use case though.

This is used together with --update-section, because the latter can modify section and segment addresses. However --update-section flag is not yet supported. I had plans to support it
as well but it involves both section and segment modification. If segment modification is not desired in llvm-objcopy then this patch probably doesn't make much sense.

[evgeny777]

Addressed

[evgeny777]

--adjust-start causes an underflow. I think this should be an error.

Why? GNU objcopy allows both underflow and overflow of start address

[jhenderson]

test/tools/llvm-objcopy/ELF/change-entry-point.test

1 ↗

(On Diff #187331)

I'm having trouble reading the below set of tests. Could you split them up with blank lines between the test cases, and also group together the switches so that all --set-start tests are first, then --change-start etc.

17 ↗

(On Diff #187331)

I don't think you've responded to this comment?

Haven't checked GNU objcopy to see if this makes sense, but can you specify -0xabcd? If so, please add a test case for that.

tools/llvm-objcopy/CopyConfig.cpp

287 ↗

(On Diff #187331)

This should return an Expected<T> and not report the error in this function.

[evgeny777]

• added test case for negative hexadecimal number
• changed way error handling is done

[jakehehrlich]

This change looks good to me and the semantics seem sound. I'd still appreciate a use case however. This is small and compact enough and I can imagine you might want to do this in cases where a process doesn't fix itself up at all and consequently there are multiple valid entry points but by and large this seems quite odd to me.

Please wait for for someone else to accept this as well.

[jhenderson]

test/tools/llvm-objcopy/ELF/change-entry-point.test

24 ↗

(On Diff #187535)

When testing aliases, we've tended to run the same run twice, and then compared the output to show it is identical. I think that you should do the same here, i.e. run --change-start -128 then compare that to --adjust-start -128.

tools/llvm-objcopy/CopyConfig.h

105 ↗

(On Diff #187535)

Couple of nits here:

1. "in input ELF file" -> "in the input ELF file"
2. "Entry address in output file" -> "The entry address in the output file"

[rupprecht]

tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #187535)

Is --set-start X --change-start Y valid? Should set start to X+Y, or EAddr+Y, or be an error?
It looks like this sets it to EAddr+Y, but I think it should probably be an error or X+Y.

[evgeny777]

Isn't it an overkill to cover all possible misuses? This is a developer tool so far ...

[rupprecht]

In D58173#1406185, @evgeny777 wrote:

Isn't it an overkill to cover all possible misuses? This is a developer tool so far ...

Isn't clang just a developer tool? Why bother going through the trouble of implementing compiler warnings -- shouldn't good C++ devs know the spec by heart?

IMO, we should be setting the bar high for the quality of these tools. If this were just a random script that a handful of developers will share, I'd agree, but this is a tool that we should be encouraging broad adoption for. Also, in this case, I think it's just a couple lines to check, right? No fancy algorithms needed to check if two flags are being used.

if (InputArgs.hasArg(OBJCOPY_set_start) && InputArgs.hasArg(OBJCOPY_change_start))
  error();

[jhenderson]

I agree with @rupprecht on this. It gets even more involved when multiple build scripts start interacting with each other, as it may not even be clear to the developer that they are using both switches. I would be happy either with --change-start changing the value set by --set-start, or a warning or error.

[evgeny777]

Addressed

[evgeny777]

Didn't realize that D58316 has been pushed. Rebased.

[jhenderson]

test/tools/llvm-objcopy/ELF/change-entry-point.test

23 ↗

(On Diff #187941)

Nit: trailing full stop, here and on the other comments in this test.

tools/llvm-objcopy/CopyConfig.cpp

572 ↗

(On Diff #187941)

Could you just make this the default? I.e. set Config.EntryExpr to return the input unless something overrides it?

tools/llvm-objcopy/CopyConfig.h

106 ↗

(On Diff #187941)

You still are missing the first "the" in the third sentence, as requested. Also, it should be "The input parameter is..." not just "Input parameter is..."

[evgeny777]

tools/llvm-objcopy/CopyConfig.cpp

572 ↗

(On Diff #187941)

How to deal with COFF variant? Setting this option should be an error in such case.

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

572 ↗

(On Diff #187941)

Ah, I didn't think about COFF here, fair enough.

[evgeny777]

Addressed and rebased

[jhenderson]

LGTM, with one remaining nit.

test/tools/llvm-objcopy/ELF/change-entry-point.test

31 ↗

(On Diff #188159)

You don't need the "The" here.

[jhenderson]

It would probably be wise to wait for @rupprecht to confirm he's okay before committing.

[rupprecht]

Two differences we're making from GNU objcopy:

1. --set-start and --change-start are now order dependent, e.g.

--set-start 0x100 --change-start 0x4 => both GNU objcopy and llvm-objcopy agree the start is 0x104
--change-start 0x4 --set-start 0x100 => GNU objcopy starts at 0x104, llvm-objcopy starts at 0x100 (i.e. --set-start overrides previous values)

2. --change-start is cumulative, e.g.

--set-start 0x100 --change-start 0x4 --change-start 0x2 => GNU objcopy starts at 0x102 (last value of --change-start wins), llvm-objcopy starts at 0x106 (both values are added together)

I'm ok with both of these changes (if anyone is relying on this... that's scary), but it'd be nice to call this out in the help string (or man pages if we had those).

tools/llvm-objcopy/ObjcopyOpts.td

241 ↗

(On Diff #188159)

Add to the help string: Overrides any previous --change-start or --adjust-start values

243 ↗

(On Diff #188159)

Add to the help string: Can be specified multiple times, all values will be applied cumulatively.

(I'm not sure if that's the best wording)

llvm/trunk/test/tools/llvm-objcopy/ELF/change-entry-point.test

+# RUN: yaml2obj %s -o %t
+
+# Test --set-start.
+# RUN: llvm-objcopy --set-start 0x1000 %t %t2
+# RUN: llvm-readobj --file-headers %t2 | FileCheck %s --check-prefix=SET
+# RUN: llvm-objcopy --set-start 4096 %t %t3
+# RUN: cmp %t2 %t3
+# RUN: llvm-objcopy --set-start 0x100000000 %t %t4
+# RUN: llvm-readobj --file-headers %t4 | FileCheck %s --check-prefix=SET-LARGE
+# RUN: not llvm-objcopy --set-start -123 %t %t5 2>&1 | FileCheck %s --check-prefix=SET-ERR
+
+# Test --change-start.
+# RUN: llvm-objcopy --change-start 0x50 %t %t6
+# RUN: llvm-readobj --file-headers %t6 | FileCheck %s --check-prefix=ADD
+# RUN: llvm-objcopy --change-start 0x100000000 %t %t7
+# RUN: llvm-readobj --file-headers %t7 | FileCheck %s --check-prefix=ADD-LARGE
+# RUN: llvm-objcopy --change-start -4353 %t %t8
+# RUN: llvm-readobj --file-headers %t8 | FileCheck %s --check-prefix=ADD-UNDERFLOW
+# RUN: llvm-objcopy --change-start -0x1101 %t %t9
+# RUN: cmp %t8 %t9
+# RUN: not llvm-objcopy --change-start -xyz %t %t10 2>&1 | FileCheck %s --check-prefix=ADD-ERR
+
+# Test --change-start after --set-start. Result should be 0x1150.
+# RUN: llvm-objcopy --set-start 0x1000 --change-start 0x100 --change-start 0x50 %t %t11
+# RUN: cmp %t6 %t11
+
+# If --set-start is after --change-start then --change-start has no effect.
+# RUN: llvm-objcopy --change-start 0x150 --set-start 0x1000 %t %t12
+# RUN: cmp %t2 %t12
+
+# --adjust-start is an alias for --change-start.
+# RUN: llvm-objcopy --adjust-start -0x1101 %t %t13
+# RUN: cmp %t9 %t13
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x1100
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000001000
+    Size:            0x200
+
+# SET: Entry: 0x1000
+# ADD: Entry: 0x1150
+# SUB: Entry: 0x1080
+# SET-ERR: error: bad entry point address: '-123'
+# SET-LARGE: Entry: 0x100000000
+# ADD-LARGE: Entry: 0x100001100
+# ADD-ERR: error: bad entry point increment: '-xyz'
+# ADD-UNDERFLOW: Entry: 0xFFFFFFFFFFFFFFFF

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

if (!Config.BuildIdLinkDir.empty() || Config.BuildIdLinkInput ||
       !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.SymbolsToAdd.empty()) {
+      !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "Option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config, COFFObjectFile &In,

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

struct CopyConfig {
   std::vector<NameOrRegex> ToRemove;
   std::vector<NameOrRegex> SymbolsToKeepGlobal;
 
   // Map options
   StringMap<SectionRename> SectionsToRename;
   StringMap<SectionFlagsUpdate> SetSectionFlags;
   StringMap<StringRef> SymbolsToRename;
 
+  // ELF entry point address expression. The input parameter is an entry point
+  // address in the input ELF file. The entry address in the output file is
+  // calculated with EntryExpr(input_address), when either --set-start or
+  // --change-start is used.
+  std::function<uint64_t(uint64_t)> EntryExpr;
+
   // Boolean options
   bool DeterministicArchives = true;
   bool ExtractDWO = false;
   bool KeepFileSymbols = false;
   bool LocalizeHidden = false;
   bool OnlyKeepDebug = false;
   bool PreserveDates = false;
   bool StripAll = false;

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

for (size_t LineNo = 0; LineNo < NumLines; ++LineNo) {
     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();
 }
+
+template <class T> static ErrorOr<T> getAsInteger(StringRef Val) {
+  T Result;
+  if (Val.getAsInteger(0, Result))
+    return errc::invalid_argument;
+  return Result;
+}
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
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);
 
+  for (auto Arg : InputArgs)
+    if (Arg->getOption().matches(OBJCOPY_set_start)) {
+      auto EAddr = getAsInteger<uint64_t>(Arg->getValue());
+      if (!EAddr)
+        return createStringError(
+            EAddr.getError(), "bad entry point address: '%s'", Arg->getValue());
+
+      Config.EntryExpr = [EAddr](uint64_t) { return *EAddr; };
+    } else if (Arg->getOption().matches(OBJCOPY_change_start)) {
+      auto EIncr = getAsInteger<int64_t>(Arg->getValue());
+      if (!EIncr)
+        return createStringError(EIncr.getError(),
+                                 "bad entry point increment: '%s'",
+                                 Arg->getValue());
+      auto Expr = Config.EntryExpr ? std::move(Config.EntryExpr)
+                                   : [](uint64_t A) { return A; };
+      Config.EntryExpr = [Expr, EIncr](uint64_t EAddr) {
+        return Expr(EAddr) + *EIncr;
+      };
+    }
+
   if (Config.DecompressDebugSections &&
       Config.CompressionType != DebugCompressionType::None) {
     return createStringError(
         errc::invalid_argument,
         "Cannot specify --compress-debug-sections at the same time as "
         "--decompress-debug-sections at the same time");
   }
 

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

static Error handleArgs(const CopyConfig &Config, Object &Obj,
   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,
                                Sec ? SYMBOL_SIMPLE_INDEX : SHN_ABS, 0);
   }
 
+  if (Config.EntryExpr)
+    Obj.Entry = Config.EntryExpr(Obj.Entry);
   return Error::success();
 }
 
 Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 Buffer &Out) {
   BinaryReader Reader(Config.BinaryArch, &In);
   std::unique_ptr<Object> Obj = Reader.create();
 

llvm/trunk/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 set_start : Eq<"set-start", "Set the start address to <addr>. Overrides "
+                    "any previous --change-start or --adjust-start values.">,
+                 MetaVarName<"addr">;
+defm change_start : Eq<"change-start", "Add <incr> to the start address. Can be "                        
+                       "specified multiple times, all values will be applied "
+                       "cumulatively.">,
+                    MetaVarName<"incr">;
+def adjust_start : JoinedOrSeparate<["-", "--"], "adjust-start">,
+                   Alias<change_start>;
+
 defm add_symbol
     : Eq<"add-symbol", "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.">,
       MetaVarName<"name=[section:]value[,flags]">;