clean up C4244 warnings on MSVC builds

[compnerd]

C4244 appertains to implicit truncation of integer values [1]. Theoretically, these warnings could be detected using -fsanitize=implicit-unsigned-integer-truncation and -fsanitize=implicit-signed-integer-truncation as well as statically using -Werror=implicit-int-conversion.

[1] https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-3-and-4-c4244?view=msvc-170

[compnerd]

@jgm I'm not sure if this is better or worse really. We could just disable the warning if the truncation is not something worth bothering with. I have two follow up patches that will completely eradicate all warnings when building with MSVC 2022. I'm happy to rework the patches to just disable the warning as well.

[jgm]

I don't know what is better here. Happy to go with your judgment.

[compnerd]

I personally tend to go with this approach. The reasoning being that this codifies the behaviour (correct or not). This means that its less likely to change with compiler changes but does mean that it would be difficult to later identify what was intentional vs not. The count here is small enough, so, if you could verify that these sites are meant to truncate (which it did appear to me), I would just go with this.

[nwellnhof]

Note that adding explicit casts makes it impossible to detect actual truncation errors with UBSan's -fsanitize=integer.

[jgm]

In light of that, @nwellnhof , do you think this change should be reverted?

[compnerd]

True, but, at the same time, if there are truncation errors, those should be addressed. This is effectively one way to deal with them. The other way would be to change the signature of the called function. I think that the question to ask is which of the two are preferable. The goal should be to properly express the semantics of the operation, and if there are truncation issues, addressing them with explicit casts to indicate the truncation.

[nwellnhof]

No, an explicit cast doesn't deal with unexpected truncation, it just makes the cast obvious to readers of the code and helps with some compiler warnings. Regarding the generated code, there's no difference between an implicit and an explicit cast.

I'm a big fan of UBSan's integer sanitizers which catch well-defined but mostly unwanted things like unsigned integer overflow or truncations when casting. But some of these checks only work with implicit casts. Adding an explicit cast tells the sanitizer not to report an error. So if someone were to start fuzzing libcmark with these sanitizers, they'd first have to remove most of these explicit casts. That's why I'd prefer to leave the implicit casts and tell the compiler not to warn about them.

[compnerd]

The compiler is free to make assumptions and optimize based upon the truncation. It often has resulted in incorrect code generation (at least within clang). If the truncation is "well-defined" then there should be no warning generated by UBSan and that sounds like a UBSan bug. If there is a proper warning being emitted by UBSan, then there is a undefined behaviour and the desired behaviour should be codified into the code IMO.

[nwellnhof]

The compiler is free to make assumptions and optimize based upon the truncation.

There's no difference between an implicit cast and an explicit cast to the target type. For example, the following code means exactly the same:

my_type var = expression;
my_type var = (my_type) expression;

It often has resulted in incorrect code generation (at least within clang).

I think you're misunderstanding something. As I said, the generated code will be exactly the same.

If the truncation is "well-defined" then there should be no warning generated by UBSan and that sounds like a UBSan bug.

Yes, that's why the integer sanitizers are turned off by default. But as I said, these sanitizers are incredibly useful to catch actual bugs. If you have a narrowing conversion that truncates integers, it is most likely a bug. It doesn't matter whether there's an explicit cast or not.

[compnerd]

If you have a narrowing conversion that truncates integers, it is most likely a bug. It doesn't matter whether there's an explicit cast or not.

This is my point. It is a bug, and doing the explicit cast is a valid way to address the bug. Changing the types is a valid way to address it as well. I am not tied to the truncation by casting, but I'd rather get these addressed, either by the cast of changing the types. Is there another way that you would prefer to address them?

[nwellnhof]

This is my point. It is a bug

No, whether it's a bug depends on the context. An explicit cast changes nothing and makes it impossible to detect truncation with UBSan. Consider the following code (strlen returns a size_t):

int len = strlen(string);

If you can prove that the length of the string won't be larger than INT_MAX, this code is perfectly fine. If there's a possibility that the result overflows, it's a bug and -fsanitize=integer can detect the truncation. If you add an explicit cast, it's still a bug but the sanitizer won't complain.

[compnerd]

I think that is where we disagree. I think that is universally a bug. If you can prove that the length is bounded by INT_MAX you should assert that (in release mode) somehow (at which point the sanitizer should not complain as it should have the flow sensitive diagnostics to see that it is impossible). So, either do the check or widen the storage, but as is, I consider the presented code incorrect.

[nwellnhof]

I think that is universally a bug.

Even if it was, what's the difference if you add an explicit cast?

So, either do the check or widen the storage, but as is, I consider the presented code incorrect.

You could of course add range checks before each cast (and taken to the extreme before every single arithmetic operation) but that's simply not practical. You have to make certain assumptions about the range of inputs all the time. Fuzz testing with sanitizers is helpful to check whether your assumptions are correct.

[compnerd]

Even if it was, what's the difference if you add an explicit cast?

I think in that case, I wouldn't add the explicit cast, I would widen the storage to size_t.

You could of course add range checks before each cast (and taken to the extreme before every single arithmetic operation) but that's simply not practical. You have to make certain assumptions about the range of inputs all the time. Fuzz testing with sanitizers is helpful to check whether your assumptions are correct.

Sure, the way to deal with this is to audit the sites and determine what needs to be done at each of them. I don't think that the sanitizer is any more useful than the warnings - they both identify the call sides which need to be audited and have the developer make a call on what the right behaviour is. In the case of truncation, explicitly state that so that its clear to others that is the right thing or widen it as appropriate.

For what it is worth, that is practical to do - just not with C/C++. Other languages (e.g. Swift) do perform these checks on the arithmetic operations and will ensure that overflows do not go unnoticed.

[nwellnhof]

I think in that case, I wouldn't add the explicit cast, I would widen the storage to size_t.

In libcmark we deliberately store many buffer sizes and indexes as 32-bit values to save memory on 64-bit platforms. There are checks, for example in S_parser_feed, that the total input size never exceeds 32 bits. That's a perfectly reasonable and safe way to avoid unnecessary range checks in other places.

Anyway, I'm just asking to stop adding explicit casts which defeat integer sanitizers. If a compiler complains about implicit casts, simply disable these warnings.

[compnerd]

Anyway, I'm just asking to stop adding explicit casts which defeat integer sanitizers. If a compiler complains about implicit casts, simply disable these warnings.

Sure, if there are guaranteed checks, that is reasonable. But I still think that cleaning up those cases is something that should be done. If the checks are there, then it makes sense to explicitly cast the value when assigning.

[compnerd]

@jgm let me know what you would like to do here. Disabling warnings U.S.A. certainly possible but it not make sense to start disabling in larger buckets to deal with implicit conversions and truncations if those are preferred.

[jgm]

I don't fully understand this issue, but if the tradeoff is making warnings disappear vs allowing sanitizers to be used, then the latter seems more important.

[nwellnhof]

Can't we just disable C4244 and similar warnings on MSVC?

[compnerd]

Sure, but if we are disabling it for MSVC, I feel like we should also be disabling similarly for other compilers. At that point, disabling chunks of warnings would be easier :)

[nwellnhof]

gcc and clang only warn about implicit conversion with -Wconversion which we don't use. For MSVC, it might help to lower the warning level from /W4 to /W3.

It's also a good idea to treat warnings as errors when running CI tests.

[compnerd]

@nwellnhof agreed on both counts :) That is in fact, exactly what I did in #512 - dropped the warning back down to /W3. The original approach did actually mark the warning as errors as well to ensure that we didn't regress. But, if you want to mark all warnings as errors in CI, that would be a separate change IMO (a little more involved since the CI setup seems pretty complex with multiple layers of indirection).

[nwellnhof]

It should be enough to set the CFLAGS environment variable in workflows/ci.yml to -Werror for gcc and clang and to /WX for MSVC.