Skip to content

Conversation

silesmo
Copy link
Member

@silesmo silesmo commented Mar 15, 2025

Fixes a bug where headers returned from a C# component wouldn't be properly set. Adds a test for it and some cleanup.

@silesmo silesmo requested review from jsturtevant and yowl March 15, 2025 14:44
Copy link
Collaborator

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

Is the fix here around the alignment? How was the issue presenting?

"
void* {address};
if (({size} * {list}.Count) < 1024) {{
var {ret_area} = stackalloc {element_type}[({array_size}*{list}.Count)+1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this moved the logic to add the additional 1 to the dotnet_aligned_array function, we should update

var {ret_area} = stackalloc {element_type}[{array_size}+1];
as well.

void* {address};
if (({size} * {list}.Count) < 1024) {{
var {ret_area} = stackalloc {element_type}[({array_size}*{list}.Count)+1];
{address} = (void*)(((int){ret_area}) + ({align} - 1) & -{align});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this line is still required or in some cases this won't align properly since stackalloc won't align on the correct memory boundary (dotnet/csharplang#1799). Unless something else is subtly addressing this?

Copy link
Collaborator

@pavelsavara pavelsavara Mar 18, 2025

Choose a reason for hiding this comment

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

Refactor void* AlignStackPtr(void* stackAddress, uint alignment) helper and use it everywhere.

);
}
results.push(format!("(nint){ptr}"));
results.push(format!("(int){ptr}"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because it generates

BitConverter.TryWriteBytes(new Span<byte>((void*)(basePtr + 8), 4), (nint)listPtr);

And that uses TryWriteBytes(Span<Byte>, UInt64) which is wrong. There is no TryWriteBytes overload for nint.
And then it returns false but we ignore it.
Let's stop using TryWriteBytes.

Instead it could be just

new Span<nint>((void*)(basePtr + 8), 1)[0] = listPtr;

string-roundtrip: func(a: string) -> string;

wasi-http-headers-roundtrip: func(a: list<tuple<string, list<u8>>>) -> list<tuple<string, list<u8>>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

will need to add this to https://github.com/bytecodealliance/wit-bindgen/tree/main/tests/runtime-new/lists since this test has moved to the new framework #1221

@ignatz
Copy link

ignatz commented Sep 18, 2025

I ran into this issue as well. Are there any plans to land this fix or similar? Thanks 🙏

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

Labels

None yet

4 participants