Try enabling LTO for LLVM 18 + ASan configuration

[laurynas-biveinis]

No description provided.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch llvm-asan-lto

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.34%. Comparing base (8eef8c0) to head (77792dd).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #580      +/-   ##
==========================================
+ Coverage   98.31%   98.34%   +0.03%     
==========================================
  Files          35       35              
  Lines        6483     6483              
==========================================
+ Hits         6374     6376       +2     
+ Misses        109      107       -2     

Flag	Coverage Δ
Debug	98.28% <ø> (-0.04%)	⬇️
Release	97.73% <ø> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Claude finished @laurynas-biveinis's task —— View job

---

Code Review: LTO + ASan Configuration

Todo List:

• Read the modified CMakeLists.txt file
• Analyze the LTO + ASan configuration changes
• Check for potential compatibility issues
• Review code quality and best practices
• Assess performance considerations
• Check security implications
• Provide comprehensive feedback

---

Review Summary

This PR attempts to enable LTO (Link Time Optimization) for LLVM clang 21+ when AddressSanitizer is enabled. The change modifies the condition that previously disabled LTO for all clang 17+ versions with ASan to only disable it for clang 17-20.

Code Quality & Best Practices ✅

Strengths:

• Clean, well-documented code change with clear comments explaining the rationale
• Follows existing code patterns and style in the CMake configuration
• Properly formatted condition with appropriate line breaks for readability
• Updated status message accurately reflects the new version range (17-20)
• Version range logic is correct: VERSION_GREATER_EQUAL 17.0 AND VERSION_LESS 21.0

Technical Analysis

What Changed

# Before: Disabled LTO for clang 17+
elseif(is_clang AND SANITIZE_ADDRESS AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 17.0)

# After: Disables LTO only for clang 17-20  
elseif(is_clang AND SANITIZE_ADDRESS AND
  CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 17.0 AND
  CMAKE_CXX_COMPILER_VERSION VERSION_LESS 21.0)

Root Issue Context

The original workaround addresses a specific ThinLTO bug in clang 17-20 when combined with AddressSanitizer:

/usr/bin/ld: error: Failed to link module
../libunodb_qsbr.a.llvm.3080.qsbr.cpp: Expected at most one ThinLTO module per bitcode file

Potential Issues ⚠️

1. Unverified Compatibility

• Risk: The assumption that clang 21 fixed the LTO + ASan bug is not verified
• Impact: Could cause build failures in CI/CD or production environments
• Mitigation: Should be tested across different platforms before merging

2. Missing Documentation

• No reference to upstream bug reports or LLVM release notes confirming the fix
• Future maintainers won't understand why version 21 was chosen as the cutoff

3. Platform Dependencies

• The original bug may be platform-specific (Linux/Windows/macOS)
• Different LLVM distributions might have different behavior

Performance Considerations 🚀

Benefits:

• Significant optimization opportunity: LTO can provide 5-15% performance improvements
• Memory usage: Better dead code elimination and function inlining
• Binary size: Often reduces final binary size through better optimization

Costs:

• Build time: LTO significantly increases compilation/linking time
• Memory usage during build: Higher memory requirements during link phase

Security Implications 🔒

Positive aspects:

• LTO enables better optimization which can improve security through:
  • More aggressive dead code elimination
  • Better control flow integrity
• AddressSanitizer remains fully functional for memory safety

No security concerns identified with this change.

Test Coverage 📊

Current status:

• SonarQube: Quality gate passed ✅
• Codecov: Slight coverage decrease (-0.02%) - likely unrelated to this change
• No new test failures reported

Recommendation: The change should be tested with:

• Various build configurations (Debug/Release)
• Different target platforms
• Large-scale builds to verify linking stability

Recommendations

1. Before Merging

# Test the configuration explicitly
cmake -DSANITIZE_ADDRESS=ON -DCMAKE_CXX_COMPILER=clang++-21 -DSTANDALONE=ON ..
make -j$(nproc)

2. Add Documentation

# Reference LLVM bug report or release notes that confirm fix in v21
# Example: "Fixed in LLVM 21.x.x - see https://github.com/llvm/llvm-project/issues/XXXXX"

3. Consider Conditional Approach

# Could add a cmake option to force enable/disable if issues arise
option(FORCE_LTO_WITH_ASAN "Force enable LTO with AddressSanitizer" OFF)

Verdict: ✅ APPROVE with Conditions

This is a reasonable and well-implemented change that should provide performance benefits. The code quality is excellent and follows project conventions. The main concern is verifying that clang 21 actually resolves the ThinLTO + ASan compatibility issue.

Recommend: Merge after verification testing on the target platforms.

---