feat: compile llcppg with llgo and inline toolchain#653
Conversation
Summary of ChangesHello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors llcppg to inline its sub-tools into a single process, aiming to improve performance and maintainability by removing the overhead of spawning separate processes and inter-process communication. However, a security audit identified two medium-severity vulnerabilities in cmd/llcppg/llcppg.go: insecure file permissions when writing the symbol table and a potential path traversal vulnerability when determining the output directory from the configuration file. These issues need to be addressed to enhance the tool's robustness against malicious configurations and local attacks. Additionally, there are a few suggestions to improve error handling and overall file permissions, which will make the tool more robust and user-friendly.
|
The architectural change from subprocess-based execution to in-process library calls is a solid improvement -- it eliminates serialization overhead and simplifies the data flow. The package restructuring ( Key issues to address: symbol table file written with 0777 permissions, potential nil dereference if |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
- Coverage 85.16% 83.32% -1.84%
==========================================
Files 28 28
Lines 2447 2501 +54
==========================================
Hits 2084 2084
- Misses 315 369 +54
Partials 48 48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| FILENAME="llgo${VERSION_NUMBER}.${OS}-${ARCH}.tar.gz" | ||
| URL="https://github.com/goplus/llgo/releases/download/${VERSION}/${FILENAME}" | ||
| # Use fork releases while upstream release is not available yet. | ||
| URL="https://github.com/luoliwoshang/llgo/releases/download/${VERSION}/${FILENAME}" |
There was a problem hiding this comment.
NOTE: This should be replaced with latest version when llgo release.
There was a problem hiding this comment.
@xgopilot actually because the goplus/llgo with v0.12.1 have lots problem for compile goplus/gogen，and goplus/main branch is fix this problem，but the goplus/llgo is not release the latest code，so we current use the fork luoliwoshang/llgo to use the bug fix version llgo，when llgo release we will replace the repo，make a better comment !
and use this version
https://github.com/luoliwoshang/llgo/releases/tag/v0.12.2-preview
will more like the next version for llgo
There was a problem hiding this comment.
@xgopilot actually because the goplus/llgo with v0.12.1 have lots problem for compile goplus/gogen，and goplus/main branch is fix this problem，but the goplus/llgo is not release the latest code，so we current use the fork luoliwoshang/llgo to use the bug fix version llgo，when llgo release we will replace the repo，make a better comment !
and use this version
https://github.com/luoliwoshang/llgo/releases/tag/v0.12.2-preview
will more like the next version for llgo
There was a problem hiding this comment.
Thanks for the context! That makes sense — upstream goplus/llgo v0.12.1 has compilation issues with goplus/gogen, and the fix is on main but not yet released.
A few suggestions for the comment to capture this context:
# Temporary: use fork releases because goplus/llgo v0.12.1 has gogen
# compilation bugs fixed on main but not yet released.
# Revert to github.com/goplus/llgo once v0.12.2+ is available.
URL="https://github.com/luoliwoshang/llgo/releases/download/${VERSION}/${FILENAME}"Also, if v0.12.2-preview is the intended tag, consider updating the default version in action.yml from v0.12.14 to v0.12.2-preview to better signal this is a pre-release.
There was a problem hiding this comment.
Updated the fork comment in download-llgo.sh and switched LLGo version to v0.12.2-preview across all CI files. Changes are ready but need push access.
Changes Made
download-llgo.sh: Improved comment to explain why the fork is used (goplus/llgo v0.12.1 has gogen compilation bugs, fix on main but unreleased) with aTODOto revert once v0.12.2+ is officially releasedaction.yml: Default versionv0.12.14→v0.12.2-previewend2end.yml,gentest.yml,go.yml: Matrix versionv0.12.14→v0.12.2-preview
Permission Required
Hi @luoliwoshang! I've prepared your changes but need permission to push to your fork.
View Details & Solution
Target: luoliwoshang/llcppg (fork)
Push blocked:
remote: Permission to luoliwoshang/llcppg.git denied to xgopilot[bot].
fatal: unable to access 'https://github.com/luoliwoshang/llcppg.git/': The requested URL returned error: 403
Recommended Solution: Install GitHub App
- Install xgopilot
- Select repository:
luoliwoshang/llcppg - Click "Install"
- Return here and comment to retry
There was a problem hiding this comment.
@xgopilot now i give you permission ， try to give the PR!
There was a problem hiding this comment.
Done! Pushed the changes and created a PR in your fork:
PR: luoliwoshang#8
Changes
download-llgo.sh: Improved comment explaining the fork usage (goplus/llgo v0.12.1 gogen compilation bugs, fix on main but unreleased) with aTODOto revert once v0.12.2+ is officially releasedaction.yml+ workflow files: Versionv0.12.14→v0.12.2-preview
Please merge that PR into your llcppg-compile-with-llgo branch to include these changes in this PR.
| if err := tmpFile.Close(); err != nil { | ||
| _ = os.Remove(tmpName) | ||
| return err | ||
| } | ||
| defer os.Remove(tmpName) |
There was a problem hiding this comment.
NOTE: It seems like AI defense code, just defer os.Remove(tmpName) should work. However, it's fine to keep it.
There was a problem hiding this comment.
NOTE: this will break out current architecture, however, refactor is in deed for llcppg. So i think it's not so important for now to consider a lot about it?
MeteorsLiu
left a comment
MeteorsLiu
left a comment
There was a problem hiding this comment.
Approved, but architecture is a huge problem for now, becasue we combined all these modules now instead of llcppgsymg + llcppsigfetch + gogensig. However, it's acceptable for now.
2 participants
Summary
_xtool/llcppsymg/internal/symgto_xtool/symgand update imports_xtool/llcppsigfetch/internal/parseto_xtool/parseand update importscmd/llcppgcall symg/parse/convert in-process instead of spawning tool binaries*llcppg.Pkg) directly to convert incmd/llcppg, avoiding sigfetch JSON/unmarshal hopinstall.shto installcmd/llcppgviallgo installWhy
Validation
GOCACHE=/tmp/llcppg-gocache go test -vet=off ./cmd/llcppg -run TestNonExistent -count=1