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

HW/DSPHLE: Fix struct aliasing undefined behavior in AX ucode #8525

Merged
merged 2 commits into from
Dec 24, 2019

Conversation

Techjar
Copy link
Contributor

@Techjar Techjar commented Dec 19, 2019

Fixes no audio when compiled on VS2019 issue in Old_AXWii games (Wii Sports, Mario Party 8, WarioWare: Smooth Moves, probably a few others). There are more undefined behaviors in AXWii code, I will address those soon.

Fixes https://bugs.dolphin-emu.org/issues/11781

@Techjar
Copy link
Contributor Author

Techjar commented Dec 19, 2019

The buildbot build now has audio in Mario Party 8, so this seems to fix the issue.

@Techjar Techjar changed the title [WIP] HW/DSPHLE: Fix struct aliasing undefined behavior [WIP] HW/DSPHLE: Fix struct aliasing undefined behavior in AX ucode Dec 19, 2019
@shuffle2
Copy link
Contributor

Could a warning or some compiler flag have caught this?

@Techjar
Copy link
Contributor Author

Techjar commented Dec 19, 2019

As far as I know compilers don't have any warnings regarding strict aliasing.

// Similar to BitCastPtr, but specifically for aliasing structs to arrays.
template <typename ArrayType, typename T,
typename Container =
std::array<ArrayType, (sizeof(T) + sizeof(ArrayType) - 1) / sizeof(ArrayType)>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really a case for supporting sizeof(T) % sizeof(ArrayType) != 0 ? seems like it could cause surprising bugs in the future.

Copy link
Contributor Author

@Techjar Techjar Dec 19, 2019

Choose a reason for hiding this comment

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

Yeah I don't think there's any reasonable circumstances under which you'd use it like that. Should we just enforce alignment with ArrayType using an assert?

@booto suggested doing it that way, but thinking about it maybe it's better to just disallow that use case.

@Techjar Techjar force-pushed the axwii-ub branch 3 times, most recently from c15b9c9 to 3fe8a36 Compare December 20, 2019 12:05
Source/Core/Core/HW/DSPHLE/UCodes/AXWii.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/DSPHLE/UCodes/AXWiiOld.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DSPHLE/UCodes/AXWiiOld.cpp Outdated Show resolved Hide resolved
{
u16 cmd = m_cmdlist[curr_idx++];

switch (cmd)

Choose a reason for hiding this comment

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

This whole switch statement looks quite duplicated, is there a way the common parts could be abstracted into a template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. All the enum constants have an offset and there's some other subtle differences.

Choose a reason for hiding this comment

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

(I know this discussion was ended on IRC yesterday, leaving it here just in case)

https://godbolt.org/z/p-3Dvf

@Techjar Techjar force-pushed the axwii-ub branch 3 times, most recently from 0b03e07 to 25c6110 Compare December 21, 2019 11:01
@Techjar
Copy link
Contributor Author

Techjar commented Dec 21, 2019

I have reverted this PR to the original patch and will do the refactor in a separate PR.

@Techjar Techjar changed the title [WIP] HW/DSPHLE: Fix struct aliasing undefined behavior in AX ucode HW/DSPHLE: Fix struct aliasing undefined behavior in AX ucode Dec 21, 2019
@delroth
Copy link
Member

delroth commented Dec 21, 2019

LGTM.

This fixes Old AX Wii games having no audio when compiled under VS2019.
This also includes some minor code cleanup and moving a function to
avoid duplication.
@delroth delroth merged commit 0341288 into dolphin-emu:master Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants