Make Debug config use ASAN

[tophyr]

The Debug variant includes various asserts and sanity checks, and Address Sanitizer is really just another type of assert/sanity check. So, it follows that Debug should just have ASAN turned on. RelWithDebInfo can be used to produce a debugger-friendly yet error-tolerant build.

Pull Request Type

• [ ] GitHub Workflow changes
• [ ] Documentation or Wiki changes
• [x] Build and Dependency changes
• [ ] Runtime changes
  • [ ] Render changes
  • [ ] Audio changes
  • [ ] Input changes
  • [ ] Network changes
  • [ ] Other changes

Description

Related Issues

Screenshots (if applicable)

Checklist

• [x] I have tested my changes locally and verified that they work as intended.
• [x] I have documented any new or modified functionality.
• [x] I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
• [x] I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

[tophyr]

MSVC-based Windows Debug builds are failing because something in CMake is trying to be too smart about ASAN. Switching to Ninja (via https://github.com/DescentDevelopers/Descent3/pull/422) solves that failure, so I'm not going to try to address it here.

[Lgt2x]

422 introducing ninja for Windows is now merged, you can rebase on main branch. The flag seems to be slightly different for MSVC. I'd also prefer ASAN to be hidden behind a CMake option such as USE_ASAN defaulting to OFF, but enabled in CI. It may not be something you want all the time when debugging

[tophyr]

Using a flag is a good idea.. I had tried to chase down making a new config for it but that's surprisingly difficult.

What do you think of making the flag available but make asan still default to on? I do want to push people into using asan while debugging, but I agree that we shouldn't force its use.

On Sat, Jun 15, 2024 at 9:26 AM Louis Gombert @.***> wrote:

422 https://github.com/DescentDevelopers/Descent3/pull/422 introducing

ninja for Windows is now merged, you can rebase on main branch. The flag seems to be slightly different for MSVC https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170#ide-cmake. I'd also prefer ASAN to be hidden behind a CMake option such as USE_ASAN defaulting to OFF, but enabled in CI. It may not be something you want all the time when debugging

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/426#issuecomment-2169770852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RIVB4DUZLL4PEK4PCDZHRFIJAVCNFSM6AAAAABI5HG722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRZG43TAOBVGI . You are receiving this because you authored the thread.Message ID: @.***>

[Lgt2x]

ASAN can have a big impact on runtime performance, I'd leave the default to OFF

[tophyr]

I'll figure out some benchmarks to quantify that. I don't imagine that most computers people will be developing with will have any trouble handling the extra load, but that's something that'd be on me to justify for sure.

For actual perf-sensitive use I would expect people to use Release or RelWithDebInfo, which wouldn't use ASAN.

On Sat, Jun 15, 2024 at 10:03 AM Louis Gombert @.***> wrote:

ASAN can have a big impact on runtime performance, I'd leave the default to OFF

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/426#issuecomment-2169850546, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6ROHQW6R5BRYUWJOXKTZHRJUJAVCNFSM6AAAAABI5HG722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRZHA2TANJUGY . You are receiving this because you authored the thread.Message ID: @.***>

[Lgt2x]

I agree that a benchmark is necessary.

[Lgt2x]

Closing for inactivity after 3 weeks. I still wish we can add a CMake option to enable ASan, anyone is welcome to open a PR to add it