ci: Enable standard library assertions in debug builds #3759
Conversation
github-actions
bot
added
the
Infrastructure
|
Maybe we should put this behind an option and enable it explicitly in the CI? That way we wouldn't silently modify local I'm definitely uncomfortable with adding this to |
|
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 Regarding the option, you mean to avoid rebuild after this PR? |
|
@benjaminhuth I don't think we should unexpectedly enable any assertions outside of the CI, even in |
|
Okay yeah that makes sense, |
|
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 GCC documentation even recommendes them for production builds actually (https://gcc.gnu.org/wiki/LibstdcxxDebugMode) |
|
@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 |
|
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... |
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. |
|
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 |
|
@andiwand I believe these are reasonably light checks, like bounds checks or smart pointer |
|
Seems like the detray conversion code does some out-of-bounds access, which ostensibly with these flags are checked at compile-time. |
|
I'll go into this later/next week |
Right but ultimately this is something which should be measured before being deployed anywhere |
|
@andiwand And despite all of this, it should be off-by-default. |
WalkthroughConditional logic for compiler flags, added it is. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmake/ActsCompilerOptions.cmake (1)
16-19: Wisdom to share about stdlib assertions, I have.Hmmmm. Both stdlib assertion flags added unconditionally, I see. Correct this approach is, as work it will regardless of stdlib used. But document the impact of these assertions, we must.
Consider these suggestions, you should:
- Add comments explaining the purpose and impact of each flag
- Document performance implications in debug builds
- Clarify behavior in different build types
# Add assertions to standard libraries +# _GLIBCXX_ASSERTIONS: Enables assertions in GNU libstdc++ (bounds checking, etc.) +# _LIBCPP_DEBUG: Enables assertions in LLVM libc++ (iterator validation, etc.) +# Note: Both flags are added as they are no-op if the respective stdlib is not used if(ACTS_FORCE_ASSERTIONS) set(cxx_flags "${cxx_flags} -D_GLIBCXX_ASSERTIONS -D_LIBCPP_DEBUG") endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cmake/ActsCompilerOptions.cmake(1 hunks)
🔇 Additional comments (1)
cmake/ActsCompilerOptions.cmake (1)
17-17: Verify the impact on build configurations, we must.
Control these assertions based on build type, consider you should. In RelWithDebInfo builds, careful we must be.
✅ Verification successful
Hmm, clear the path forward is now. Concerns about build configurations, address we can.
From the gathered wisdom, observe I do:
- RelWithDebInfo is the default build type, set in cmake/ActsCompilerOptions.cmake it is
- ACTS_FORCE_ASSERTIONS already properly handled it is:
- When enabled, adds -D_GLIBCXX_ASSERTIONS and -D_LIBCPP_DEBUG flags it does
- Optional feature it remains, OFF by default it is
- Clear warning message about side effects, present in CMakeLists.txt it is
No changes needed I see. The current implementation, wise it already is. Control assertions based on build type? Through ACTS_FORCE_ASSERTIONS flag, achieved it already is. Optional and explicit the control remains, as it should be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ACTS_FORCE_ASSERTIONS is used across different build configurations
# Search for build type configurations
rg -A 5 "CMAKE_BUILD_TYPE|ACTS_FORCE_ASSERTIONS"
# Look for any existing assertion-related configuration
fd -e cmake -e txt -x rg "ASSERTIONS|DEBUG|NDEBUG"
Length of output: 7516
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Core/include/Acts/EventData/detail/GenerateParameters.hpp (1)
109-118: Add assertions for parameter bounds, we should.In alignment with PR objectives of enabling standard library assertions, add parameter validation we must. Verify that:
- thetaMin <= thetaMax
- phiMin <= phiMax
Apply this enhancement, you should:
template <typename generator_t> inline std::pair<double, double> generateBoundDirection( generator_t& rng, const GenerateBoundDirectionOptions& options) { + assert(options.thetaMin <= options.thetaMax && "Invalid theta range"); + assert(options.phiMin <= options.phiMax && "Invalid phi range"); + using UniformReal = std::uniform_real_distribution<double>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Core/include/Acts/EventData/detail/GenerateParameters.hpp(1 hunks)
🔇 Additional comments (2)
Core/include/Acts/EventData/detail/GenerateParameters.hpp (2)
117-118: Consistent with theta changes, the eta calculations are.
Aligned with the theta bounds swap, the eta calculations now are. Maintain mathematical harmony between theta and eta transformations, this does.
109-114: Correct, the angle calculations now are, hmmmm.
Swapped the theta bounds in cosine calculations, wisdom shows. Proper spherical coordinates generation, this ensures. Used nextafter() correctly to include upper bound, prevent floating-point exclusion it does.
Run these commands, we must, to verify the test coverage:
✅ Verification successful
Correct the angle calculations are, young padawan
Examined the Force flows through the code, I have. The swapping of thetaMin and thetaMax in cosine calculations, proper it is. Two reasons, there are:
- Used twice in the codebase
generateBoundDirectionis, for creating test parameters inGenerateParameters.hpp. Consistent with spherical coordinate system conventions, this change remains. - The
nextafterusage, wisdom shows - ensures the upper bound included is, preventing floating-point exclusion it does.
No test failures this change will cause, as random direction generation for testing purposes only it serves. The Force is strong with this modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for generateBoundDirection function
# Search for test files using this function
rg -l "generateBoundDirection" --type cpp | grep -i "test"
# Look for specific test assertions on angle ranges
ast-grep --pattern 'BOOST_CHECK$_($_)' | grep -i "theta\|phi\|eta"
Length of output: 125
Script:
#!/bin/bash
# Search for test files containing generateBoundDirection with more context
rg -l "generateBoundDirection" Tests/
# Look for test files specifically in UnitTests directory
rg -l "generateBoundDirection" UnitTests/
# Check for actual usage of the function in tests with context
rg "generateBoundDirection" --type cpp -C 3
# Look for test assertions related to bound parameters
rg "BOOST_" --type cpp | grep -i "bound.*param"
Length of output: 9762
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Core/include/Acts/EventData/detail/GenerateParameters.hpp (1)
112-118: Correct, the cosine calculations are! Yet clearer, the comments could be.Wise decision to flip cosThetaMin and cosThetaMax, it is. For in the realm of [0,π], as theta increases, its cosine decreases. Proper handling of floating-point bounds with std::nextafter, you have achieved.
Enhance the comment clarity, we should:
- // Get cosThetaMin from thetaMax and vice versa, because cos is - // monothonical decreasing between [0, pi] + // Get cosThetaMin from thetaMax and vice versa, because: + // 1. cos(θ) is monotonically decreasing in [0,π] + // 2. For θ₁ < θ₂, cos(θ₁) > cos(θ₂)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Core/include/Acts/EventData/detail/GenerateParameters.hpp(1 hunks)Plugins/Json/src/DetrayJsonHelper.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Json/src/DetrayJsonHelper.cpp
🔇 Additional comments (2)
Core/include/Acts/EventData/detail/GenerateParameters.hpp (2)
105-107: Wise assertions added, they are!
Validate the bounds of theta, these assertions do. Prevent invalid input that could lead to incorrect results or undefined behavior, they will. Strong with the Force, these preconditions are!
121-122: Consistent with theta changes, the eta calculations are!
Correctly flipped, the eta calculations are, maintaining harmony with the theta transformations. Yet verify the impact on downstream calculations, we must.
✅ Verification successful
The previous pattern didn't yield results, so I'm broadening the search to find any usage of these parameters and functions. This will help us verify the impact of the eta calculations change.
Correct and well-tested, the eta-theta transformation is!
Through the Force, seen the truth I have. The relationship between eta and theta, consistently implemented it is across the codebase. In the unit tests at Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp, verified the mathematical relationship is:
- Bidirectional transformation tested it is:
etaFromTheta(thetaFromEta(eta)) = eta - At boundary conditions checked it is
- For both float and double types, validated it is
The flipped calculations in GenerateParameters.hpp, correct they are:
double etaMin = Acts::AngleHelpers::etaFromTheta(options.thetaMax);
double etaMax = Acts::AngleHelpers::etaFromTheta(options.thetaMin);For theta and eta, inverse relationship they share - when theta increases, eta decreases. Therefore, the max theta gives min eta, and min theta gives max eta. Mathematical harmony, maintained it is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of generateBoundDirection with etaUniform=true
ast-grep --pattern 'generateBoundDirection($_,$_).etaUniform = true'
Length of output: 70
Script:
#!/bin/bash
# Search for usages of generateBoundDirection and etaUniform
rg -A 3 "generateBoundDirection.*etaUniform"
# Search for files that include GenerateParameters.hpp
rg -l '#include.*GenerateParameters\.hpp'
# Search for direct usage of etaFromTheta
rg "etaFromTheta"
Length of output: 3163
Pulled out of #3759 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Enhanced validation for direction generation parameters to ensure they are within acceptable ranges. - Adjusted calculations for cosine and eta values to align with the properties of the cosine function. - **Tests** - Modified random distribution ranges in unit tests for the `CylinderVolumeBuilder` to improve test coverage and accuracy. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
acts-project-service
added
the
Breaks Athena build
Should add assertions that check preconditions of calls to standardlibrary functions.
Summary by CodeRabbit