search

ares-emulator / ares

ares is a cross-platform, open source, multi-system emulator, focusing on accuracy and preservation.
https://ares-emu.net
Other
940 stars 114 forks source link

Concerning-looking compiler warnings #1399

Open omarandlorraine opened 7 months ago

omarandlorraine commented 7 months ago

Describe the bug I'm getting some concerning-looking compiler warnings when I build the project.

To Reproduce Steps to reproduce the behavior:

  1. check out commit ed0a1c8852f0448905f7ed0d75cb8702203805bc
  2. type make

Expected behavior I expected the project to compile cleanly, especially since a cursory glance over the source, it appears to be well-written, and modern C++.

Screenshots Not a screenshot exactly, but here's one of the warnings, and there are a few others like it.

make[1]: Entering directory '/home/sam/ares/desktop-ui'
Linking out/ares ...
In function ‘copy’,
    inlined from ‘copy’ at ../nall/memory.hpp:142:17,
    inlined from ‘_allocate’ at ../nall/string/allocator/adaptive.hpp:100:15,
    inlined from ‘reserve’ at ../nall/string/allocator/adaptive.hpp:51:14,
    inlined from ‘resize.constprop.isra’ at ../nall/string/allocator/adaptive.hpp:63:10:
../nall/memory.hpp:137:19: warning: ‘__builtin_memcpy’ writing 24 bytes into a region of size 20 [-Wstringop-overflow=]

So it looks like where's going to get a buffer over-run at runtime.

Some of the warnings are coming from third party things like libco and sljit. I'm not sure if we have a policy here for keeping the diff to upstream minimal, or if there's another reason to let this be?

Additional context My compiler is Debian clang version 14.0.6, in case that matters

I see there has already been some effort to fix some warnings (see #1295), but apparently some remain. Maybe because I am building for a system no-one else is building it on (Debian Linux). Would the project accept a pull-request that enables -Wall -Wextra or whatever to get this kind of thing tidied up, and perhaps a CI/CD pipeline to ensure compliance in future pull-requests?

remutro commented 1 month ago

Hmmm.... I checked the automated runner builds which uses Ubuntu for Linux builds (obviously Debian based) and these warnings do show in that log, so not doing a good job catching them. They don't seem to appear on other platforms though even when clang is in use. I agree it looks concerning though so it should be investigated.

remutro commented 1 month ago

I brought this issue up in our Discord, and Luke found the following:

It’s the copy loop in the string allocator. If you replace it with a memcpy the error goes away; but there’s a note in the code that manually looping is faster May be able to fix it with min around the length calculation memory::copy(_data, _temp, SSO); The compiler doesn’t like that we allocate data and then copy SSO bytes to it rather than checking the length of data If you swap the SSO length argument for a min of SSO and the byte capacity of data it goes away

So there doesn't seem to be any real issue with the code as written, and just using min around the length calculation should be enough to prevent the warning from occurring.