Fix warnings reported by MSVC (`-W2`) and clang-cl (`-W4`) by MisterDA · Pull Request #13243 · ocaml/ocaml

MisterDA · 2024-06-18T09:18:25Z

This PR fixes warnings raised by MSVC at level 2, and clang-cl at level 4 (minus unused parameters and sign-compare). It should be rather uneventful. I think it's sane to enable some warnings rather than none at all (the current), but C compilers (and sometimes programmers too) have a tendency of being overzealous when they report issues.

MSVC really likes to warn when an assignment of two different ranges, like float to int, is made without an explicit cast. If the warnings C4244 and C4146 are too annoying, they could also be disabled.

Clang-cl -Wall is equivalent to clang -Weverything, which is really too noisy, and enables some C++ warnings. Levels above 2 for MSVC are also noisy, and some warnings are even checking for C++ things in C. Someone with spare time might revisit this later…

nojb

Thanks! A chunk of the diff is dead code removal and clearly correct. Some of the other changes involve introducing explicit type casts; it would be good to have a second opinion because this can sometimes be tricky. Otherwise, I am happy to approve this PR.

@dustanddreams maybe you can take a look? Thanks!

ghost · 2024-06-19T05:51:04Z

runtime/alloc.c

+#define Enter_gc_preserve_vals(dom_st, wosize) do {                     \
+    CAMLparam0();                                                       \
+    CAMLlocalN(vals_copy, (wosize));                                    \
+    for (mlsize_t j = 0; j < (wosize); j++) vals_copy[j] = vals[j];     \


Why did you had to introduce a new local for the loop counter? Enter_gc_preserve_vals is only used from the Do_alloc_small macro next to it, which declares mlsize_t i.

Is there a warning about the use of i which is neither local to nor an argument of the macro? If so, such a too-pendantic warning ought to be disabled.

caml_alloc_9 defines parameters from a to i:

CAMLexport value caml_alloc_9 (tag_t tag, value a, value b, value c, value d, value e, value f, value g, value h, value i) { Do_alloc_small(9, tag, a, b, c, d, e, f, g, h, i); }

then Do_alloc_small expands to

#define Do_alloc_small(wosize, tag, ...) \ { \ Caml_check_caml_state(); \ value v; \ value vals[wosize] = {__VA_ARGS__}; \ mlsize_t i; \ CAMLassert ((tag) < 256); \ // ...

the local i shadows the i parameter, thus the warning. Also, the Enter_gc_preserve_vals relies on the outer block declaring i, which I don't find particularly nice.
My intent was to kill two birds with one stone: first remove the shadowing warning by renaming the local i to j, then reduce the scope of the for loop iterators.

This was an incursion into MSVC level 4 warnings, so I'll just withdraw this commit.

ghost · 2024-06-19T05:52:59Z

runtime/alloc.c

  Alloc_small(v, wosize, tag, Enter_gc_preserve_vals);  \
-  for (i = 0; i < (wosize); i++) {                      \
-    Field(v, i) = vals[i];                              \
+  for (mlsize_t j = 0; j < (wosize); j++) {             \


Almost the same question here. Why did you have to rename the variable and reduce its scope? Did you get a shadowing warning? If so, wouldn't simply rename from i to something else in both macros would be good enough?

See the answer above. I'll withdraw the commit.

ghost · 2024-06-19T05:55:49Z

runtime/ints.c

    }
  }
-  if (sign < 0) res = - res;
+  if (sign < 0) res = ~res + 1;


Not really fond of this. Wouldn't res = (uint64_t)-res be good enough to silence those warnings?

I agree with @dustanddreams 's comment. Writing ~x + 1 instead of -x is correct but really hard to read.

res is already uint64_t. Not sure how (uint64_t)-(int64_t)res really behaves.
res = UINT64_MAX - res + 1 isn't an improvement.
What if we went with a comment? res = ~res + 1 /* res = -res */

If res is already int64_t, then (uint64_t)-res will work nicely, since its value has already been bounds checked above so we know 0 <= res <= INT64_MAX.

I think you've misread, res is unsigned (uint64_t). The compiler warning is probably overzealous, but the compiler doesn't like when an unary minus is applied to an unsigned integer.

Ah, so two casts. Ugly but still more human-friendly than bitmasking.

Turns out a single cast is sufficient.

xavierleroy

I reviewed this PR a while ago but apparently forgot to submit the review. I'm globally in favor of this PR, but there are a few places where I think a better fix is warranted, see below.

xavierleroy · 2024-06-20T14:07:17Z

runtime/major_gc.c

+  heap_sweep_words = (uintnat)heap_words;

  total_cycle_work =
-    heap_sweep_words + (heap_words * 100 / (100 + caml_percent_free));
+    heap_sweep_words + (uintnat)(heap_words * 100 / (100 + caml_percent_free));


The original code is strange: why is heap_words a double-precision FP number when it obviously always holds integer values? I'd rather declare heap_words as uintnat and fix the only place where it needs to be converted to FP:

total_cycle_work = heap_sweep_words + (uintnat) ((double) heap_words * 100.0 / (100.0 + caml_percent_free));

Done, thanks.

xavierleroy · 2024-06-20T14:09:06Z

runtime/ints.c

    }
  }
-  if (sign < 0) res = - res;
+  if (sign < 0) res = ~res + 1;


I agree with @dustanddreams 's comment. Writing ~x + 1 instead of -x is correct but really hard to read.

xavierleroy · 2024-06-20T14:10:32Z

runtime/caml/platform.h

+Caml_inline uintnat atomic_fetch_sub_verify_ge0(atomic_uintnat* p, uintnat v) {
+  uintnat result = atomic_fetch_sub(p,v);


I'm used to "fetch and add", so "fetch and sub" feels a bit strange. Why not declare the second parameter as intnat v and keep using "fetch and add -1" ? This way the rest of the code doesn't need to be changed.

It wouldn't work for direct calls to atomic_fetch_sub. There's just one of them, I'll keep it. Your idea works for the rest, thanks.

xavierleroy · 2024-06-20T14:12:38Z

runtime/signals.c

 {
-  int i;
-  for (i = 0; i < sizeof(posix_signals) / sizeof(int); i++)
+  for (int i = 0; i < (int)(sizeof(posix_signals) / sizeof(int)); i++)


Is the (int) cast necessary? Why change the original code?

I wanted to avoid a clang-cl warning, but I ended up disabling this warning (too much to "fix"). I also thought this change had a nice symmetry with the one above.

runtime/signals.c(516,21): error: comparison of integers of different signs: 'int' and 'unsigned long long' [-Werror,-Wsign-compare] 516 | for (int i = 0; i < sizeof(posix_signals) / sizeof(int); i++) | ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

MisterDA · 2024-07-05T11:17:43Z

I think I've addressed all the comments, many thanks for the reviews.

shindere · 2024-07-18T14:35:27Z

@dustanddreams are you happy with the PR in its current state? If so, I'm happy to approve on your behalf.

MisterDA · 2024-07-18T15:26:00Z

It's conflicting with trunk now, I need to rebase.

shindere · 2024-07-18T15:57:30Z

Antonin Décimo (2024/07/18 08:26 -0700):

It's conflicting with trunk now, I need to rebase.

Oops, sorry, too quick.

MisterDA · 2024-07-19T11:18:05Z

Rebased!

shindere · 2024-07-19T11:52:23Z

Thanks. Happy to approve on somebody's behalf if necessary.

ghost

🚀

This makes CAMLunused macros work with clang-cl, which doesn't define __GNUC__ and doesn't support this MSVC compiler pragma.

Avoids an unused variable warning for the perm value on Windows in: int perm = Int_val(vperm); ret = mkdir_os(path, perm);

> runtime/major_gc.c: warning C4244: '=': conversion from 'double' to 'uintnat', possible loss of data > runtime/major_gc.c: warning C4244: '=': conversion from 'double' to 'uintnat', possible loss of data Co-authored-by: Xavier Leroy <xavier.leroy@college-de-france.fr>

> runtime/gc_ctrl.c: warning C4244: 'initializing': conversion from 'uintnat' to 'double', possible loss of data > runtime/major_gc.c: warning C4244: 'initializing': conversion from 'uintnat' to 'double', possible loss of data > runtime/major_gc.c: warning C4244: 'initializing': conversion from 'intnat' to 'double', possible loss of data > runtime/major_gc.c: warning C4244: 'initializing': conversion from 'intnat' to 'double', possible loss of data

> runtime/major_gc.c(1353): warning C4244: 'initializing': conversion from 'intnat' to 'double', possible loss of data

> runtime/memprof.c: warning C4244: 'initializing': conversion from 'int32_t' to 'float', possible loss of data

gettimeofday_win32.c: warning C4244: =: conversion from ULONGLONG to double, possible loss of data

> sleep_win32.c: warning C4244: function: conversion from double to DWORD, possible loss of data > select_win32.c: warning C4244: =: conversion from double to DWORD, possible loss of data

> runtime/ints.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned > runtime/backtrace_nat.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned > runtime/backtrace_nat.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned

> unixsupport_win32.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned > runtime/signals.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned

Static values are zero-initialized by default. clang-cl would warn that the remaining fields are missing initializers.

This prevents warnings when -1 is converted to an unsigned type.

No need to test for versions of MSVC before 1939, they don't support C11 atomics. MSVC now supports the inline C keyword. No need for __assume(0) to mark unreachable code paths as ExitProcess is already __declspec(noreturn).

MisterDA · 2024-07-22T09:05:39Z

Rebased to fix conflicts.

shindere · 2024-07-23T10:03:09Z

Thanks. Approving this on behalf of @dustanddreams.

nojb reviewed Jun 18, 2024

View reviewed changes

ghost reviewed Jun 19, 2024

View reviewed changes

xavierleroy reviewed Jul 3, 2024

View reviewed changes

MisterDA force-pushed the msvc-warnings branch 4 times, most recently from 6e6a091 to e48bc05 Compare July 4, 2024 09:56

MisterDA mentioned this pull request Jul 4, 2024

clang-cl: support remaining compiler builtins #13280

Merged

MisterDA force-pushed the msvc-warnings branch 2 times, most recently from 5b011b0 to c2f6f38 Compare July 17, 2024 08:34

MisterDA force-pushed the msvc-warnings branch from c2f6f38 to 4bee22c Compare July 19, 2024 10:48

ghost approved these changes Jul 22, 2024

View reviewed changes

MisterDA and others added 12 commits July 22, 2024 11:01

sak: remove unused macros, fix symbols visibility

8f5ce4d

Port CAMLunused* to C23/C++17

e2739cf

This makes CAMLunused macros work with clang-cl, which doesn't define __GNUC__ and doesn't support this MSVC compiler pragma.

Remove unreferenced local variables

25fe1a1

Work around _wmkdir not taking a permissions parameter

45e4ff8

Avoids an unused variable warning for the perm value on Windows in: int perm = Int_val(vperm); ret = mkdir_os(path, perm);

Fix MSVC warning C4244 converting from intnat to double

5d7a093

> runtime/major_gc.c(1353): warning C4244: 'initializing': conversion from 'intnat' to 'double', possible loss of data

Fix MSVC warning C4244 converting from int32_t to float

aa40f71

> runtime/memprof.c: warning C4244: 'initializing': conversion from 'int32_t' to 'float', possible loss of data

Fix MSVC warning C4244 converting from ULONGLONG to double

a79d086

gettimeofday_win32.c: warning C4244: =: conversion from ULONGLONG to double, possible loss of data

Fix MSVC warning C4244 converting from double to DWORD

e5a3ad3

> sleep_win32.c: warning C4244: function: conversion from double to DWORD, possible loss of data > select_win32.c: warning C4244: =: conversion from double to DWORD, possible loss of data

Fix MSVC warning C4146 applying unary minus operator to unsigned

8366c2f

> unixsupport_win32.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned > runtime/signals.c: warning C4146: unary minus operator applied to unsigned type, result still unsigned

MisterDA added 6 commits July 22, 2024 11:01

Fix minor int/size_t conversion warning

856927e

Prevent incomplete initialization warning of a static value

6cfd112

Static values are zero-initialized by default. clang-cl would warn that the remaining fields are missing initializers.

Prevent sign conversion warnings when decrementing with atomic_fetch_add

a31ee5b

This prevents warnings when -1 is converted to an unsigned type.

Enable some warnings of MSVC and clang-cl

8c7f4df

Remove workarounds for old versions of MSVC

254a9d2

No need to test for versions of MSVC before 1939, they don't support C11 atomics. MSVC now supports the inline C keyword. No need for __assume(0) to mark unreachable code paths as ExitProcess is already __declspec(noreturn).

Fix include style of system headers and of caml/ headers in tests

bb7a0fc

MisterDA force-pushed the msvc-warnings branch from 4bee22c to bb7a0fc Compare July 22, 2024 09:05

shindere approved these changes Jul 23, 2024

View reviewed changes

shindere merged commit 201b0ac into ocaml:trunk Jul 23, 2024

MisterDA deleted the msvc-warnings branch July 23, 2024 10:25

		Caml_inline uintnat atomic_fetch_sub_verify_ge0(atomic_uintnat* p, uintnat v) {
		uintnat result = atomic_fetch_sub(p,v);

Conversation

MisterDA commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Jul 5, 2024

Uh oh!

shindere commented Jul 18, 2024 via email

Uh oh!

MisterDA commented Jul 18, 2024

Uh oh!

shindere commented Jul 18, 2024 via email

Uh oh!

MisterDA commented Jul 19, 2024

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Jul 22, 2024

Uh oh!

shindere commented Jul 23, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MisterDA commented Jun 18, 2024 •

edited

Loading