Bugs caused by signedness errors

nyanpasu64 commented 2 years ago

I tried building libvgm on gcc/clang, and found a few signedness warnings:

https://github.com/ValleyBell/libvgm/blob/57713471eef1db49e84f39d4ee3ac83662f01316/emu/cores/np_nes_dmc.c#L562

abs(dmc->clock - DEFAULT_CLK_PAL) is wrong, you need to cast clock to signed INT32.

https://github.com/ValleyBell/libvgm/blob/57713471eef1db49e84f39d4ee3ac83662f01316/player/s98player.cpp#L353

Comparing char to hex literals above 0x80 doesn't work, because char is signed. Perhaps cast to unsigned char, or use '\xEF' and such instead. Also assuming that EF BB BF corresponds to UTF-8 is horrifying, since UTF-8 documents should not include a "byte order mark" in the first place. (But https://vgmrips.net/mirror/s98spec3.txt mentions a BOM... it's sad to see bad practice be standardized.)

https://github.com/ValleyBell/libvgm/blob/57713471eef1db49e84f39d4ee3ac83662f01316/emu/cores/fmopn.c#L1492

fc is a UINT32 parameter, overwritten within the function body. Should the parameter be made INT32, or should we use a different local variable? This was caught by Clang's -Wtautological-unsigned-zero-compare:

GCC enables -Wtype-limits under -Wextra. Warnings under GCC's -Wtype-limits is covered in Clang by -Wtautological-type-limit-compare and -Wtautological-unsigned-zero-compare.

On my projects, I enable -Wall -Wextra and on Clang -Wtautological-unsigned-zero-compare as well. (Clang doesn't add -Wtautological-unsigned-zero-compare to -Wextra due to https://reviews.llvm.org/D51545. I think that's a mistake, because in my experience, this warning has caught multiple bugs with zero false positives.)

EDIT: Just found out that -Wtype-limits is included in -Wextra on GCC, and enables -Wtautological-unsigned-zero-compare on Clang. So I can include it on both GCC and Clang, and remove one more GCC/Clang check in my personal projects.

There are likely more undiscovered errors. I get a massive pile of warnings with -Wconversion -Wsign-conversion, but I think most don't result in problems in practice. I still think programs should not produce warnings under -Wconversion -Wsign-conversion, but don't feel like fixing up libvgm.

`CMAKE_C_FLAGS` doesn't work

Note that libvgm setting CMAKE_C_FLAGS on the top-level CMakeLists.txt doesn't actually get reflected in the build flags. Working options include:

Set CMAKE_C_FLAGS in every subproject's CMakeLists.txt.
To avoid specifying the same variable in many files, have each subproject call set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${EXTRA_C_FLAGS}).
Call target_compile_options() on each target, either in the subproject or the global CMakeLists.txt.
(Untested) Call add_compile_options in the root CMakeLists.txt.

ValleyBell commented 2 years ago

Thanks - I fixed those warnings in ea0f1dc74e97d9bfaf190c556e2de3f54fd5bb0c. The bugs in the cores you listed above were all part of my fixes/modifications, so thanks a lot!

I need to admit that I just ignore most of the warnings from sound cores - especially because Visual Studio often outputs more typecast warnings then necessary. Some sound cores are a bit sloppy when it comes to type conversions and doing too many modifications makes it harder to pull updates. I'd like my own code to be mostly free of warnings though. (even though I still ignore size_t <-> UINT32 warnings sometimes)

The S98 one was a good catch. The code was never tested properly, because I couldn't find any S98 tags with UTF-8 encoding. I now verified that it works by handcrafting a file that uses UTF-8 tags in the way the specification says.

CMAKE_C_FLAGS: I'm sure that this works just fine with GCC. I just did this:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -91,6 +91,7 @@ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wpointer-arith -Winit-self -Wstrict-aliasin
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wformat -Wformat-security -Wformat-nonliteral")
 #set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fstack-protector")
 #set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fstack-protector")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror")

 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_C_FLAGS}")
 set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${CMAKE_C_FLAGS_DEBUG}")

and it immediately stopped in emu/cores/sn76496.c due to an unused function.

Maybe it didn't work for you due to if(CMAKE_COMPILER_IS_GNUCC)?

nyanpasu64 commented 2 years ago

Perhaps it's because I'm using Clang rather than GCC, whose compiler flags almost (but not quite) match GCC. In my projects (exotracker, qspcplay, TODO qvgmsplit) I share most flags between Clang and GCC, with a few Clang-only flags (no GCC-only flags yet).

ValleyBell commented 2 years ago

Yes, that's what I mean: CMAKE_COMPILER_IS_GNUCC probably doesn't include Clang.

ValleyBell / libvgm