[Mono] Fix warnings to enable W4 on Windows. #66710

lateralusX · 2022-03-16T12:55:57Z

Upstream work done in different downstream repros.

Fix selected W4 warnings on Windows as well as suppressing specific W4 warnings to be fixed. Give us option to do Mono runtime build using W4 on Windows and also turn on warnings as errors to prevent regressions on the warnings we enable.

Fix Warning C4146 in Mono runtime build.

Enable "Treat Warnings As Errors" to prevent regressions on the warnings we decided to fix.

dotnet-issue-labeler · 2022-03-16T12:56:03Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

AaronRobinsonMSFT · 2022-03-20T14:25:13Z

@lateralusX Is this helping to contribute to #66154?

AaronRobinsonMSFT · 2022-03-20T14:26:21Z

If so, I would also add the following to help ensure regressions are caught early.

add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/WX>) # treat warnings as errors

lateralusX · 2022-03-21T08:07:05Z

If so, I would also add the following to help ensure regressions are caught early.
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/WX>) # treat warnings as errors

Yes, that was my plan to do in this PR, once I worked it through.

lateralusX · 2022-03-21T08:11:35Z

@lateralusX Is this helping to contribute to #66154?

This PR is mainly upstreaming other work getting W4 building on Mono, so not explicitly target any of those warnings, but yet another stepping stone bringing down warnings altogether on Mono while still bumping to W4.

I think I will be able to take care of:

#pragma warning(disable:4146) // unary minus operator applied to unsigned type, result still unsigned

as part of this PR, so will add some progression on #66154 as well.

vargaz · 2022-03-21T16:03:30Z

src/mono/mono/component/debugger-agent.c

@@ -6433,7 +6429,7 @@ get_types_for_source_file (gpointer key, gpointer value, gpointer user_data)
 			files = get_source_files_for_type (klass);
 			g_hash_table_insert (info->source_files, klass, files);

-			for (int i = 0; i < files->len; ++i) {
+			for (guint i = 0; i < files->len; ++i) {


Can we change GPtrArray->len to an int instead ?

Yes we probably could, did a quick test and it still builds without additional warnings. Any particular reason to do that, except that we might have usage in many places that do int i when iterating it? It will also affect the signature of g_ptr_array_remove_* that currently accept indexes to go from guint to int.

Changing it to int would reduce the amount of changes required and make it easier to write code to iterate on it.

So for the warnings, this PR makes sure all builds as expected, but enabling more warnings might trigger more warnings related to doing unsinged->signed conversion. This PR is already very big, and I mainly did small number of changes like the one above, just to align with original types in files already touched. Could an alternative be to look into that when we enable some additional warnings that will trigger errors due to singed/unsigned mismatch? An alternative would be to revert all of the int i -> source type in this PR and then take care of all in separate PR when enabling more warnings where we also could discuss if we should change type when iterating or at the source. So mainly three options for this PR:

Leave as is, changes done so far will simplify future fixes of warnings, but might need to be change back in a couple of places if we change any of the source types like len -> int.

Revert the int i -> source type done in this PR (unless it breaks some other warnings enabled by this PR).

Change len to int and fix all usage of it as part of this PR, includes changing some of the array API's as well, but as said, this PR is already big enough.

Thoughts?

vargaz · 2022-03-21T16:03:56Z

src/mono/mono/component/hot_reload.c

@@ -529,14 +529,14 @@ hot_reload_close_all (MonoImage *base_image)
 	if (!info)
 		return;
 	for (GList *ptr = info->delta_info; ptr; ptr = ptr->next) {
-		DeltaInfo *info = (DeltaInfo *)ptr->data;
-		if (!info)
+		DeltaInfo *delta_info = (DeltaInfo *)ptr->data;


What was the problem here ?

That change was mainly aligning the last DeltaInfo *info to use same naming as all others in this file, delta_info, the other change in this file fix a local hiding issue, warning C4456: declaration of 'info' hides previous local declaration, that was very common through the code and fixed in this PR.

lateralusX · 2022-04-06T10:05:42Z

Rebased and fixed review feedback.

# Conflicts: # src/mono/mono/component/debugger-agent.c # Conflicts: # src/mono/mono/component/debugger-agent.c

# Conflicts: # src/mono/mono/mini/method-to-ir.c # Conflicts: # src/mono/mono/mini/aot-runtime.c

# Conflicts: # src/mono/mono/component/hot_reload.c

lateralusX · 2022-04-11T08:37:53Z

Moved disable of zlib warnings from source file into cmake file + rebase branch.

ghost assigned lateralusX Mar 16, 2022

lateralusX added os-windows area-Build-mono labels Mar 16, 2022

runfoapp bot mentioned this pull request Mar 16, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

lateralusX force-pushed the lateralusX/fix-warnings-w4 branch 2 times, most recently from 3f6fbb8 to e29a0b8 Compare March 17, 2022 18:19

runfoapp bot mentioned this pull request Mar 18, 2022

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

lateralusX force-pushed the lateralusX/fix-warnings-w4 branch from e026c33 to 49b688f Compare March 21, 2022 15:42

lateralusX changed the title ~~WIP: [Mono] Fix warnings to enable W4 on Windows.~~ [Mono] Fix warnings to enable W4 on Windows. Mar 21, 2022

lateralusX marked this pull request as ready for review March 21, 2022 15:45

lateralusX requested review from lambdageek, vargaz, BrzVlad, naricc, SamMonoRT, thaystg and marek-safar as code owners March 21, 2022 15:45

vargaz reviewed Mar 21, 2022

View reviewed changes

lateralusX force-pushed the lateralusX/fix-warnings-w4 branch 3 times, most recently from 9ce5ecc to c81c2b0 Compare March 23, 2022 15:33

AaronRobinsonMSFT mentioned this pull request Mar 23, 2022

SDL signoff requirements - please enable additional C/C++ compiler warnings #66154

Closed

21 tasks

lateralusX force-pushed the lateralusX/fix-warnings-w4 branch 2 times, most recently from 0e88c45 to 277a366 Compare March 24, 2022 10:04

lambdageek approved these changes Apr 6, 2022

View reviewed changes

lateralusX added 24 commits April 11, 2022 10:29

Fix diagnostics_tracing_objects to build under W4 on Windows.

8f529cb

Fix debugger-objects to build under W4 on Windows.

3ebc4ca

# Conflicts: # src/mono/mono/component/debugger-agent.c # Conflicts: # src/mono/mono/component/debugger-agent.c

Fix monosgen-objects and monosgen-shared to build under W4 on Windows.

fb9f87b

# Conflicts: # src/mono/mono/mini/method-to-ir.c # Conflicts: # src/mono/mono/mini/aot-runtime.c

Fix hot-reload-objects to build under W4 on Windows.

8d254ec

# Conflicts: # src/mono/mono/component/hot_reload.c

Enable warning level W4 on Windows + disable not yet fixed w4 warnings.

f996846

Fix signed/unsigned in for blocks of modified files.

a504e9a

Fix Warning 4146 on Windows.

e51f349

Enable warnings as errors on Windows build to prevent regression.

4d1655e

Fix build error in mscordbi socket.

a5f5219

Fix additional warnings added since rebase.

bb04cb4

Fix Windows x86 warnings.

9e3f55f

Fix additional warnings.

67245c1

Fix build error.

4da5f0c

Fix warning build errors.

592ae95

Switch back from treating all warnings as errors to promote strategy.

62f2553

Fix some more warnings.

926c058

Disable some warnings as errors to flush out last CI failures.

6a2b9d0

Fix warnings on x86/arm.

4b765ec

Fix build error.

a287d25

Re-enable warnings.

4071809

Fix build errors.

00d6b20

32-bit shift assumes wrap around in some scenarios.

c259ec3

Review feedback.

3b635bd

Disable zlib warnings in cmake instead of source files.

0617776

lateralusX force-pushed the lateralusX/fix-warnings-w4 branch from 4a86047 to 0617776 Compare April 11, 2022 08:34

lateralusX merged commit ff4d3d4 into dotnet:main Apr 12, 2022

ghost locked as resolved and limited conversation to collaborators May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mono] Fix warnings to enable W4 on Windows. #66710

[Mono] Fix warnings to enable W4 on Windows. #66710

lateralusX commented Mar 16, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Mar 16, 2022

AaronRobinsonMSFT commented Mar 20, 2022

AaronRobinsonMSFT commented Mar 20, 2022

lateralusX commented Mar 21, 2022

lateralusX commented Mar 21, 2022 •

edited

Loading

vargaz Mar 21, 2022

lateralusX Mar 22, 2022

vargaz Mar 23, 2022

lateralusX Mar 24, 2022

vargaz Mar 21, 2022

lateralusX Mar 22, 2022 •

edited

Loading

lateralusX commented Apr 6, 2022

lateralusX commented Apr 11, 2022

[Mono] Fix warnings to enable W4 on Windows. #66710

[Mono] Fix warnings to enable W4 on Windows. #66710

Conversation

lateralusX commented Mar 16, 2022 • edited Loading

dotnet-issue-labeler bot commented Mar 16, 2022

AaronRobinsonMSFT commented Mar 20, 2022

AaronRobinsonMSFT commented Mar 20, 2022

lateralusX commented Mar 21, 2022

lateralusX commented Mar 21, 2022 • edited Loading

vargaz Mar 21, 2022

Choose a reason for hiding this comment

lateralusX Mar 22, 2022

Choose a reason for hiding this comment

vargaz Mar 23, 2022

Choose a reason for hiding this comment

lateralusX Mar 24, 2022

Choose a reason for hiding this comment

vargaz Mar 21, 2022

Choose a reason for hiding this comment

lateralusX Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

lateralusX commented Apr 6, 2022

lateralusX commented Apr 11, 2022

lateralusX commented Mar 16, 2022 •

edited

Loading

lateralusX commented Mar 21, 2022 •

edited

Loading

lateralusX Mar 22, 2022 •

edited

Loading