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

UBFix: SetMonData #1964

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

UBFix: SetMonData #1964

wants to merge 37 commits into from

Conversation

AreaZeroArven
Copy link

@AreaZeroArven AreaZeroArven commented Dec 2, 2023

SetMonData is cursed.

It is also UB, or implementation defined at least.

Because it works by casting to a u8 and then accessing it like an array, which works, but the proper way would be to have it be a void and treat it based on the type already.

As such, I wrote a few macros so that all that needs to happen is that the right pointer needs to be passed.

As such, I did UBFixes where there are workarounds such as assigning a u16 to a u8 array.

Thank you!

Copy link
Collaborator

@mrgriffin mrgriffin left a comment

Choose a reason for hiding this comment

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

What aboutSetCurrentBoxMonData, surely it needs the same treatment? And also arguably SetBoxMonDataAt for consistency.
EDIT: Actually, I don't think it does, it seems to always be passed a u16 *.

Two alternative changes to avoid violating strict aliasing rules that could be local to SetMonData:

  1. Use memcpy.
  2. Use unsigned char *.

I don't actually think you've fixed all the implementation-defined behavior, because you're still relying on the values being little endian. I'm not actually sure what UB/implementation-defined behavior you have fixed, could you explain it to me?
EDIT 2: Actually this probably does address the endianness issues--I suspect the data in the battle controllers was explicitly written as little-endian, and everywhere else we're reading through a pointer to the correct type.

That said, it's nice that the u8 arrays have been turned into u16s... Although it's very unfortunate that the compiler won't give you a warning if any new code erroneously passes a u8 array to SetMonData (that now takes a u16 *) because that's something that a user could easily have done via copy and paste.

include/battle_controllers.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@mrgriffin mrgriffin left a comment

Choose a reason for hiding this comment

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

Because my review was requested:

I think this change is correct based on inspection alone, although there was that SetMonData32 bug, so it's probably not tested?
EDIT: Obviously not quite correct, see the review comment below. (Forgot to come back and edit this sentence before submitting)

I don't love the names of SetMonData16/SetMonData32. If I was going to design it myself, I'd maybe do something like:

-        SetMonData16(&gEnemyParty[monId], MON_DATA_SPECIES, 
&gBattleBufferA[gActiveBattler][3]);
+        u16 data16 = T1_READ_16(&gBattleBufferA[gActiveBattler][3]);
+        SetMonData(&gEnemyParty[monId], MON_DATA_SPECIES, &data16);

Although this is definitely a bit of a trade-off because you wouldn't be able to declare variables at these places in the code.

I've written one review comment that I think represents a bug that needs fixing, besides that this review could be considered an "Approve", but I would caution the person who accepts + merges this PR to give it another scan to ensure there's no other incorrect data types being used.

src/field_specials.c Outdated Show resolved Hide resolved
@AreaZeroArven
Copy link
Author

Because my review was requested:

I think this change is correct based on inspection alone, although there was that SetMonData32 bug, so it's probably not tested? EDIT: Obviously not quite correct, see the review comment below. (Forgot to come back and edit this sentence before submitting)

I don't love the names of SetMonData16/SetMonData32. If I was going to design it myself, I'd maybe do something like:

-        SetMonData16(&gEnemyParty[monId], MON_DATA_SPECIES, 
&gBattleBufferA[gActiveBattler][3]);
+        u16 data16 = T1_READ_16(&gBattleBufferA[gActiveBattler][3]);
+        SetMonData(&gEnemyParty[monId], MON_DATA_SPECIES, &data16);

Although this is definitely a bit of a trade-off because you wouldn't be able to declare variables at these places in the code.

I've written one review comment that I think represents a bug that needs fixing, besides that this review could be considered an "Approve", but I would caution the person who accepts + merges this PR to give it another scan to ensure there's no other incorrect data types being used.

You are absolutely correct. Let me redefine these macros to use this macro. Thank you!

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