Skip to content

[lld][COFF] Fill only gaps in code sections. #72138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 14, 2023
Merged

[lld][COFF] Fill only gaps in code sections. #72138

merged 1 commit into from
Nov 14, 2023

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Nov 13, 2023

Filling entire buffer would require all chunks to overwrite it later, which is not the case for uninitialized chunks merged into a code section.

Fixes the problem I mentioned in #72030.

@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2023 •
edited
Loading

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld

Author: Jacek Caban (cjacek)

Changes

Filling entire buffer would require all chunks to overwrite it later, which is not the case for uninitialized chunks merged into a code section.

Fixes the problem I mentioned in #72030.

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

2 Files Affected:

  • (modified) lld/COFF/Writer.cpp (+10-2)
  • (added) lld/test/COFF/code-merge.s (+146)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 218a4f336fedc6f..be7265cfc194411 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -2021,8 +2021,16 @@ void Writer::writeSections() {
     // instead of leaving as NUL bytes (which can be interpreted as
     // ADD instructions).
     if ((sec->header.Characteristics & IMAGE_SCN_CNT_CODE) &&
-        (ctx.config.machine == AMD64 || ctx.config.machine == I386))
-      memset(secBuf, 0xCC, sec->getRawSize());
+        (ctx.config.machine == AMD64 || ctx.config.machine == I386)) {
+      uint32_t prevEnd = 0;
+      for (Chunk *c : sec->chunks) {
+        uint32_t off = c->getRVA() - sec->getRVA();
+        memset(secBuf + prevEnd, 0xCC, off - prevEnd);
+        prevEnd = off + c->getSize();
+      }
+      memset(secBuf + prevEnd, 0xCC, sec->getRawSize() - prevEnd);
+    }
+
     parallelForEach(sec->chunks, [&](Chunk *c) {
       c->writeTo(secBuf + c->getRVA() - sec->getRVA());
     });
diff --git a/lld/test/COFF/code-merge.s b/lld/test/COFF/code-merge.s
new file mode 100644
index 000000000000000..e7aa86533ce547a
--- /dev/null
+++ b/lld/test/COFF/code-merge.s
@@ -0,0 +1,146 @@
+# REQUIRES: x86
+
+# Test merging code sections with data sections.
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
+# RUN: lld-link -machine:amd64 -dll -noentry -out:%t.dll %t.obj -merge:.testx=.testd -merge:.testx2=.testbss -merge:.testd2=.testx3 -merge:.testbss2=.testx4
+
+# RUN: llvm-readobj --sections %t.dll | FileCheck %s
+# CHECK:      Sections [
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 1
+# CHECK-NEXT:     Name: .testbss (2E 74 65 73 74 62 73 73)
+# CHECK-NEXT:     VirtualSize: 0x18
+# CHECK-NEXT:     VirtualAddress: 0x1000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x400
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0xC0000080)
+# CHECK-NEXT:       IMAGE_SCN_CNT_UNINITIALIZED_DATA (0x80)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_WRITE (0x80000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 2
+# CHECK-NEXT:     Name: .testd (2E 74 65 73 74 64 00 00)
+# CHECK-NEXT:     VirtualSize: 0x18
+# CHECK-NEXT:     VirtualAddress: 0x2000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x600
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x40000040)
+# CHECK-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA (0x40)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 3
+# CHECK-NEXT:     Name: .testx3 (2E 74 65 73 74 78 33 00)
+# CHECK-NEXT:     VirtualSize: 0x12
+# CHECK-NEXT:     VirtualAddress: 0x3000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x800
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x60000020)
+# CHECK-NEXT:       IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-NEXT:       IMAGE_SCN_MEM_EXECUTE (0x20000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 4
+# CHECK-NEXT:     Name: .testx4 (2E 74 65 73 74 78 34 00)
+# CHECK-NEXT:     VirtualSize: 0x14
+# CHECK-NEXT:     VirtualAddress: 0x4000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0xA00
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x60000020)
+# CHECK-NEXT:       IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-NEXT:       IMAGE_SCN_MEM_EXECUTE (0x20000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+# RUN: llvm-objdump -d %t.dll | FileCheck -check-prefix=DISASM %s
+# DISASM:      Disassembly of section .testx3:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180003000 <.testx3>:
+# DISASM-NEXT: 180003000: 48 c7 c0 03 00 00 00         movq    $0x3, %rax
+# DISASM-NEXT: 180003007: c3                           retq
+# DISASM-NEXT: 180003008: cc                           int3
+# DISASM-NEXT: 180003009: cc                           int3
+# DISASM-NEXT: 18000300a: cc                           int3
+# DISASM-NEXT: 18000300b: cc                           int3
+# DISASM-NEXT: 18000300c: cc                           int3
+# DISASM-NEXT: 18000300d: cc                           int3
+# DISASM-NEXT: 18000300e: cc                           int3
+# DISASM-NEXT: 18000300f: cc                           int3
+# DISASM-NEXT: 180003010: 02 00                        addb    (%rax), %al
+# DISASM-EMPTY:
+# DISASM-NEXT: Disassembly of section .testx4:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180004000 <.testx4>:
+# DISASM-NEXT: 180004000: 48 c7 c0 04 00 00 00         movq    $0x4, %rax
+# DISASM-NEXT: 180004007: c3                           retq
+# DISASM-NEXT: 180004008: cc                           int3
+# DISASM-NEXT: 180004009: cc                           int3
+# DISASM-NEXT: 18000400a: cc                           int3
+# DISASM-NEXT: 18000400b: cc                           int3
+# DISASM-NEXT: 18000400c: cc                           int3
+# DISASM-NEXT: 18000400d: cc                           int3
+# DISASM-NEXT: 18000400e: cc                           int3
+# DISASM-NEXT: 18000400f: cc                           int3
+# DISASM-NEXT: 180004010: 00 00                        addb    %al, (%rax)
+# DISASM-NEXT: 180004012: 00 00                        addb    %al, (%rax)
+
+
+        .section .testx, "xr"
+        .p2align 4
+        movq $1, %rax
+        retq
+
+        .section .testx2, "xr"
+        .p2align 4
+        movq $2, %rax
+        retq
+
+        .section .testd, "dr"
+        .p2align 4
+        .word 1
+
+        .section .testbss, "b"
+        .p2align 4
+        .skip 4
+
+        .section .testx3, "xr"
+        .p2align 4
+        movq $3, %rax
+        retq
+
+        .section .testx4, "xr"
+        .p2align 4
+        movq $4, %rax
+        retq
+
+        .section .testd2, "dr"
+        .p2align 4
+        .word 2
+
+        .section .testbss2, "b"
+        .p2align 4
+        .skip 4

@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2023

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

Filling entire buffer would require all chunks to overwrite it later, which is not the case for uninitialized chunks merged into a code section.

Fixes the problem I mentioned in #72030.

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

2 Files Affected:

  • (modified) lld/COFF/Writer.cpp (+10-2)
  • (added) lld/test/COFF/code-merge.s (+146)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 218a4f336fedc6f..be7265cfc194411 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -2021,8 +2021,16 @@ void Writer::writeSections() {
     // instead of leaving as NUL bytes (which can be interpreted as
     // ADD instructions).
     if ((sec->header.Characteristics & IMAGE_SCN_CNT_CODE) &&
-        (ctx.config.machine == AMD64 || ctx.config.machine == I386))
-      memset(secBuf, 0xCC, sec->getRawSize());
+        (ctx.config.machine == AMD64 || ctx.config.machine == I386)) {
+      uint32_t prevEnd = 0;
+      for (Chunk *c : sec->chunks) {
+        uint32_t off = c->getRVA() - sec->getRVA();
+        memset(secBuf + prevEnd, 0xCC, off - prevEnd);
+        prevEnd = off + c->getSize();
+      }
+      memset(secBuf + prevEnd, 0xCC, sec->getRawSize() - prevEnd);
+    }
+
     parallelForEach(sec->chunks, [&](Chunk *c) {
       c->writeTo(secBuf + c->getRVA() - sec->getRVA());
     });
diff --git a/lld/test/COFF/code-merge.s b/lld/test/COFF/code-merge.s
new file mode 100644
index 000000000000000..e7aa86533ce547a
--- /dev/null
+++ b/lld/test/COFF/code-merge.s
@@ -0,0 +1,146 @@
+# REQUIRES: x86
+
+# Test merging code sections with data sections.
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
+# RUN: lld-link -machine:amd64 -dll -noentry -out:%t.dll %t.obj -merge:.testx=.testd -merge:.testx2=.testbss -merge:.testd2=.testx3 -merge:.testbss2=.testx4
+
+# RUN: llvm-readobj --sections %t.dll | FileCheck %s
+# CHECK:      Sections [
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 1
+# CHECK-NEXT:     Name: .testbss (2E 74 65 73 74 62 73 73)
+# CHECK-NEXT:     VirtualSize: 0x18
+# CHECK-NEXT:     VirtualAddress: 0x1000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x400
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0xC0000080)
+# CHECK-NEXT:       IMAGE_SCN_CNT_UNINITIALIZED_DATA (0x80)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_WRITE (0x80000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 2
+# CHECK-NEXT:     Name: .testd (2E 74 65 73 74 64 00 00)
+# CHECK-NEXT:     VirtualSize: 0x18
+# CHECK-NEXT:     VirtualAddress: 0x2000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x600
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x40000040)
+# CHECK-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA (0x40)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 3
+# CHECK-NEXT:     Name: .testx3 (2E 74 65 73 74 78 33 00)
+# CHECK-NEXT:     VirtualSize: 0x12
+# CHECK-NEXT:     VirtualAddress: 0x3000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0x800
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x60000020)
+# CHECK-NEXT:       IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-NEXT:       IMAGE_SCN_MEM_EXECUTE (0x20000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Number: 4
+# CHECK-NEXT:     Name: .testx4 (2E 74 65 73 74 78 34 00)
+# CHECK-NEXT:     VirtualSize: 0x14
+# CHECK-NEXT:     VirtualAddress: 0x4000
+# CHECK-NEXT:     RawDataSize: 512
+# CHECK-NEXT:     PointerToRawData: 0xA00
+# CHECK-NEXT:     PointerToRelocations: 0x0
+# CHECK-NEXT:     PointerToLineNumbers: 0x0
+# CHECK-NEXT:     RelocationCount: 0
+# CHECK-NEXT:     LineNumberCount: 0
+# CHECK-NEXT:     Characteristics [ (0x60000020)
+# CHECK-NEXT:       IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-NEXT:       IMAGE_SCN_MEM_EXECUTE (0x20000000)
+# CHECK-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+# RUN: llvm-objdump -d %t.dll | FileCheck -check-prefix=DISASM %s
+# DISASM:      Disassembly of section .testx3:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180003000 <.testx3>:
+# DISASM-NEXT: 180003000: 48 c7 c0 03 00 00 00         movq    $0x3, %rax
+# DISASM-NEXT: 180003007: c3                           retq
+# DISASM-NEXT: 180003008: cc                           int3
+# DISASM-NEXT: 180003009: cc                           int3
+# DISASM-NEXT: 18000300a: cc                           int3
+# DISASM-NEXT: 18000300b: cc                           int3
+# DISASM-NEXT: 18000300c: cc                           int3
+# DISASM-NEXT: 18000300d: cc                           int3
+# DISASM-NEXT: 18000300e: cc                           int3
+# DISASM-NEXT: 18000300f: cc                           int3
+# DISASM-NEXT: 180003010: 02 00                        addb    (%rax), %al
+# DISASM-EMPTY:
+# DISASM-NEXT: Disassembly of section .testx4:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180004000 <.testx4>:
+# DISASM-NEXT: 180004000: 48 c7 c0 04 00 00 00         movq    $0x4, %rax
+# DISASM-NEXT: 180004007: c3                           retq
+# DISASM-NEXT: 180004008: cc                           int3
+# DISASM-NEXT: 180004009: cc                           int3
+# DISASM-NEXT: 18000400a: cc                           int3
+# DISASM-NEXT: 18000400b: cc                           int3
+# DISASM-NEXT: 18000400c: cc                           int3
+# DISASM-NEXT: 18000400d: cc                           int3
+# DISASM-NEXT: 18000400e: cc                           int3
+# DISASM-NEXT: 18000400f: cc                           int3
+# DISASM-NEXT: 180004010: 00 00                        addb    %al, (%rax)
+# DISASM-NEXT: 180004012: 00 00                        addb    %al, (%rax)
+
+
+        .section .testx, "xr"
+        .p2align 4
+        movq $1, %rax
+        retq
+
+        .section .testx2, "xr"
+        .p2align 4
+        movq $2, %rax
+        retq
+
+        .section .testd, "dr"
+        .p2align 4
+        .word 1
+
+        .section .testbss, "b"
+        .p2align 4
+        .skip 4
+
+        .section .testx3, "xr"
+        .p2align 4
+        movq $3, %rax
+        retq
+
+        .section .testx4, "xr"
+        .p2align 4
+        movq $4, %rax
+        retq
+
+        .section .testd2, "dr"
+        .p2align 4
+        .word 2
+
+        .section .testbss2, "b"
+        .p2align 4
+        .skip 4

@cjacek cjacek mentioned this pull request Nov 13, 2023
Merged
mstorsjo
mstorsjo approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this seems mostly straightforward.

lld/COFF/Writer.cpp
memset(secBuf, 0xCC, sec->getRawSize());
(ctx.config.machine == AMD64 || ctx.config.machine == I386)) {
uint32_t prevEnd = 0;
for (Chunk *c : sec->chunks) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to leave a comment here, saying what this does. E.g. something like this? "Only fill the gaps between chunks. Most chunks overwrite it anyway, but uninitialized data chunks merged into a code section don't."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

@cjacek
[lld][COFF] Fill only gaps in code sections.
27f05d1 
Filling entire buffer would require all chunks to overwrite it later, which is not the case for uninitialized chunks merged into a code section.
@cjacek cjacek merged commit 54f83e6 into llvm:main Nov 14, 2023
@cjacek cjacek deleted the lld-fill-gaps branch November 14, 2023 19:48
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
@cjacek @zahiraam
[lld][COFF] Fill only gaps in code sections. (llvm#72138)
d4b121e 
Filling entire buffer would require all chunks to overwrite it later,
which is not the case for uninitialized chunks merged into code
sections.
@cjacek cjacek mentioned this pull request Nov 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstorsjo mstorsjo mstorsjo approved these changes

@rnk rnk Awaiting requested review from rnk

@compnerd compnerd Awaiting requested review from compnerd

@MaskRay MaskRay Awaiting requested review from MaskRay

Assignees
No one assigned
Labels
lld:COFF lld platform:windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cjacek @llvmbot @mstorsjo