ci: Enable standard library assertions in debug builds

[benjaminhuth]

Should add assertions that check preconditions of calls to standardlibrary functions.

[paulgessinger]

Maybe we should put this behind an option and enable it explicitly in the CI? That way we wouldn't silently modify local Debug builds. We could also reuse the ACTS_FORCE_ASSERTIONS flag.

I'm definitely uncomfortable with adding this to RelWithDebInfo.

[benjaminhuth]

Hmm I added it also for clang... but actually, I do not check which stdlib is used. Maybe we should add both macros unconditionally of the CMAKE_COMPILER_ID.

Regarding the option, you mean to avoid rebuild after this PR?

[paulgessinger]

@benjaminhuth I don't think we should unexpectedly enable any assertions outside of the CI, even in Debug builds. But if you gate this with ACTS_FORCE_ASSERTIONS, which we turn on in the CI, and don't make it depend on the build type, we could still get the assertions there.

[benjaminhuth]

Okay yeah that makes sense, ACTS_FORCE_ASSERTIONS seems a good place to do this I haven't though of.

[benjaminhuth]

But just out of interest @paulgessinger: To me every standard library assertion would be a violated precondition, so what would be the issue with enabling them for RelWithDebMode?

GCC documentation even recommendes them for production builds actually (https://gcc.gnu.org/wiki/LibstdcxxDebugMode)

[github-actions]

📊: Physics performance monitoring for 1038fcb

Full contents
🟥 summary not found!

[paulgessinger]

@benjaminhuth I don't see it being recommended, just that it's suitable to run in production. It's a HUGE change to enable e.g. bounds checking quietly. ATLAS builds with RelWithDebInfo, so we'd force any compilation unit compiled in Athena that links with ACTS to have these assertions on. That'd be super disruptive.

[benjaminhuth]

Yeah I agree that its a huge change, but would be a good change in my opinion. But yeah, I wasn't aware that by default acts is build in RelWithDebInfo for production builds in ATLAS. Thats of course a different story then...

[paulgessinger]

Yeah I agree that its a huge change, but would be a good change in my opinion.

Sure, but there's no way we can decide this as a library. We can test with these assertions on (which is a very good idea imo), and experiments can then decide if they want them, but there's no way we can force this.

[andiwand]

Also this does not come for free right? I guess the standard lib functions will do extra checks which are skipped otherwise. So it is a bit like with our assertions while ours might be more expensive

[paulgessinger]

@andiwand I believe these are reasonably light checks, like bounds checks or smart pointer nullptr checks, which a reasonable branch predictor will probably get right most of the time (at least that's the justification for this in Rust land). So @benjaminhuth is probably right that they're not terrible for performance and are likely safe for production use.

[paulgessinger]

Seems like the detray conversion code does some out-of-bounds access, which ostensibly with these flags are checked at compile-time.

/usr/include/c++/13/bits/stl_algobase.h:437:30: error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' forming offset [16, 17] is out of the bounds [0, 16] of object '<anonymous>' with type 'const double [2]' [-Werror=array-bounds=]
  437 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/acts/acts/Plugins/Json/src/DetrayJsonHelper.cpp: In function 'std::tuple<unsigned int, std::vector<double, std::allocator<double> > > Acts::DetrayJsonHelper::maskFromBounds(const Acts::SurfaceBounds&, bool)':
/__w/acts/acts/Plugins/Json/src/DetrayJsonHelper.cpp:46:47: note: '<anonymous>' declared here
   46 |         boundaries = {bValues[0u], bValues[1u]};
      |                                               ^
cc1plus: all warnings being treated as errors

[benjaminhuth]

I'll go into this later/next week

[andiwand]

@andiwand I believe these are reasonably light checks, like bounds checks or smart pointer nullptr checks, which a reasonable branch predictor will probably get right most of the time (at least that's the justification for this in Rust land). So @benjaminhuth is probably right that they're not terrible for performance and are likely safe for production use.

Right but ultimately this is something which should be measured before being deployed anywhere