Skip to content
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.

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

Avoid unaligned access to Shape_Type->width #982

Open
wants to merge 1 commit into
base: vanilla
Choose a base branch
from

Conversation

th-otto
Copy link
Contributor

@th-otto th-otto commented Mar 24, 2024

No description provided.

@@ -242,7 +241,9 @@ int Get_Shape_Width(void const* shape)
{
Shape_Type* shp = (Shape_Type*)shape;

return le16toh(shp->Width);
uint16_t width;
memcpy(&width, &shp->Width, sizeof(width));
Copy link

Choose a reason for hiding this comment

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

Perhaps create a template function ReadUnaligned<uint16_t> to help communicate the intent.

@giulianobelinassi
Copy link
Collaborator

is this really necessary? DSi port has the same issue (unaligned access) but I did not need to patch this function in order to get things to work. What exactly does this fix?

@xezon
Copy link

xezon commented May 21, 2024

Unaligned access is undefined behaviour and it will crash on some hardware.

@th-otto
Copy link
Contributor Author

th-otto commented May 21, 2024

You might be right, Shape_Type is declared as packed(1), and the compiler should already access the Width member with byte accesses on architectures that need this. OTOH, it should not hurt, as the memcpy of 2 bytes is optimized anyway.

Simple test:
both

uint16_t test(Shape_Type *t)
{
	return t->Width;
}

and

uint16_t test(Shape_Type *t)
{
	uint16_t w;
	
	memcpy(&w, &t->Width, sizeof(w));
	return w;
}

produce the same code on x86.

@giulianobelinassi
Copy link
Collaborator

You might be right, Shape_Type is declared as packed(1), and the compiler should already access the Width member with byte accesses on architectures that need this. OTOH, it should not hurt, as the memcpy of 2 bytes is optimized anyway.

Simple test: both

uint16_t test(Shape_Type *t)
{
	return t->Width;
}

and

uint16_t test(Shape_Type *t)
{
	uint16_t w;
	
	memcpy(&w, &t->Width, sizeof(w));
	return w;
}

produce the same code on x86.

I Will check this on ARM9 when I have the opportunity. Sorry but the compiler always remove those memcpys on x86_64, so this arch is not ideal for this kind of test

@xezon
Copy link

xezon commented May 21, 2024

On ARM this may error as highlighted by this article:

https://www.alfonsobeato.net/arm/how-to-access-safely-unaligned-data/

@giulianobelinassi
Copy link
Collaborator

This indeed inserts a memcpy call:
https://godbolt.org/z/77W3o5M3P

Although this won't error out since memcpy is guaranteed to read the bytes in the correct order if the pointers do not alias, it still keeps the memcpy call.

The solution presented by you using templates seems to be the best approach, tho. Perhaps we should refactor the code to use this approach instead.

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.

3 participants