search

VROOM-Project / vroom

Vehicle Routing Open-source Optimization Machine
http://vroom-project.org/
BSD 2-Clause "Simplified" License
1.31k stars 327 forks source link

Use address and UB sanitizers #1082

Open jcoupey opened 5 months ago

jcoupey commented 5 months ago

For testing/debug purposes, we should probably turn on sanitizers in order to try and catch potential code problems. A good place to start would be to add -fsanitize=address -fno-omit-frame-pointer and -fsanitize=undefined to the default (non-release) build and see what happens when running on various benchmarks.

My current understanding is that both gcc and clang support those flags. From this gcc doc it looks like the UB checks would be the same under the hood. Not sure about the address checking, probably worth checking with both compilers.

jcoupey commented 4 months ago

I did a simple test using clang-15 and basic VRPTW benchmark instances. The good news is that nothing is caught by the sanitizers, but the slowdown is somewhere in the x15 to x20 range!

This makes it impracticable when doing proper benchmarking, which happens quite a lot during dev phases. So I have mixed feelings about that: on one hand it would be nice to always enable the sanitizers in dev mode, on the other hand it is only realistic to do so occasionally for targeted tests.

Maybe a good trade-off would be to define a subset of the usual academic benchmarks (e.g. a few instances per type), then periodically run on those with the sanitizers enabled. This could be done before releases, on even on CI if it is fast enough.

kkarbowiak commented 4 months ago

I think adding some checks in a CI is a very good idea. If the slowdown is this big, those could be limited only to release branches (or even release tags). If they catch an error, it can later be backtraced using git bisect command.

An imortant thing would be to try and exercise as many paths in code as possible (so use instances of many types).

One additional sanitizer that could be useful is array-bounds, but I'd have to check whether it's supported by both gcc and clang or just one of them.

jcoupey commented 4 months ago

The time it takes to solve a bunch of small instances from various benchmarks would not be that big compared to the overall build time, so probably worth it. I can work on selecting a variety of such instances.

Ideally we would build with those flags on CI in order to run the tests, but keep the flags out of the way for local dev builds. Not sure exactly how this could be achieved conveniently.