Skip to content

fix: Implement RELR bitmap packing for consecutive relocations#2179

Merged
davidlattimore merged 4 commits into
wild-linker:mainfrom
deepakshirkem:feat/relr-bitmap-packing
Jul 6, 2026
Merged

fix: Implement RELR bitmap packing for consecutive relocations#2179
davidlattimore merged 4 commits into
wild-linker:mainfrom
deepakshirkem:feat/relr-bitmap-packing

Conversation

@deepakshirkem

Copy link
Copy Markdown
Member

Pack consecutive 8-byte-aligned RELR relocations within input sections into bitmap entries, matching lld's output.

Before: 4 consecutive pointers → 4 RELR entries (32 bytes)
After: 4 consecutive pointers → 2 RELR entries (16 bytes)

Only packs within input sections as suggested by David — avoids cross-section ordering issues in Wild's parallel layout phase.

Issue #1853

@davidlattimore davidlattimore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

90% was a complete guess on my part, so we probably shouldn't put that in the comments

Comment thread libwild/src/elf.rs Outdated
/// cross-section address ordering issues in Wild's parallel layout phase.
struct RelrRunState {
/// Offset of the last RELR-eligible relocation seen in the current run.
last_offset: Option<u64>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The bitmap can contain 1s and 0s, which means that non-consecutive relocations can still be packed into the same bitmap - skipped relocations just encode as a 0. We do however need some additional state, since we need to know how much of the current bitmap remains, after which we'll need to allocate a new bitmap. If ever a relocation address indicates that we would have needed an empty bitmap, that's the point at which we should probably end the previous run and start a new one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated both the layout counting and writer to handle gaps within the 63-slot window.

I think we hit a structural issue with the allocate_resolution path (GOT-based RELR entries). That function is called inside finalise_symbol_sizes which iterates in symbol-ID order, but the writer packs in
output-address order. These don't match, causing validate_empty failures on aarch64/riscv64/loongarch64 (16 of 32 bytes remain layout counts 4 flat entries, writer packs them into 2).

Two options we can see:

  1. Skip bitmap packing for GOT-based RELR entries entirely count and write flat in both phases. Loses some savings but is consistent.
  2. Count GOT entries in output-address order during layout — unclear if this is feasible without deeper changes to the layout pipeline.

What's the right approach here?

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch from a58a2e1 to f3660de Compare July 4, 2026 06:46

@davidlattimore davidlattimore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's best to leave the GOT for a separate PR. One possible strategy for dealing with it might be to split the GOT into two parts with all the relative relocations together in one part. If we know that they're all together and how many there are, then allocation and writing should be pretty straightforward. But definitely a separate PR for something like that.

Comment thread libwild/src/elf.rs Outdated
}
}

struct RelrRunState {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this might be easier to understand if it were an enum instead, with a variant for each state

Comment thread libwild/src/elf_writer.rs Outdated
relr_bitmap_index: Option<usize>,
/// Address of the start of the current RELR run.
relr_run_base: Option<u64>,
/// Last address written to relr_dyn, to detect consecutive +8 runs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment seems to be referring to a field that doesn't exist

Comment thread libwild/src/elf_writer.rs Outdated
relr_dyn: Option<&'out mut [elf::Relr]>,
/// Index of next free slot in relr_dyn.
relr_dyn_index: usize,
/// Index of the current bitmap entry in relr_dyn, if any.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could plausibly have a similar enum-based state machine here - plausibly, the same enum might be able to be used.

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch 2 times, most recently from 10171eb to 7907af1 Compare July 4, 2026 08:38
@deepakshirkem

Copy link
Copy Markdown
Member Author

While verifying against lld on the existing pack-relative-relocs test, I noticed Wild produces 5 RELR entries vs lld's 4 for the test binary.

Reproducer (relr.c):

static int a, b, c, d;
static int *p1 = &a;
static int *p2 = &b;
static int *p3 = &c;
static int *p4 = &d;
int get() { return *p1 + *p2 + *p3 + *p4; }

gcc -fPIC -c relr.c -o relr.o

# lld: 2 entries (1 address + 1 bitmap 0x0f)
ld.lld -shared -z pack-relative-relocs relr.o -o relr.lld.so
readelf -x .relr.dyn relr.lld.so

# Wild: also 2 entries — matches lld for this simple case
./target/release/wild -shared -z pack-relative-relocs relr.o -o relr.wild.so
readelf -x .relr.dyn relr.wild.so

For the existing pack-relative-relocs test binary however Wild produces 5 entries vs lld's 4. The difference comes from .data.rel.local which has relocations at offsets 0, 9, and 26. Offset 9 is odd (RELA fallback). Offsets 0 and 26 are both RELR-eligible but delta=26 is not a multiple of 8, so they can't share a bitmap within the section each gets its own address entry. lld packs globally and sees them consecutive in the output.

This is the known cost of within-section-only packing. Is this acceptable for this PR or would you suggest a different approach?

@davidlattimore davidlattimore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine that we won't get quite as minimal packing as we would if we packed between sections. If we want to compare RELR bitmap utilisation, I'd suggest a full sized program rather than one of our test binaries, which are too small to really say much.

BTW, is it intentional that the PR is marked as a draft?

Comment thread libwild/src/elf_writer.rs Outdated
}
}
} else {
// Out of window — start new run with address entry.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we run out of bits, we can start a new bitmap

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, intentionally draft planning to add a test case before marking ready for review. Will also fix the bitmap chaining (new bitmap entry instead of new address entry when delta > 63*8) before marking ready.

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch 2 times, most recently from 2faf89d to 2ea5c95 Compare July 4, 2026 11:50
Track consecutive RELR-eligible relocations within each input section
during the layout phase. Instead of allocating one RELR entry per
eligible relocation, allocate one address entry per run and one bitmap
entry per run of 2+ consecutive relocations. Third and subsequent
members of a run fit into the existing bitmap entry at no extra cost.

This fixes the layout-phase size calculation for Issue wild-linker#1853. The writer
phase (elf_writer.rs) still needs to be updated to emit actual bitmap
entries — that will follow in the next commit.
Instead of writing one address entry per RELR-eligible relocation,
pack relocations into bitmap entries using the full RELR encoding:
- A bitmap entry covers 63 slots (bits 1-63), each = base+bit*8
- Gaps within the window encode as 0 bits
- Only out-of-window or unaligned relocations force a new run

Reset RELR run state between input sections to match the per-section
tracking done in the layout phase, ensuring layout and writer always
agree on the number of entries needed.

Issue wild-linker#1853
@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch 3 times, most recently from d435269 to 571c8b1 Compare July 4, 2026 20:19
Adds a test with 4 consecutive static pointers that verifies bitmap
packing produces 2 RELR entries (1 address + 1 bitmap) instead of
4 individual address entries. Compares output against lld.

Issue wild-linker#1853
@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch from 571c8b1 to e1611f6 Compare July 4, 2026 20:30
@deepakshirkem deepakshirkem marked this pull request as ready for review July 4, 2026 20:34

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test passes without the rest of your change. I think to actually test that RELR bitmaps are being used, you'd need to extend the test assertion system and add some assertions about the number of relocations performed via RELR bitmap. Given the slight differences in the number of bitmap-based relocations performed by lld vs wild for this test, you'd probably need to specify a range or a minimum rather than an exact number

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You were right. Added an ExpectSectionContains directive to verify that the expected byte sequence exists in the Wild output. The test now checks for the bitmap value 0x07 in .relr.dyn, which is mathematically guaranteed for 3 consecutive 8-byte-aligned entries. I verified that the test fails on upstream/main and passes with this fix.

Do you have any other approach that would be more robust for verifying that the .relr.dyn bitmap encoding is happening correctly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd probably be inclined to have the test framework parse the .relr.dyn and count the total number of relr relocations. That should be the same for all linkers, but if it's not, then you make the assertion be the minimum number of relr relocations. The ExpectSection directive could be extended to accept properties, similar to how ExpectSym does.

//#RelrCount:4
//#ExpectSection:.relr.dyn max_size=2

It's good, where possible to have assertions apply to all linkers, not just wild. With these two assertions, it should be possible to configure them such that they pass for both wild and lld - possibly max_size for the section might need to be 3 if wild has one more non-bitmap relocation.

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch 5 times, most recently from f43ba46 to 131b2b1 Compare July 6, 2026 05:19
Comment thread wild/tests/integration_tests.rs Outdated
//! ExpectSection:{section_name} Checks that the specified section exists in the output binary.
//! ExpectSection:{section_name} [properties] Checks that the specified section exists in the
//! output binary. Optional properties:
//! max_entries=N: Asserts the section has at most N entries (entry size assumed to be 8 bytes).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using an entity count seems like a good idea. Rather than assuming 8 byte entities though, how about using the entity size property on the section? It looks like we're setting it to 8 for .relr.dyn. If it's set to 0, then we should probably treat it as 1.

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch from 131b2b1 to cd55737 Compare July 6, 2026 05:46
// GNU ld ignores `-z pack-relative-relocs` on RISC-V.
//#ReferenceLinkers:lld
// Verify all 3 constructor pointers are correctly encoded as RELR relocations.
//#RelrCount:3

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@davidlattimore Not sure how to handle arch-specific values. Can you help me here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if a value like this is arch-specific, then the best bet is to have variants of the test for each arch that inherit from the base config.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank-You. It looks like we'd need architecture-specific configs, something like:

//#Config:x86_64:default
//#SkipArch:aarch64,riscv64,loongarch64,ppc64le
//#ExpectSection:.relr.dyn max_entries=2

The only concern is that I can't run the AArch64, RISC-V64, LoongArch64, or PPC64LE tests locally to verify their max_entries values. From the CI failures, I observed the following values: AArch64 = 6, LoongArch64 = 6, RISC-V64 = 7, and PPC64LE = 4.

Do you think it's okay to hardcode these values based on the CI results, or is there a better approach?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine to base the values from what you see in CI. If you'd like to get the other architectures working locally, let me know and I can provide guidance. On Ubuntu, or Ubuntu in a docker container, it should just be a matter of apt-get installing some packages.

BTW, I wouldn't suggest using "SkipArch" for this - use "Arch" instead, that way each test can just specify the one arch that it runs for.

@deepakshirkem deepakshirkem Jul 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you'd like to get the other architectures working locally, let me know and I can provide guidance. On Ubuntu, or Ubuntu in a docker container, it should just be a matter of apt-get installing some packages.

Yes, That would be great.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have a look at docker/ci/ubuntu-cross.Dockerfile. That should show which packages will be needed. There's also instructions in CONTRIBUTING.md, but they're quite possibly incomplete, so the dockerfile is likely what should be referred to. If you run into any trouble, let me know and I'll see what I can figure out. If there is anything that could be better documented, then feel free to send a PR to update the docs.

Ok(())
}

fn verify_expected_sections_with_assertions(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ExpectSection already existed. Is there some old code that needs cleaning up?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right. ExpectSection now has two separate code paths depending on whether properties are present. The old expected_sections is still used for the Wasm handling and program header checks.

I can either merge everything into a single Vec<ExpectedSection> and update all the call sites, or leave it as it is for now and do the cleanup in a follow-up PR. I'm happy to do whichever you think is the better approach.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine to leave it for a subsequent PR if you prefer

@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch from cd55737 to a998960 Compare July 6, 2026 06:56
// consecutive 8-byte-aligned offsets. Without bitmap packing, each would get
// its own RELR address entry. With packing, they share an address entry and
// a bitmap entry.
//#Object:runtime.c

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The first config should be declared as abstract, otherwise get run - with all architectures - #AbstractConfig:default

//#Config:x86_64:default
//#Arch:x86_64
//#ExpectSection:.relr.dyn max_entries=2
//#Config:aarch64:default

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you put a blank line before each #Config directive? Otherwise it's pretty hard to see where they start and end.

// GNU ld ignores `-z pack-relative-relocs` on RISC-V.
//#ReferenceLinkers:lld
// Verify all 3 constructor pointers are correctly encoded as RELR relocations.
//#RelrCount:3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have a look at docker/ci/ubuntu-cross.Dockerfile. That should show which packages will be needed. There's also instructions in CONTRIBUTING.md, but they're quite possibly incomplete, so the dockerfile is likely what should be referred to. If you run into any trouble, let me know and I'll see what I can figure out. If there is anything that could be better documented, then feel free to send a PR to update the docs.

Adds two new test framework directives:
- RelrCount:N — decodes .relr.dyn and checks at least N relocations
  are encoded (counts address entries and bitmap-packed relocations)
- ExpectSection with max_entries property — checks a section has at
  most N entries using the section's sh_entsize (or 1 if 0)

Adds relr-bitmap test using 3 consecutive constructors:
- RelrCount:3 verifies all 3 relocations are correctly encoded
- Arch-specific configs verify bitmap packing reduced entry count:
  x86_64=2, aarch64=6, loongarch64=6, riscv64=7, ppc64le=4

Verified: fails on upstream/main (no bitmap packing), passes with fix.

Issue wild-linker#1853
@deepakshirkem deepakshirkem force-pushed the feat/relr-bitmap-packing branch from a998960 to 136ef5a Compare July 6, 2026 08:06
@davidlattimore davidlattimore merged commit c01a031 into wild-linker:main Jul 6, 2026
22 checks passed
deepakshirkem added a commit to deepakshirkem/wild that referenced this pull request Jul 7, 2026
Previously ExpectSection used two separate fields:
- expected_sections: Vec<String> for plain section existence checks
- expected_sections_with_assertions: Vec<ExpectedSection> for checks
  with properties like max_entries

Merge both into a single expected_sections: Vec<ExpectedSection> field.
Plain ExpectSection directives now create an ExpectedSection with
default (empty) assertions. This removes the split code path and makes
the implementation cleaner.

Also:
- Rename verify_expected_sections_with_assertions to
  verify_section_max_entries to better reflect its purpose
- Skip redundant section_by_name calls for entries without max_entries
- Explicitly reject section property assertions for Wasm tests

Follow-up cleanup from wild-linker#2179.
deepakshirkem added a commit to deepakshirkem/wild that referenced this pull request Jul 7, 2026
Previously ExpectSection used two separate fields:
- expected_sections: Vec<String> for plain section existence checks
- expected_sections_with_assertions: Vec<ExpectedSection> for checks
  with properties like max_entries

Merge both into a single expected_sections: Vec<ExpectedSection> field.
Plain ExpectSection directives now create an ExpectedSection with
default (empty) assertions. This removes the split code path and makes
the implementation cleaner.

Also:
- Rename verify_expected_sections_with_assertions to
  verify_section_max_entries to better reflect its purpose
- Skip redundant section_by_name calls for entries without max_entries
- Explicitly reject section property assertions for Wasm tests

Follow-up cleanup from wild-linker#2179.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants