Skip to content

improve alignment for refc#25525

Open
ringabout wants to merge 1 commit intodevelfrom
pr_meed
Open

improve alignment for refc#25525
ringabout wants to merge 1 commit intodevelfrom
pr_meed

Conversation

@ringabout
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings February 16, 2026 11:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request modifies alignment checks in the reference counting garbage collector (refc) for Nim. The changes split the alignment assertions in rawNewObj and newObjRC1 into two branches based on whether custom alignment exceeds the default memory alignment.

Changes:

  • Modified alignment assertions in rawNewObj to check Cell pointer alignment when alignment equals MemAlign, instead of always checking user data alignment
  • Applied the same change to newObjRC1 for consistency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Araq
Copy link
Member

Araq commented Feb 16, 2026

@ringabout please explain your reasoning behind this change.

@ringabout
Copy link
Member Author

ringabout commented Feb 17, 2026

In my old PR, when alignment is greater than MemAlign, the data address is aligned. But it doesn't handle the case when the alignment is the default MemAlign, i.e 16 bytes. It works on Amd64 because the GC header happens to be 16 bytes, which is not the case for i386 and debugging purposes.

e.g. on i386, MemAlign is also 16 bytes, but is GC header is 8 bytes. With the default alignment mechanism, the data address is not aligned by 16

type
  MyType16 = object
    a {.align(16).}: int

Moreover, we perhaps need to take into consideration the case where the custom alignment is equal to the MemAlign

@Araq
Copy link
Member

Araq commented Feb 17, 2026

Well but if I want 16 byte alignment I should be able to get it, even on a 32bit machine.

@ringabout
Copy link
Member Author

ringabout commented Feb 17, 2026

Yeah, I will enforce alignment allocation for all the custom alignment soon, though I need to examine the ramification more. This PR should fix the cases that does not need aligned allocation

gcAssert((cast[int](cellToUsr(res)) and (alignment-1)) == 0, "newObj: 2")

This is wrong for allocation doesn't need alignment, because only res is aligned by MemAlign, which doesn't mean cellToUsr(res), i.e., the data address, is aligned by MemAlign, due to the existence of GcHeader, especially exported by i386

if alignment > MemAlign:
  gcAssert((cast[int](cellToUsr(res)) and (alignment-1)) == 0, "newObj: 2.1")
else:
  gcAssert((cast[int](res) and (MemAlign-1)) == 0, "newObj: 2.2")

In this PR, I add the else branch to cover unaligned allocation

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