Add adjustVMA option #72870

pwprzybyla · 2023-11-20T13:51:40Z

Add option to adjust section vma

llvmbot · 2023-11-20T13:52:10Z

@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (pwprzybyla)

Changes

Add option to adjust section vma

Full diff: https://github.com/llvm/llvm-project/pull/72870.diff

4 Files Affected:

(modified) llvm/include/llvm/ObjCopy/CommonConfig.h (+5)
(modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+14-1)
(modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+52)
(modified) llvm/tools/llvm-objcopy/ObjcopyOpts.td (+11)

diff --git a/llvm/include/llvm/ObjCopy/CommonConfig.h b/llvm/include/llvm/ObjCopy/CommonConfig.h
index e7ce1e6f2c54d75..1dd9dbb29dc372b 100644
--- a/llvm/include/llvm/ObjCopy/CommonConfig.h
+++ b/llvm/include/llvm/ObjCopy/CommonConfig.h
@@ -245,6 +245,11 @@ struct CommonConfig {
   StringMap<uint64_t> SetSectionType;
   StringMap<StringRef> SymbolsToRename;
 
+  // Adjust section VMA and LMA
+  int64_t SectionVMA = 0;
+  StringRef SectionName;
+  bool SectionSet = false;
+
   // Symbol info specified by --add-symbol option.
   std::vector<NewSymbolInfo> SymbolsToAdd;
 
diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index 9d02ba051a0a84b..245c11457db6c90 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -652,7 +652,20 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
         Sec.Align = I->second;
     }
   }
-
+  if (Config.SectionVMA != 0) {
+    for (SectionBase &Sec : Obj.sections()) {
+      bool AdjustThisSection = false;
+      if (Config.SectionName.empty() ||
+          Config.SectionName.contains(Sec.Name)){
+        AdjustThisSection = true;
+      }
+      if ((Sec.Flags & ELF::SHF_ALLOC) && AdjustThisSection){
+        if (Config.SectionSet)
+          Sec.Addr = 0;
+        Sec.Addr += Config.SectionVMA;
+      }
+    }
+  }
   if (Config.OnlyKeepDebug)
     for (auto &Sec : Obj.sections())
       if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index d33adb0b6a3e478..e2973c9171a408b 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -533,6 +533,45 @@ static Error loadNewSectionData(StringRef ArgValue, StringRef OptionName,
   return Error::success();
 }
 
+// readChangeSectionVMA reads the command line arguments and sets
+// sectionName and adjustValue of VMA and LMA addresses.
+static Error readChangeSectionVMA(StringRef ArgValue, StringRef OptionName,
+                                StringRef& SectionName, int64_t& AdjustValue, bool& setValue) {
+  SectionName = "";
+  AdjustValue = 0;
+  std::pair<StringRef, StringRef> SecPair;
+  if (ArgValue.contains("=")){
+    setValue = true;
+    SecPair = ArgValue.split("=");
+  }
+  else if (ArgValue.contains("+")){
+    setValue = false;
+    SecPair = ArgValue.split("+");
+  }
+  else if (ArgValue.contains("-")){
+    setValue = false;
+    SecPair = ArgValue.split("-");
+  }
+  else{
+    return createStringError(errc::invalid_argument,
+                            "bad format for " + OptionName + ": missing '='");
+  }
+  if (SecPair.second.empty())
+    return createStringError(errc::invalid_argument, "bad format for " +
+                                                         OptionName +
+                                                         ": missing offset of VMA");
+  SectionName = SecPair.first;
+  auto ChangeValue = getAsInteger<int64_t>(SecPair.second);
+  if (!ChangeValue)
+    return createStringError(ChangeValue.getError(),
+                              "Unable to parse adjustment value");
+  AdjustValue = *ChangeValue;
+  if (ArgValue.contains("-"))
+    AdjustValue *= -1;
+  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.
@@ -828,6 +867,11 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                                        Config.UpdateSection))
       return std::move(Err);
   }
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_adjust_section_vma)) {
+    if (Error Err = readChangeSectionVMA(Arg->getValue(), "--adjust-section-vma",
+                                       Config.SectionName, Config.SectionVMA, Config.SectionSet))
+      return std::move(Err);
+  }
   for (auto *Arg : InputArgs.filtered(OBJCOPY_dump_section)) {
     StringRef Value(Arg->getValue());
     if (Value.split('=').second.empty())
@@ -967,6 +1011,14 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
       ELFConfig.EntryExpr = [Expr, EIncr](uint64_t EAddr) {
         return Expr(EAddr) + *EIncr;
       };
+    } else if(Arg->getOption().matches(OBJCOPY_adjust_vma)){
+      auto EIncr = getAsInteger<int64_t>(Arg->getValue());
+      if (!EIncr)
+        return createStringError(EIncr.getError(),
+                                 "bad entry point VMA increment: '%s'",
+                                 Arg->getValue());
+      Config.SectionName = "";
+      Config.SectionVMA = *EIncr;
     }
 
   if (Config.DecompressDebugSections &&
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOpts.td b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
index ea8828637222ac6..c8adb26fb100d32 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOpts.td
+++ b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
@@ -230,3 +230,14 @@ defm add_symbol
 defm update_section
     : Eq<"update-section", "Replace the contents of section <name> with contents from a file <file>">,
       MetaVarName<"name=file">;
+
+defm adjust_section_vma
+    : Eq<"adjust-section-vma", "Adjust VMA and LMA of section <section> by <value>"
+        ""
+        "">,
+      MetaVarName<".section{=|+|-}value">;
+defm adjust_vma
+    : Eq<"adjust-vma", "Add <incr> to the vma and lma address."
+        ""
+        "">,
+      MetaVarName<"incr">;

github-actions · 2023-11-20T13:54:06Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jh7370

I haven't looked at the implementation yet, as I think there are a couple of high-level things that need addressing:

This change needs to update the llvm-objcopy docs located at llvm/docs/CommandGuide.
This change needs tests.
Should this option apply to platforms other than ELF? There's no need to implement those platforms, but it impacts things like what happens if the option is specified for a non-ELF input.
Please make sure your new/changed code is reformatted according to clang-format, but not other areas of code, to reduce noise in the patch.
A quick skim of the behaviour suggests that this only impacts section addresses. This may be correct, but could you confirm whether the option has any impact on program headers with GNU objcopy? In particular, can it result in a section address (sh_addr value) that is outside the program segment it should be in (compare it to the p_vaddr and p_memsz of the segments).
Does GNU objcopy do any sanity checking of the new address? E.g. does it stop you specifying a new address for a section if it would cause its address to overlap with another section?

jh7370 · 2023-11-21T08:02:51Z

llvm/include/llvm/ObjCopy/CommonConfig.h

+  // Adjust section VMA and LMA
+  int64_t SectionVMA = 0;
+  StringRef SectionName;
+  bool SectionSet = false;


Suggested change

// Adjust section VMA and LMA

int64_t SectionVMA = 0;

StringRef SectionName;

bool SectionSet = false;

// Adjust section VMA and LMA.

int64_t SectionVMA = 0;

StringRef SectionName;

bool SectionSet = false;

Could you be more precise with these variable names, please, since the SectionName and SectionSet variables could be for anything, and the SectionVMA variable should probably be named more after the corresponding option? Also, leverage std::optional rather than a boolean property, or even just treat an empty SectionName as unset, depending on how it's used.

jh7370 · 2023-11-21T08:03:30Z

llvm/tools/llvm-objcopy/ObjcopyOpts.td

-                                "symbols generated for binary input or added"
-                                " with --add-symbol unless otherwise"
-                                " specified. The default value is 'default'">;
+defm new_symbol_visibility


Please avoid reformatting the entire file in the middle of another change. If necessary, create an NFC patch to do the reformatting and base this PR on top of that.

jh7370 · 2023-11-21T08:08:06Z

llvm/tools/llvm-objcopy/ObjcopyOpts.td

      MetaVarName<"name=file">;
+
+defm adjust_section_vma


I think it makes sense to be able to specify this option multiple times? Also, it might make sense to add the --change-addresses/--change-section-address options at the same time (I think they're just aliases from the GNU objcopy docs, but I haven't experimented with them).

jh7370 · 2023-11-21T08:09:50Z

llvm/tools/llvm-objcopy/ObjcopyOpts.td

+         ""
+         "">,
+      MetaVarName<".section{=|+|-}value">;


Please tidy up the formatting (you don't need these empty strings for example).

The GNU docs imply that the --adjust-section-vma argument takes a section name pattern, not just a section name, but this metavar and option description don't imply this. Please revise accordingly.

Indeed GNU docs use pattern matching here I expect to use full name of section.

pwprzybyla · 2023-12-14T09:08:17Z

I haven't looked at the implementation yet, as I think there are a couple of high-level things that need addressing:

1. This change needs to update the llvm-objcopy docs located at llvm/docs/CommandGuide.

2. This change needs tests.

3. Should this option apply to platforms other than ELF? There's no need to implement those platforms, but it impacts things like what happens if the option is specified for a non-ELF input.

4. Please make sure your new/changed code is reformatted according to clang-format, but not other areas of code, to reduce noise in the patch.

5. A quick skim of the behaviour suggests that this only impacts section addresses. This may be correct, but could you confirm whether the option has any impact on program headers with GNU objcopy? In particular, can it result in a section address (sh_addr value) that is outside the program segment it should be in (compare it to the `p_vaddr` and `p_memsz` of the segments).

6. Does GNU objcopy do any sanity checking of the new address? E.g. does it stop you specifying a new address for a section if it would cause its address to overlap with another section?

Ad 3) parameter handled also in other file formats.
Ad 5,6) there are no other checks implemented. No out of bounds checking or overlap checks.

jh7370

Ad 5,6) there are no other checks implemented. No out of bounds checking or overlap checks.

I just tested on my Linux machine, using GNU objcopy and this doesn't appear to be correct. GNU objcopy complains if you try to adjust the VMA of a section in a way that causes problems with the program headers.

I'm also a little confused how just adjusting the VMA of the section achieves anything without changing the program header addresses, because the section address isn't used by the loader. What is the use case for this?

jh7370 · 2024-01-08T09:01:13Z

llvm/docs/CommandGuide/llvm-objcopy.rst

@@ -483,6 +483,12 @@ them.

 Mark all defined global symbols as weak in the output.

+.. option:: --adjust-vma <value>
+  Add ``<value>`` to VMA and LMA address


Suggested change

Add ``<value>`` to VMA and LMA address

Add ``<value>`` to VMA and LMA address.

Address of what?

jh7370 · 2024-01-08T09:04:50Z

llvm/docs/CommandGuide/llvm-objcopy.rst

+  Add ``<value>`` to VMA and LMA address
+
+.. option:: --adjust-section-vma <name>{+|-|=}<value>
+  Adjust section ``<name>`` VMA and LMA address by ``<value>``


Suggested change

Adjust section ``<name>`` VMA and LMA address by ``<value>``

Adjust section ``<name>`` VMA and LMA address by ``<value>``.

I think this could do with some elaboration regarding what = means in this context (presumably it means setting it to the value specified, but that isn't "adjust it by something").

jh7370 · 2024-01-08T09:12:04Z

llvm/include/llvm/ObjCopy/CommonConfig.h

@@ -245,6 +253,9 @@ struct CommonConfig {
  StringMap<uint64_t> SetSectionType;
  StringMap<StringRef> SymbolsToRename;

+  // Adjust section VMA and LMA


Suggested change

// Adjust section VMA and LMA

// Adjust section VMA and LMA.

Comments should use correct punctuation. That being said, this comment doesn't add any value and the member should just be added to the immediately above set of StringMap members.

jh7370 · 2024-01-08T09:25:46Z

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

@@ -316,6 +316,12 @@ static Error handleArgs(const CommonConfig &Config,
      Obj.PeHeader.MinorSubsystemVersion = *COFFConfig.MinorSubsystemVersion;
  }

+  if (!Config.AdjustSectionVMA.empty()) {


This isn't the correct place to handle unsupported options. Please see ConfigManager.cpp for how to handle options that are not yet implemented for some platforms, but might be in the future.

Add option to adjust section vma

jh7370 · 2024-03-20T08:49:53Z

Ad 5,6) there are no other checks implemented. No out of bounds checking or overlap checks.

I just tested on my Linux machine, using GNU objcopy and this doesn't appear to be correct. GNU objcopy complains if you try to adjust the VMA of a section in a way that causes problems with the program headers.

I'm also a little confused how just adjusting the VMA of the section achieves anything without changing the program header addresses, because the section address isn't used by the loader. What is the use case for this?

Please respond to these comments. I won't be reviewing further until you do.

llvmbot added the llvm:binary-utilities label Nov 20, 2023

pwprzybyla force-pushed the main branch from 7927856 to 306d9ea Compare November 20, 2023 14:10

jh7370 requested review from MaskRay and jh7370 November 21, 2023 07:59

jh7370 requested changes Nov 21, 2023

View reviewed changes

pwprzybyla force-pushed the main branch from 306d9ea to 12d0412 Compare December 14, 2023 09:03

llvmbot added the backend:WebAssembly label Dec 14, 2023

pwprzybyla requested a review from jh7370 December 22, 2023 14:20

jh7370 requested changes Jan 8, 2024

View reviewed changes

Add adjustVMA option

8b4fbe0

Add option to adjust section vma

pwprzybyla force-pushed the main branch from 12d0412 to 8b4fbe0 Compare February 14, 2024 12:39

pwprzybyla requested a review from jh7370 March 19, 2024 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add adjustVMA option #72870

Add adjustVMA option #72870

pwprzybyla commented Nov 20, 2023

llvmbot commented Nov 20, 2023 •

edited

github-actions bot commented Nov 20, 2023 •

edited

jh7370 left a comment

jh7370 Nov 21, 2023

jh7370 Nov 21, 2023

jh7370 Nov 21, 2023

jh7370 Nov 21, 2023

pwprzybyla Dec 14, 2023

pwprzybyla commented Dec 14, 2023

jh7370 left a comment

jh7370 Jan 8, 2024

jh7370 Jan 8, 2024

jh7370 Jan 8, 2024

jh7370 Jan 8, 2024

jh7370 commented Mar 20, 2024

	Add ``<value>`` to VMA and LMA address
	Add ``<value>`` to VMA and LMA address.

	Adjust section ``<name>`` VMA and LMA address by ``<value>``
	Adjust section ``<name>`` VMA and LMA address by ``<value>``.

Add adjustVMA option #72870

Are you sure you want to change the base?

Add adjustVMA option #72870

Conversation

pwprzybyla commented Nov 20, 2023

llvmbot commented Nov 20, 2023 • edited

github-actions bot commented Nov 20, 2023 • edited

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwprzybyla commented Dec 14, 2023

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 commented Mar 20, 2024

llvmbot commented Nov 20, 2023 •

edited

github-actions bot commented Nov 20, 2023 •

edited