[llvm-objcopy] Let --change-section-lma change segments wth filesz=0,… #127724

jonathonpenix · 2025-02-19T00:07:23Z

… memsz>0

Currently, segments with a file size of 0 are ignored for the purposes of --change-section-lma, regardless of their memory size. It seems reasonable to me to modify such segments given that we're changing the LMA for all sections and these LMAs may be used during loading. GNU objcopy also seems to adjust such segments.

Additionally, segments with file size > 0 and memory size = 0 will no longer be modified for the purposes of --change-section-lma as they shouldn't be part of the loaded memory image.

Fixes #124680

… memsz>0 Currently, segments with a file size of 0 are ignored for the purposes of --change-section-lma, regardless of their memory size. It seems reasonable to me to modify such segments given that we're changing the LMA for all sections and these LMAs may be used during loading. GNU objcopy also seems to adjust such segments. Fixes llvm#124680

llvmbot · 2025-02-19T00:07:53Z

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

Author: Jonathon Penix (jonathonpenix)

Changes

… memsz>0

Currently, segments with a file size of 0 are ignored for the purposes of --change-section-lma, regardless of their memory size. It seems reasonable to me to modify such segments given that we're changing the LMA for all sections and these LMAs may be used during loading. GNU objcopy also seems to adjust such segments.

Fixes #124680

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

2 Files Affected:

(modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+1-1)
(modified) llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test (+13-4)

diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index 9c78f7433ad33..20a8e67625cac 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -824,7 +824,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
 
   if (Config.ChangeSectionLMAValAll != 0) {
     for (Segment &Seg : Obj.segments()) {
-      if (Seg.FileSize > 0) {
+      if (Seg.FileSize > 0 || Seg.MemSize > 0) {
         if (Config.ChangeSectionLMAValAll > 0 &&
             Seg.PAddr > std::numeric_limits<uint64_t>::max() -
                             Config.ChangeSectionLMAValAll) {
diff --git a/llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test b/llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test
index c1cd1eb46d949..00a9e6de8d4a8 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test
@@ -14,16 +14,18 @@
 
 # CHECK-PLUS-PROGRAMS:  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz
 # CHECK-PLUS-PROGRAMS:  PHDR           0x000002 0x0000000000001102 0x0000000000001122 0x000038 0x000000
-# CHECK-PLUS-PROGRAMS:  LOAD           0x000000 0x0000000000001100 0x0000000000001120 0x000258 0x000258
-# CHECK-PLUS-PROGRAMS:  LOAD           0x000258 0xffffffff00005100 0xffffffff00006120 0x000100 0x000100
+# CHECK-PLUS-PROGRAMS:  LOAD           0x000000 0x0000000000001100 0x0000000000001120 0x000290 0x000290
+# CHECK-PLUS-PROGRAMS:  LOAD           0x000290 0xffffffff00005100 0xffffffff00006120 0x000100 0x000100
 # CHECK-PLUS-PROGRAMS:  NOTE           0x000358 0x0000000000001200 0x0000000000001220 0x000010 0x000000
 # CHECK-PLUS-PROGRAMS:  NOTE           0x000368 0x0000000000000000 0x0000000000000000 0x000000 0x000000
+# CHECK-PLUS-PROGRAMS:  LOAD           0x000390 0x0000000000001300 0x0000000000001320 0x000000 0x000010
 
 # CHECK-MINUS-PROGRAMS:  PHDR           0x000002 0x0000000000001102 0x00000000000010d2 0x000038 0x000000
-# CHECK-MINUS-PROGRAMS:  LOAD           0x000000 0x0000000000001100 0x00000000000010d0 0x000258 0x000258
-# CHECK-MINUS-PROGRAMS:  LOAD           0x000258 0xffffffff00005100 0xffffffff000060d0 0x000100 0x000100
+# CHECK-MINUS-PROGRAMS:  LOAD           0x000000 0x0000000000001100 0x00000000000010d0 0x000290 0x000290
+# CHECK-MINUS-PROGRAMS:  LOAD           0x000290 0xffffffff00005100 0xffffffff000060d0 0x000100 0x000100
 # CHECK-MINUS-PROGRAMS:  NOTE           0x000358 0x0000000000001200 0x00000000000011d0 0x000010 0x000000
 # CHECK-MINUS-PROGRAMS:  NOTE           0x000368 0x0000000000000000 0x0000000000000000 0x000000 0x000000
+# CHECK-MINUS-PROGRAMS:  LOAD           0x000390 0x0000000000001300 0x00000000000012d0 0x000000 0x000010
 
 # CHECK-PLUS-SECTIONS:      [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
 # CHECK-PLUS-SECTIONS:           .text1
@@ -55,6 +57,9 @@ Sections:
   - Name:          .text2
     Type:          SHT_PROGBITS
     Size:          0x100
+  - Name:          .bss1
+    Type:          SHT_NOBITS
+    Size:          0x10
 ProgramHeaders:
   - Type:          PT_PHDR
     FileSize:      0x38
@@ -77,3 +82,7 @@ ProgramHeaders:
   - Type:          PT_NOTE
     FileSize:      0x0
     Offset:        0x368
+  - Type:          PT_LOAD
+    VAddr:         0x1300
+    FirstSec:      .bss1
+    LastSec:       .bss1

jh7370

@MaskRay, I'd appreciate your thoughts on this. I think the fundamental change is correct, but would appreciate you double-checking my understanding of the zero size bit I mentioned.

jh7370 · 2025-02-24T09:54:27Z

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

@@ -824,7 +824,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,

  if (Config.ChangeSectionLMAValAll != 0) {
    for (Segment &Seg : Obj.segments()) {
-      if (Seg.FileSize > 0) {
+      if (Seg.FileSize > 0 || Seg.MemSize > 0) {


Should this check simply be if (Seg.MemSize > 0)? The difference would be that sections with file size and zero mem size would not be changed, but such segments are only valid if they are not PT_LOAD segments and presumably aren't part of the loaded file image.

I didn't have a particularly principled reason for not doing this in the first place. Broadly, I was seeing such segments (non-PT_LOAD, filesize >0, memsize = 0) generated by ld.bfd for ex: the RISC-V PT_RISCV_ATTRIBUTES segment, and GNU objcopy seemed to modify the LMA but also would at times touch the offset/filesize/memsize in ways I didn't entirely follow. As such, I tried to err towards being consistent with our previous behavior here (to modify such segments).

That said, I think what you said about "presumably aren't part of the loaded file image" seems correct to me, so I went ahead and made the change/added an explicit test for this/updated the description--hopefully this looks reasonable!

…lesz=0, memsz>0

jh7370

Thanks, this LGTM now, with one minor nit.

I checked a downstream toolchain with non-loaded segments and they definitely don't have a memsize in this case, so I think the new check is the correct one.

Do you need assistance with merging?

jh7370 · 2025-02-26T08:32:06Z

llvm/test/tools/llvm-objcopy/ELF/change-section-lma.test

+  - Type:          PT_NOTE
+    FileSize:      0x10
+    MemSize:       0x0
+    VAddr:         0x1400


Nit: I don't think a VAddr makes sense for a segment that is not loaded.

…lesz=0, memsz>0

jonathonpenix · 2025-02-26T19:14:10Z

Sounds good, thank you for your help with this!

I can merge this--I'll probably let it sit for a day or so just in case there are any additional follow-ups from anyone before doing so though.

MaskRay · 2025-03-01T19:55:06Z

LGTM. I agree that checking just p_memsz > 0 suffices.

jonathonpenix requested a review from eleanor-arm February 19, 2025 00:07

llvmbot added the llvm:binary-utilities label Feb 19, 2025

jonathonpenix requested review from MaskRay and jh7370 February 19, 2025 00:07

jh7370 reviewed Feb 24, 2025

View reviewed changes

fixup! [llvm-objcopy] Let --change-section-lma change segments wth fi…

3d23251

…lesz=0, memsz>0

jonathonpenix requested a review from jh7370 February 25, 2025 00:12

jh7370 approved these changes Feb 26, 2025

View reviewed changes

fixup! [llvm-objcopy] Let --change-section-lma change segments wth fi…

91ebc90

…lesz=0, memsz>0

jonathonpenix merged commit ffecd72 into llvm:main Feb 27, 2025
11 checks passed

jonathonpenix deleted the objcopy_lma branch February 27, 2025 21:48

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[llvm-objcopy] Let --change-section-lma change segments wth filesz=0,… #127724

[llvm-objcopy] Let --change-section-lma change segments wth filesz=0,… #127724

Uh oh!

jonathonpenix commented Feb 19, 2025 •

edited

Loading

Uh oh!

llvmbot commented Feb 19, 2025

Uh oh!

jh7370 left a comment •

edited

Loading

Uh oh!

jh7370 Feb 24, 2025

Uh oh!

jonathonpenix Feb 25, 2025

Uh oh!

jh7370 left a comment

Uh oh!

jh7370 Feb 26, 2025

Uh oh!

jonathonpenix Feb 26, 2025

Uh oh!

jonathonpenix commented Feb 26, 2025

Uh oh!

Uh oh!

MaskRay commented Mar 1, 2025

Uh oh!

Uh oh!

[llvm-objcopy] Let --change-section-lma change segments wth filesz=0,… #127724

[llvm-objcopy] Let --change-section-lma change segments wth filesz=0,… #127724

Uh oh!

Conversation

jonathonpenix commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 19, 2025

Uh oh!

jh7370 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jh7370 Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

jonathonpenix Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

jonathonpenix Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

jonathonpenix commented Feb 26, 2025

Uh oh!

Uh oh!

MaskRay commented Mar 1, 2025

Uh oh!

Uh oh!

jonathonpenix commented Feb 19, 2025 •

edited

Loading

jh7370 left a comment •

edited

Loading