[mono] Enable -Werror=shorten-64-to-32 #70062

lambdageek · 2022-06-01T02:24:49Z

See https://clang.llvm.org/docs/DiagnosticsReference.html#wshorten-64-to-32

Follow-up to #69236

Related to #70026

Follow-up to dotnet#69236 Related to dotnet#70026

lambdageek · 2022-06-01T02:25:09Z

@AaronRobinsonMSFT @lateralusX take a look, please

lambdageek · 2022-06-01T02:26:43Z

This was enough for linux-x64. I think probably arm64 will have some issues too - there's a lot of bookkeeping around the ALIGN_TO macro which is used all over the platform-specific code

It's only in clang

lateralusX · 2022-06-01T06:50:06Z

This was enough for linux-x64. I think probably arm64 will have some issues too - there's a lot of bookkeeping around the ALIGN_TO macro which is used all over the platform-specific code

Jupp, previous PR only did x86/x64 on code that build on Windows runtime build, so if you do this in Clang you will get more from none Windows source files and if you build cross compilers, none x86/x64 source files.

lateralusX · 2022-06-01T06:57:01Z

src/mono/mono/arch/arm64/arm64-codegen.h

@@ -170,7 +170,7 @@ arm_is_bl_disp (void *code, void *target)
 static G_GNUC_UNUSED inline unsigned int
 arm_get_disp (void *p, void *target)
 {
-	unsigned int disp = ((char*)target - (char*)p) / 4;
+	unsigned int disp = (unsigned int) ((char*)target - (char*)p) / 4;


GPTRDIFF_TO_UINT? Several occurrences through the file.

lateralusX · 2022-06-01T07:05:41Z

src/mono/mono/eglib/glib.h

@@ -199,1535 +199,1542 @@ typedef guint16 gunichar2;
 #endif
 typedef guint32 gunichar;

-/*


That is a big diff :), mainly reshuffling code?

lateralusX · 2022-06-01T07:11:03Z

src/mono/mono/eventpipe/ep-rt-mono.c

@@ -2236,7 +2236,7 @@ ep_rt_mono_file_write (

 	do {
 		MONO_ENTER_GC_SAFE;
-		ret = write (fd, buffer, numbytes);
+		ret = (uint32_t) write (fd, buffer, numbytes);


GSSIZE_TO_UINT32 ?

lateralusX · 2022-06-01T07:16:09Z

src/mono/mono/metadata/exception.c

@@ -1145,7 +1145,7 @@ mono_exception_handle_get_native_backtrace (MonoExceptionHandle exc)
 		MONO_HANDLE_ARRAY_GETVAL (ip, arr, gpointer, i);
 		MonoJitInfo *ji = mono_jit_info_table_find_internal (ip, TRUE, FALSE);
 		if (ji) {
-			char *msg = mono_debug_print_stack_frame (mono_jit_info_get_method (ji), (char*)ip - (char*)ji->code_start, NULL);
+			char *msg = mono_debug_print_stack_frame (mono_jit_info_get_method (ji), (uint32_t) ((char*)ip - (char*)ji->code_start), NULL);


GPTRDIFF_TO_UINT32?

src/mono/mono/metadata/icall-eventpipe.c

src/mono/mono/metadata/memory-manager.c

lateralusX · 2022-06-01T08:31:27Z

src/mono/mono/metadata/reflection.c

@@ -1539,19 +1539,19 @@ assembly_name_to_aname (MonoAssemblyName *assembly, char *p)
 			p++;
 			while (*p && g_ascii_isspace (*p))
 				p++;
-			assembly->major = strtoul (p, &s, 10);
+			assembly->major = (int32_t) strtoul (p, &s, 10);


Maybe add a couple of GLONG_TO_XXX, GULONG_TO_XXX and use them through the PR?

lateralusX · 2022-06-01T08:34:01Z

src/mono/mono/metadata/w32handle.c

@@ -735,7 +735,7 @@ mono_w32handle_wait_one (gpointer handle, guint32 timeout, gboolean alertable)
 				goto done;
 			}

-			waited = mono_w32handle_timedwait_signal_handle (handle_data, timeout - elapsed, FALSE, alertable ? &alerted : NULL);
+			waited = mono_w32handle_timedwait_signal_handle (handle_data, timeout - (guint32)elapsed, FALSE, alertable ? &alerted : NULL);


GINT64_TO_UINT32 ?

this is on a branch where elapsed <= timeout so there's no need to check

lateralusX · 2022-06-01T08:34:39Z

src/mono/mono/mini/aot-runtime.c

@@ -5656,7 +5657,7 @@ get_new_trampoline_from_page (int tramp_type)
 		 */
 		if (tramp_type != MONO_AOT_TRAMP_SPECIFIC) {
 			/* Register the rest of the page as a single trampoline */
-			sp_info = mono_tramp_info_create (NULL, code, page->trampolines_end - code, NULL, NULL);
+			sp_info = mono_tramp_info_create (NULL, code, (guint32)(page->trampolines_end - code), NULL, NULL);


GPTRDIFF_TO_UINT32 ?

lateralusX · 2022-06-01T08:35:18Z

src/mono/mono/mini/cfgdump.c

@@ -152,7 +152,7 @@ add_pool_entry (MonoCompile *cfg, ConstantPoolEntry *entry)

 			write_string (cfg, mono_inst_name (insn->opcode));
 			GString *insndesc = mono_print_ins_index_strbuf (-1, insn);
-			const int len = g_strnlen (insndesc->str, 0x2000);
+			const int len = (int) g_strnlen (insndesc->str, 0x2000);


GSIZE_TO_INT ?

this call to g_strnlen has a constant upper bound, so it is manifestly an int

lateralusX · 2022-06-01T08:37:46Z

src/mono/mono/mini/cfgdump.c

@@ -175,7 +175,7 @@ add_pool_entry (MonoCompile *cfg, ConstantPoolEntry *entry)
 			write_short (cfg, NUM_SUCCESSOR);
 			for (int i = 0; i < NUM_SUCCESSOR; i++) {
 				char *str = g_strdup ("successor1");
-				str[9] = '0' + i;
+				str[9] = '0' + (char)i;


Maybe change type of counter or use GINT_TO_CHAR.

i is in the range 0 to 5

lateralusX

Went through part of it and made some comments.

I see that you sometimes uses straight casts and sometimes the GXX_TO_YY, I made some comments around that, I understand that some times there are unique types not covered by macros, different on different platforms etc, but any reasons not to use them for other cases where we have macros (or could easily extend with some additional once)?

Regarding the ALIGN_TO, I believe the type out of it is gssize, not sure but should we use macros around that as well, like GSSIZE_TO_INT (ALIGN_TO (x,8)); if needed or if its very common to use we could have limited set of special macros for the calls and hide needed casting inside the macro?

Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>

lambdageek · 2022-06-01T13:40:37Z

but any reasons not to use them for other cases where we have macros (or could easily extend with some additional once)?

I don't see a lot of value in doing a checked cast in a case where it is evident that the value is in range. (famous last words in C, but nonetheless...)

lateralusX · 2022-06-02T07:14:35Z

but any reasons not to use them for other cases where we have macros (or could easily extend with some additional once)?

I don't see a lot of value in doing a checked cast in a case where it is evident that the value is in range. (famous last words in C, but nonetheless...)

I guess this could be argued both ways, using the macros will end up in the exact same cast, but it will add potential validation logic that values fall within range. If we get warnings from compiler around possible truncation, there is already a potential issue in the code, and if we validate it manually to be a "false positive", you could of course do a straight cast, but then you open up the possibility introducing issues in the future on that code path when maintaining this code and potentially change the assumptions.

In my previous PR I mainly use the following heuristics and reasoning:

Primarily mitigate data truncation issues by fixing root cause of issue, and start using compatible types, if that change cause to much ripple effects into other functions used in a lot of places, revert back to using cast macros to clearly show the intent around code paths detected by compiler as well as potentially protect against future change of value ranges (even in cases where there where a valid range could be detected in code). Using the macro also open up for more potentially validation logic in the future, like more strict type checks in C++ triggering more compiler errors when types are changed but cast is kept (something direct casts will hide). Types seldom used through the code base, platforms specific types etc. can be resolved with direct cast but should be rather infrequent in comparison with others.

So based on that I personally believed that adding a couple of additional characters GSIZE_TO_INT (x) instead of (int) (x) will give you a lot of value and it will compile down to the exact same cast in the end, so no overhead on regular builds.

lambdageek · 2022-06-02T19:04:06Z

So based on that I personally believed that adding a couple of additional characters GSIZE_TO_INT (x) instead of (int) (x) will give you a lot of value and it will compile down to the exact same cast in the end, so no overhead on regular builds.

Ok, I'll stick to using the macros.

Just to set expectations, this is a relatively low-priority PR for me, so I'm not expecting to make a ton of progress in the near future. Just when I have spare cycles.

ghost · 2022-07-02T23:02:24Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

AaronRobinsonMSFT and others added 5 commits May 31, 2022 09:34

Reenable 4244 warning for Mono.

6c29b7c

Enable compiler errors for gcc/clang.

2e6f9d1

Fix issues enabling -Werror=implicit-int-conversion

adcceba

[mono] use -Werror=shorten-64-to-32 and -Werror=implicit-int-conversion

d8e76bb

Follow-up to dotnet#69236 Related to dotnet#70026

fix all -Wshorten-64-to-32 errors on x64

ae39210

lambdageek requested review from vargaz, BrzVlad, naricc, SamMonoRT, lateralusX, thaystg and marek-safar as code owners June 1, 2022 02:24

ghost assigned lambdageek Jun 1, 2022

dotnet-issue-labeler bot added the area-VM-meta-mono label Jun 1, 2022

lambdageek added 2 commits May 31, 2022 22:50

Turn off warnings-as-errors for zlib

4932b06

Don't set -Werror=implicit-int-conversion with GCC

7a00c7b

It's only in clang

lambdageek force-pushed the werror-shorten-implicit-int branch from 9d09b5b to d4037d1 Compare June 1, 2022 03:40

lambdageek added 2 commits May 31, 2022 23:52

fix osx shorten-64-to-32

00715cb

fix arm64 shorten-64-to-32

5e0c071

lambdageek force-pushed the werror-shorten-implicit-int branch from d4037d1 to 5e0c071 Compare June 1, 2022 03:52

lambdageek added 6 commits June 1, 2022 00:26

llvm fix -Wshorten-64-to-32

5862e6a

linux-arm64 fix -Wshorten-64-to-32

7d3a1d9

osx fixup

f7649d7

turn off warnings for mono mscordbi and dbgshim

93eabbd

fix osx-x64

c6050b8

fix tvos

8bfb629

lambdageek force-pushed the werror-shorten-implicit-int branch from cfeee8c to 8bfb629 Compare June 1, 2022 05:27

lateralusX reviewed Jun 1, 2022

View reviewed changes

src/mono/mono/metadata/icall-eventpipe.c Outdated Show resolved Hide resolved

lateralusX reviewed Jun 1, 2022

View reviewed changes

src/mono/mono/metadata/icall-eventpipe.c Outdated Show resolved Hide resolved

lateralusX reviewed Jun 1, 2022

View reviewed changes

src/mono/mono/metadata/memory-manager.c Show resolved Hide resolved

lateralusX reviewed Jun 1, 2022

View reviewed changes

lambdageek and others added 2 commits June 1, 2022 09:31

Update src/mono/mono/metadata/icall-eventpipe.c

f77b9b2

Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>

Update src/mono/mono/metadata/icall-eventpipe.c

24f067e

Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>

runfoapp bot mentioned this pull request Jun 1, 2022

System.Diagnostics.Tests.InstanceDataTests.InstanceDataCollectionCollection_CopyTo failing in CI #68291

Closed

lateralusX mentioned this pull request Jun 2, 2022

Reenable C4244 in repo #70026

Merged

lambdageek marked this pull request as draft June 2, 2022 19:04

ghost closed this Jul 2, 2022

ghost locked as resolved and limited conversation to collaborators Aug 2, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Enable -Werror=shorten-64-to-32 #70062

[mono] Enable -Werror=shorten-64-to-32 #70062

lambdageek commented Jun 1, 2022

lambdageek commented Jun 1, 2022

lambdageek commented Jun 1, 2022

lateralusX commented Jun 1, 2022 •

edited

Loading

lateralusX Jun 1, 2022 •

edited

Loading

lateralusX Jun 1, 2022

lateralusX Jun 1, 2022

lateralusX Jun 1, 2022

lateralusX Jun 1, 2022

lateralusX Jun 1, 2022

lambdageek Jun 1, 2022

lateralusX Jun 1, 2022

lateralusX Jun 1, 2022

lambdageek Jun 1, 2022

lateralusX Jun 1, 2022

lambdageek Jun 1, 2022

lateralusX left a comment •

edited

Loading

lambdageek commented Jun 1, 2022

lateralusX commented Jun 2, 2022 •

edited

Loading

lambdageek commented Jun 2, 2022

ghost commented Jul 2, 2022

[mono] Enable -Werror=shorten-64-to-32 #70062

[mono] Enable -Werror=shorten-64-to-32 #70062

Conversation

lambdageek commented Jun 1, 2022

lambdageek commented Jun 1, 2022

lambdageek commented Jun 1, 2022

lateralusX commented Jun 1, 2022 • edited Loading

lateralusX Jun 1, 2022 • edited Loading

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

lateralusX left a comment • edited Loading

Choose a reason for hiding this comment

lambdageek commented Jun 1, 2022

lateralusX commented Jun 2, 2022 • edited Loading

lambdageek commented Jun 2, 2022

ghost commented Jul 2, 2022

lateralusX commented Jun 1, 2022 •

edited

Loading

lateralusX Jun 1, 2022 •

edited

Loading

lateralusX left a comment •

edited

Loading

lateralusX commented Jun 2, 2022 •

edited

Loading