wamrc: Build with LLVM-17 - #12338
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe WAMR CMake configuration updates LLVM-version-specific C++ settings, disables assertions for one GNU and LLVM combination, and filters ChangesLLVM build compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized CMake change adjusts LLVM-version handling and library filtering, with no unresolved correctness or integration issue identified at the current head. It is merge-ready after normal checks, with no actionable merge-blocking risk remaining. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d99a2b88a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
d99a2b8 to
be46411
Compare
Ubuntu 26.04 (Resolute Racoon) does not provide to install llvm-15. So, I adopted to build with llvm-17.
Minimal exclusion works and the binary runs. Tidying the comment to match:
Simplified. Three version-gated hunks, verified by a clean rebuild —
wamrc 2.4.1, no sharedlibLLVMinDT_NEEDED.You committed the intermediate version as
ff8382463; the working tree now amends it in two places.1. C++17 only where it's needed
Moved out of the unconditional header (line 11 stays at
14) into theLLVM_FOUNDblock, gated on LLVM ≥ 16 — LLVM 15 headers build fine at C++14, 16+ require 17.2. Library filter narrowed to just
omptargetThe
^LLVM.include-regex inff8382463builds, but the binary aborts at startup:Dropping
LTO/Remarks/Polly/LLVMreshuffles the link line enough that--as-neededkeepslibLLVM-17.so.1(pulled in transitively viaLLVMDebuginfod, whoseINTERFACE_LINK_LIBRARIESisCURL::libcurl;LLVM) alongside the static components — so LLVM's cl::opt registry gets populated twice. Excluding only the genuinely-missing targets leaves the link order intact and--as-neededdropslibLLVMentirely:Also correcting something I got wrong earlier: I reported
Pollyas missing.libPolly.aandlibPollyISL.aare both present —omptarget*are the only actually-missing entries, which is why the one-line exclusion is sufficient.3. NDEBUG guard
Kept, but reduced to a single LLVM-version clause (covers 15 and 17):
list(FILTER)needs CMake 3.6; the project requires 3.12, so that's fine.One caveat on the LLVM 15 claim:
llvm-15-devhas no install candidate on this box, so I could only build against LLVM 17 and probe the export-list shape against LLVM 21. Debian/Ubuntu'sllvm-15-devexports the sameomptargettargets, so the filter applies there too — but LLVM 15 is untested end-to-end here. If CI covers it, that's the place it'll show.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Build Improvements