[Mono] Fix 4244 warnings. #69236
[Mono] Fix 4244 warnings. #69236
Conversation
lateralusX
changed the title
lateralusX
force-pushed
the
lateralusX/fix-inital-4244-warnings
branch
from
d15522a
to
3201cca
Compare
lateralusX
requested review from
lambdageek,
vargaz,
BrzVlad,
naricc,
SamMonoRT,
thaystg and
marek-safar
as code owners
| @@ -1316,4 +1310,424 @@ gint | |||
| g_clock_nanosleep (clockid_t clockid, gint flags, const struct timespec *request, struct timespec *remain); | |||
| #endif | |||
|
|
|||
| //#define ENABLE_CHECKED_CASTS | |||
| #ifdef ENABLE_CHECKED_CASTS | |||
There was a problem hiding this comment.
@steveisok should we have some checked cast CI runs?
There was a problem hiding this comment.
I think we probably need to work that through locally first, maybe after we are done with the warnings? When enabled (I locally implemented asserts in the inline functions to validate underflow/overflow), we hit underflow/overflows that appears to be intentional in some cases, so if we would like to assert, we would probably need to do local test run and replace use of macro with direct cast or have a variant of the macro not enforcing checks where the intention is to underflow/overflow (and maybe do explicit comment that it's the case) or investigate if these are actual real bugs that needs to be addressed.
| @@ -182,19 +182,19 @@ static gboolean generate_code (TransformData *td, MonoMethod *method, MonoMethod | |||
| } while (0) | |||
|
|
|||
| static InterpInst* | |||
| interp_new_ins (TransformData *td, guint16 opcode, int len) | |||
| interp_new_ins (TransformData *td, int opcode, int len) | |||
There was a problem hiding this comment.
Is this something we want to eventually change back to uint16_t ? but right now it would lead to major refactoring?
what if we make typedef int opcode_t and use it here. so that later we can find all the ints more easily?
There was a problem hiding this comment.
Majority of runtime uses int for opcodes until it hits internal structs that handle it as guint16, I had an ambition to change this, but the number of changes exploded, so instead I keep things as int and cast to opcode using GINT_TO_OPCODE macro when we need to convert it to guint16. This change, using int for opcodes in the static methods fixes around 40 warnings and the majority of call sites already have it as int opcode type. So for now I believe we could keep it and its only internal static methods.
|
LGTM, but maybe let's hold off on the marshaling changes until Nathan's PR is in? |
aligning with SDL requirements (dotnet#66154). There will be additional warnings in the cross compilers that needs to be fixed separately, but this should fix the major bulk of 4244 warnings. All changes done in this PR are mitigating compiler issued data type truncation 4244 warnings: 'conversion' conversion from 'type1' to 'type2', possible loss of data and PR will align to the same type as the compiler issued in the warning. PR introduce a new set of macros to cast between types following patter of existing GPOINTER_TO_INT, GPOINTER_TO_UINT, GTYPE1_TO_TYPE2. The idea of use these macros is that it will be clear where casts are done, and we will have ability to intercept and add validation logic in specialized builds to catch truncation errors. The PR also introduce the needed inline functions under ENABLE_CHECKED_CASTS that could be used for extended validation when using the macros. Failure actions in these inline functions are not currently fully implemented, since that will be different from case to case, but can be added when needed since all infrastructure is prepared. If it is possible to change types used in code to mitigate the warnings, that will be the initial strategy used by this PR, if that however would cause ripple effects through the source code (unfortunately quite common), we will fallback using the GTYPE1_TO_TYPE2 macros to clearly show the intent, keep trackability of the change as well as adding ability to do additional validation of casts in the future.
lateralusX
force-pushed
the
lateralusX/fix-inital-4244-warnings
branch
from
3201cca
to
7cff7a5
Compare
|
My marshaling change (#69208) has merged, so I believe this is good to go. |
Follow-up to dotnet#69236 Related to dotnet#70026
ghost
locked as resolved and limited conversation to collaborators
Fixes all 4244 warnings on Mono runtime x86/x64 Windows build, aligning with SDL requirements (#66154). There will be additional warnings in the cross compilers that needs to be fixed separately, but this should fix the major bulk of 4244 warnings.
All changes done in this PR are mitigating compiler issued data type truncation 4244 warnings:
'conversion' conversion from 'type1' to 'type2', possible loss of dataand PR will align to the same type as the compiler issued in the warning.
PR introduce a new set of macros to cast between types following patter of existing GPOINTER_TO_INT, GPOINTER_TO_UINT, GTYPE1_TO_TYPE2. The idea of use these macros is that it will be clear where casts are done, and we will have ability to intercept and add validation logic in specialized builds to catch truncation errors. The PR also introduce the needed inline functions under
ENABLE_CHECKED_CASTSthat could be used for extended validation when using the macros. Failure actions in these inline functions are not currently fully implemented, since that will be different from case to case, but can be added when needed since all infrastructure is prepared.If it is possible to change types used in code to mitigate the warnings, that will be the initial strategy used by this PR, if that however would cause ripple effects through the source code (unfortunately quite common), we will fallback using the GTYPE1_TO_TYPE2 macros to clearly show the intent, keep trackability of the change as well as adding ability to do additional validation of casts in the future.