Enable `-Wint-conversion` warning #1266

AngheloAlf · 2023-06-01T23:02:05Z

Turning this warning on makes CC_CHECK to warn about implicit int/pointer conversions, to avoid it we just need to make a explicit cast.

For Physical/rom addresses I try to us uintptr_t as much as I can (as we mostly do already)
Because of the previous point, I added a uintptr_t cast in the SEGMENT_ROM_START/SEGMENT_ROM_END macros
I also had to make a SEGMENT_ROM_START_OFFSET macro because some places in the codebase do arithmetic with a u8* pointer and doesn't like the address to be casted to uintptr_t before the arithmetic. Same reason for SEGMENT_ROM_SIZE2.

EllipticEllipsis

I like the idea of this in principle but we need to decide what to do with rom addresses and segmented addresses first, I think. I have a few comments on the current state, mostly where one or the other cast seems better.

EllipticEllipsis · 2023-06-05T21:39:29Z

Makefile

@@ -102,8 +102,8 @@ endif

 # Check code syntax with host compiler
 ifneq ($(RUN_CC_CHECK),0)
-  CHECK_WARNINGS := -Wall -Wextra -Wno-format-security -Wno-unknown-pragmas -Wno-unused-parameter -Wno-unused-variable -Wno-missing-braces -Wno-int-conversion -Wno-unused-but-set-variable -Wno-unused-label -Wno-sign-compare -Wno-tautological-compare


I see you also removed -Wno-format-security, what does that imply?

It does nothing currently. https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-security

It warns you for cases when you misuse printf/scanf functions, like passing a float when you have a %u

We could annotate some variadic functions that work similar to printf (with __attribute__ ((format (printf, 2, 3)))) but that's a out of scope for this PR

That's a confusing attribute. Yeah, don't think we need to worry about that.

Forgot to link the docs about it
https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes
I think it could actually be useful in the future

Makefile

include/variables.h

EllipticEllipsis · 2023-06-05T22:25:37Z

src/boot_O2/system_heap.c

-typedef void (*BlockFunc8)(void*, u32, u32, u32, u32, u32, u32, u32, u32);
+typedef void (*BlockFunc)(uintptr_t);
+typedef void (*BlockFunc1)(uintptr_t, u32);
+typedef void (*BlockFunc8)(uintptr_t, u32, u32, u32, u32, u32, u32, u32, u32);


One thing I will note with these functions is this is what happens in OoT N64: https://decomp.me/scratch/0cnK7 , which might affect how we want to do this.

I don't know what I'm supposed to look at

The point is that the casting will surely follow the same pattern, so if there's discrepancy it's probably wrong.

Or maybe they got reworked like a lot of stuff between OoT and MM? 🤷‍♂️

The only reworking I see there is the interrupt handling is removed for MM and OoT GC.

src/boot_O2_g3/idle.c

EllipticEllipsis · 2023-06-05T23:04:26Z

src/audio/lib/heap.c

@@ -1438,7 +1438,7 @@ void AudioHeap_ApplySampleBankCacheInternal(s32 apply, s32 sampleBankId) {

    sampleBankTable = gAudioCtx.sampleBankTable;
    numFonts = gAudioCtx.soundFontTable->numEntries;
-    change.oldAddr = AudioHeap_SearchCaches(SAMPLE_TABLE, CACHE_EITHER, sampleBankId);
+    change.oldAddr = (uintptr_t)AudioHeap_SearchCaches(SAMPLE_TABLE, CACHE_EITHER, sampleBankId);


I defer to @engineer124 on all the audio stuff in here, most of these structs are quite deep.

Admittedly, low level audio does a lot of weird pointer manipulation with offsets and indexing when both transferring audio assets from rom to ram (sequences/soundfonts/samples), and even more weird pointer/offset math when extracting meaningful information from it (especially soundfonts, that's really messy when it comes to pointers).

Given that, I think I'd need to sit down and summarize all the pointer math involved in audio and share that to figure out what kind of types we need. That would take time though, so for now I think I'm fine with just making sure it's consistent i.e. warning free?

src/code/TwoHeadArena.c

EllipticEllipsis · 2023-06-05T23:06:54Z

src/code/TwoHeadArena.c

@@ -32,7 +32,7 @@ void* THA_GetTail(TwoHeadArena* tha) {
 void* THA_AllocHead(TwoHeadArena* tha, size_t size) {
    void* start = tha->head;

-    tha->head = (u8*)tha->head + size;
+    tha->head = (void*)((uintptr_t)tha->head + size);


I don't really see the point of casting out of and back into a pointer here when it's not necessary, (u8*) just seems better for this sort of arithmetic where possible.

Personally I see doing pointer arithmetic as something bad and which should be avoided. That's why we have types to do proper pointer arithmetic

But this is standard pointer arithmetic, not alignment, it's not an unusual operation. Arithmetic is one of the two main properties of pointers, after all.

Casting to uintptr_t to perform pointer arithmetic is way more common in the codebase than using u8*

EllipticEllipsis · 2023-06-05T23:07:10Z

src/code/TwoHeadArena.c

@@ -103,7 +103,7 @@ void* THA_AllocTailAlign(TwoHeadArena* tha, size_t size, uintptr_t mask) {
 * @return Remaining size. A negative number indicates an overflow.
 */
 s32 THA_GetRemaining(TwoHeadArena* tha) {
-    return (s32)((u8*)tha->tail - (u8*)tha->head);
+    return (uintptr_t)tha->tail - (uintptr_t)tha->head;


Actually won't this output an unsigned thing that then gets cast? I think keeping it as u8* is a much better option.

We can't keep this as u8* without casting somewhere, that's the point of -Wint-conversion

So cast to the return type afterwards.

EllipticEllipsis · 2023-06-05T23:08:45Z

src/code/z_kanfont.c

@@ -39,8 +39,7 @@ void Font_LoadOrderedFont(Font* font) {
            loadOffset = 0;
        }

-        DmaMgr_SendRequest0(writeLocation, (uintptr_t)SEGMENT_ROM_START(nes_font_static) + loadOffset,
-                            FONT_CHAR_TEX_SIZE);
+        DmaMgr_SendRequest0(writeLocation, SEGMENT_ROM_START(nes_font_static) + loadOffset, FONT_CHAR_TEX_SIZE);


So does SEGMENT_ROM_START_OFFSET work here or not? Either way I think it confuses the operation here.

Weirdly no, this place requires the cast to happen before the arithmetic and not before. The devs were definitely not consistent with this

hensldm

A couple more nits though overall LGTM. Does seem good to enable this warning.

include/z64.h

src/overlays/actors/ovl_En_Fish2/z_en_fish2.c

Co-authored-by: Derek Hensley <[email protected]>

tom-overton

Admittedly, I don't know if all of Elliptic's concerns were fully addressed, but this has been open for months. I think it's better for us to get this in and fix things later.

engineer124 · 2023-10-31T03:40:11Z

src/overlays/effects/ovl_Effect_Ss_Extra/z_eff_ss_extra.c

-    if ((objectSlot > OBJECT_SLOT_NONE) && (Object_IsLoaded(&play->objectCtx, objectSlot))) {
-        void* segBackup = gSegments[6];
+    if ((objectSlot >= 0) && (Object_IsLoaded(&play->objectCtx, objectSlot))) {


Just a bad merge. Could you bring back OBJECT_SLOT_NONE and remove the extra brackets around Object_IsLoaded

AngheloAlf added 18 commits November 21, 2022 19:49

fixing some warnings

4969e83

more pointer fixing

f103844

Use -Wno-int-conversion on audio files

be427de

more progress

c6f2a0e

fix

4d96323

more pointer fixes

d6ce52b

kinda progress

8c8cad7

Fix remaining casts

3b5f503

Format

8e77c4a

fix merge

7c566eb

Merge remote-tracking branch 'upstream/master' into int-conversion

ab64e54

Merge remote-tracking branch 'upstream/master' into int-conversion

f2e4c5c

fix warnings

f991415

fix parameter

a258564

use SEGMENT_ROM_START_OFFSET a bit more

d6c2dce

format

b086cc7

fix audio warnings

19d6dfa

some more fixes

0657e70

AngheloAlf added Clean up Needs-second-approval Second approval Needs-first-approval First approval labels Jun 1, 2023

AngheloAlf added 3 commits June 2, 2023 08:58

Merge remote-tracking branch 'upstream/master' into int-conversion

443b3c4

fix en_go

26237ce

Merge remote-tracking branch 'upstream/master' into int-conversion

b25b01c

engineer124 mentioned this pull request Jun 5, 2023

sys_math.c Rename #1258

Merged

EllipticEllipsis reviewed Jun 5, 2023

View reviewed changes

AngheloAlf added 4 commits June 5, 2023 21:06

review

3ece9f5

review

ca2f535

whoops, missing cast

f86069f

review

c49ad33

AngheloAlf added 6 commits June 24, 2023 11:49

engo

dd897f4

format

ad5ae53

Merge remote-tracking branch 'upstream/master' into int-conversion

82a2f59

fix

674b6a7

Merge remote-tracking branch 'upstream/master' into int-conversion

b9f983b

format

e88f040

hensldm approved these changes Jul 4, 2023

View reviewed changes

include/z64.h Outdated Show resolved Hide resolved

src/overlays/actors/ovl_En_Fish2/z_en_fish2.c Outdated Show resolved Hide resolved

AngheloAlf and others added 2 commits July 4, 2023 09:48

Update include/z64.h

d51729a

Co-authored-by: Derek Hensley <[email protected]>

TexturePtr

51dede1

hensldm mentioned this pull request Jul 8, 2023

Clang-Format 14 #1321

Merged

AngheloAlf added 4 commits July 8, 2023 14:52

Merge remote-tracking branch 'upstream/master' into int-conversion

87a29db

fix warnings

7812d7e

Merge remote-tracking branch 'upstream/master' into int-conversion

a0047e4

Merge remote-tracking branch 'upstream/master' into int-conversion

d0808f1

engineer124 approved these changes Aug 15, 2023

View reviewed changes

engineer124 removed the Needs-second-approval Second approval label Aug 15, 2023

AngheloAlf added 7 commits September 14, 2023 12:25

Merge remote-tracking branch 'upstream/master' into int-conversion

bb59adc

Merge remote-tracking branch 'upstream/master' into int-conversion

86f5b68

Merge remote-tracking branch 'upstream/main' into int-conversion

7daadeb

fix warnings

e718227

format

6d51251

Merge remote-tracking branch 'upstream/main' into int-conversion

7d6c361

fix

6013f79

tom-overton approved these changes Oct 31, 2023

View reviewed changes

tom-overton added Merge-ready All reviewers satisfied, just waiting for CI and removed Needs-first-approval First approval labels Oct 31, 2023

engineer124 reviewed Oct 31, 2023

View reviewed changes

review

c5d27ee

engineer124 merged commit f276d2b into zeldaret:main Nov 1, 2023
1 check passed

AngheloAlf deleted the int-conversion branch November 1, 2023 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `-Wint-conversion` warning #1266

Enable `-Wint-conversion` warning #1266

AngheloAlf commented Jun 1, 2023

EllipticEllipsis left a comment

EllipticEllipsis Jun 5, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 6, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 5, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 6, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 6, 2023

EllipticEllipsis Jun 5, 2023

engineer124 Jun 7, 2023

EllipticEllipsis Jun 5, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 6, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 5, 2023

AngheloAlf Jun 6, 2023

EllipticEllipsis Jun 6, 2023

EllipticEllipsis Jun 5, 2023

AngheloAlf Jun 6, 2023

hensldm left a comment

tom-overton left a comment

engineer124 Oct 31, 2023

Enable -Wint-conversion warning #1266

Enable -Wint-conversion warning #1266

Conversation

AngheloAlf commented Jun 1, 2023

EllipticEllipsis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hensldm left a comment

Choose a reason for hiding this comment

tom-overton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enable `-Wint-conversion` warning #1266

Enable `-Wint-conversion` warning #1266