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

Already on GitHub? Sign in to your account

Ensure 16-byte alignment for amd64 coroutine stack #230

Merged
merged 1 commit into from Oct 12, 2012

Conversation

Projects
None yet
3 participants
Contributor

jeremyevans commented Oct 12, 2012

The AMD64 ABI requires that stacks be 16-byte aligned.  stackend is
going to be 16-byte aligned, 128 is a multiple of 16, and
sizeof(uintptr_t) is going to 8 on amd64, so this results in a stack
that is not 16-byte aligned on amd64 (at least for platforms where
this code branch is used, such as OpenBSD).  This results in bus errors
when floating point numbers are converted to strings, since snprintf
calls movaps with a non 16-byte aligned memory location.

With this patch, the iovm correctness tests pass on OpenBSD/amd64.

@jeremyevans jeremyevans Ensure 16-byte alignment for amd64 coroutine stack
The AMD64 ABI requires that stacks be 16-byte aligned.  stackend is
going to be 16-byte aligned, 128 is a multiple of 16, and
sizeof(uintptr_t) is going to 8 on amd64, so this results in a stack
that is not 16-byte aligned on amd64 (at least for platforms where
this code branch is used, such as OpenBSD).  This results in bus errors
when floating point numbers are converted to strings, since snprintf
calls movaps with a non 16-byte aligned memory location.

With this patch, the iovm correctness tests pass on OpenBSD/amd64.
84c9e4f

@stevedekorte stevedekorte added a commit that referenced this pull request Oct 12, 2012

@stevedekorte stevedekorte Merge pull request #230 from jeremyevans/patch-1
Ensure 16-byte alignment for amd64 coroutine stack
91c356f

@stevedekorte stevedekorte merged commit 91c356f into stevedekorte:master Oct 12, 2012

Contributor

jeremytregunna commented on 84c9e4f Oct 12, 2012

The proper way to do alignment is like this:

static inline size_t aligned_size(size_t size, size_t alignment)
{
    assert(alignment <= 0x8000);
    size_t r = size + --alignment + 2;
    return (r + 2 + alignment) & ~alignment;
}

Calling this function assumes that "alignment" is power of 2, and "size" would be the original stackend - sizeof(uintptr_t) - 128.

Contributor

jeremyevans replied Oct 12, 2012

That's certainly looks like a more portable solution, compared to the quick hack I used. I'll be happy as long as the problem get's fixed. Not being familiar with the amd64 ABI or Io's internals, it took me many hours to diagnose the underlying problem causing the bus error, and figure out where the fix should be applied.

Owner

stevedekorte replied Oct 12, 2012

I'll accept whichever working version you guys recommend. Thanks for helping with this.

Contributor

jeremyevans replied Oct 12, 2012

If you plan on having Io run on many platforms with different alignment requirements, then Jeremy Tregunna's code is probably best. If you only care about fixing amd64, then this pull request gets the job done with the least number of changes (2 characters).

Contributor

jeremytregunna replied Oct 12, 2012

I'll test out my code in place, and see if there are any regressions. Should all look ok, then I will send another pull.

Contributor

jeremytregunna replied Oct 12, 2012

Alright tested, and seems to work just fine. It's in #232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment