Conversation
Implement LLVM/Clang coverage backend as an alternative to the existing GCC gcov/gcovr implementation. The new backend uses: - `-fprofile-instr-generate -fcoverage-mapping` for instrumented builds - `llvm-profdata merge` + `llvm-cov export` for JSON coverage reports - LLVM IR parsing (`.ll` files) for CFG-guided basic block selection Key changes: - New `LLVMCoverage` type implementing `Coverage` + `FilteredLineExtractor` - New `LLVMReport` with canonical on-disk line-set format - New LLVM IR CFG parser for function/BB/successor/line extraction - New `FilteredLineExtractor` interface to decouple engine from backend type - Config `coverage_backend` field (`"gcc"` default, `"llvm"` optional) - `NewAnalyzerFromCFGFunctions` to reuse existing BB selection logic - Full test suite including end-to-end integration tests
Reviewer's GuideAdds an LLVM/Clang source-based coverage backend alongside the existing GCC/gcovr backend, wires it into the fuzzing app and engine via a backend-agnostic Coverage/FilteredLineExtractor interface, and introduces LLVM IR-based CFG parsing plus configuration, validation, and tests to support selecting and running either backend. Sequence diagram for selecting GCC vs LLVM coverage backend in runFuzzsequenceDiagram
participant RunFuzz
participant Config
participant GCCCoverage
participant LLVMCoverage
participant Engine
RunFuzz->>Config: read Compiler.CoverageBackend
alt CoverageBackend == llvm
RunFuzz->>RunFuzz: toLLVMTargets
RunFuzz->>LLVMCoverage: NewLLVMCoverage
RunFuzz-->>Engine: Coverage (LLVMCoverage)
else CoverageBackend == gcc (default)
RunFuzz->>GCCCoverage: NewGCCCoverage
RunFuzz-->>Engine: Coverage (GCCCoverage)
end
Engine->>Engine: extractCoveredLines
Engine->>Engine: type assert FilteredLineExtractor
Engine->>LLVMCoverage: ExtractCoveredLinesFiltered
Engine-->>Engine: []string
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- In the LLVM compile path you call os.Setenv("LLVM_PROFILE_FILE", ...) without restoring the previous value, which can leak into later compilations or other parts of the process; consider saving and restoring the old value or scoping this via the command environment instead.
- The llvm-profdata/llvm-cov invocations build shell strings with fmt.Sprintf and "sh -c" (e.g. using %s/*.profraw and redirects), which will break on paths with spaces and is vulnerable to injection; prefer passing arguments directly via the executor (no shell) or at least quote/escape paths robustly.
- There are two separate demangling paths (LLVMCoverage.demangleFunctionNames using exec.Executor and llvm_cfg.demangleNames using os/exec) that implement similar logic; consolidating them into a single helper would reduce duplication and keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the LLVM compile path you call os.Setenv("LLVM_PROFILE_FILE", ...) without restoring the previous value, which can leak into later compilations or other parts of the process; consider saving and restoring the old value or scoping this via the command environment instead.
- The llvm-profdata/llvm-cov invocations build shell strings with fmt.Sprintf and "sh -c" (e.g. using %s/*.profraw and redirects), which will break on paths with spaces and is vulnerable to injection; prefer passing arguments directly via the executor (no shell) or at least quote/escape paths robustly.
- There are two separate demangling paths (LLVMCoverage.demangleFunctionNames using exec.Executor and llvm_cfg.demangleNames using os/exec) that implement similar logic; consolidating them into a single helper would reduce duplication and keep behavior consistent.
## Individual Comments
### Comment 1
<location path="internal/coverage/llvm.go" line_range="419" />
<code_context>
+
+// demangleFunctionNames returns a map of mangled -> demangled name. If no
+// demangler is configured (or it fails), names map to themselves.
+func (l *LLVMCoverage) demangleFunctionNames(export *llvmCovExport) map[string]string {
+ names := make([]string, 0)
+ seen := make(map[string]bool)
</code_context>
<issue_to_address>
**suggestion:** Consolidate demangling logic to avoid duplicated, slightly diverging implementations
Similar demangling logic exists here (via executor.Run) and in llvm_cfg.go (demangleNames using os/exec). To avoid subtle drift in behavior (e.g., error handling, whitespace, argument limits), please extract a shared demangling helper in this package and have both LLVMCoverage and the CFG parser call it with a consistent failure and name-mapping contract.
</issue_to_address>
### Comment 2
<location path="internal/config/config.go" line_range="590-591" />
<code_context>
cfg.Compiler.Oracle.Options = make(map[string]interface{})
}
+ // Coverage backend: default to "gcc" for backward compatibility.
+ if cfg.Compiler.CoverageBackend == "" {
+ cfg.Compiler.CoverageBackend = "gcc"
+ }
</code_context>
<issue_to_address>
**suggestion:** Normalize coverage_backend casing before validation to avoid surprising config errors
`validateCoverageBackend` currently expects a normalized value, but `LoadConfig` only defaults empty strings and otherwise passes the value through. Mixed-case values like `"LLVM"` or `"Gcc"` will therefore hit the default case and be rejected as unknown. Normalizing (e.g., `strings.ToLower`, and optionally `strings.TrimSpace`) before calling `validateCoverageBackend` would avoid these surprising config failures while preserving behavior.
Suggested implementation:
```golang
// Coverage backend: normalize casing and default to "gcc" for backward compatibility.
cfg.Compiler.CoverageBackend = strings.ToLower(strings.TrimSpace(cfg.Compiler.CoverageBackend))
if cfg.Compiler.CoverageBackend == "" {
cfg.Compiler.CoverageBackend = "gcc"
}
```
1. Ensure `internal/config/config.go` imports the `strings` package, e.g.:
- Add `strings` to the existing import block: `import ( ... "strings" )`.
2. Confirm that any call to `validateCoverageBackend(cfg)` in `LoadConfig` happens after this normalization block so that validation always sees a normalized `CoverageBackend` value.
</issue_to_address>
### Comment 3
<location path="internal/coverage/llvm_cfg.go" line_range="340" />
<code_context>
cmd := osexec.Command(command, names...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| // demangleFunctionNames returns a map of mangled -> demangled name. If no | ||
| // demangler is configured (or it fails), names map to themselves. | ||
| func (l *LLVMCoverage) demangleFunctionNames(export *llvmCovExport) map[string]string { |
There was a problem hiding this comment.
suggestion: Consolidate demangling logic to avoid duplicated, slightly diverging implementations
Similar demangling logic exists here (via executor.Run) and in llvm_cfg.go (demangleNames using os/exec). To avoid subtle drift in behavior (e.g., error handling, whitespace, argument limits), please extract a shared demangling helper in this package and have both LLVMCoverage and the CFG parser call it with a consistent failure and name-mapping contract.
Comment on lines
+590
to
+591
| // Coverage backend: default to "gcc" for backward compatibility. | ||
| if cfg.Compiler.CoverageBackend == "" { |
There was a problem hiding this comment.
suggestion: Normalize coverage_backend casing before validation to avoid surprising config errors
validateCoverageBackend currently expects a normalized value, but LoadConfig only defaults empty strings and otherwise passes the value through. Mixed-case values like "LLVM" or "Gcc" will therefore hit the default case and be rejected as unknown. Normalizing (e.g., strings.ToLower, and optionally strings.TrimSpace) before calling validateCoverageBackend would avoid these surprising config failures while preserving behavior.
Suggested implementation:
// Coverage backend: normalize casing and default to "gcc" for backward compatibility.
cfg.Compiler.CoverageBackend = strings.ToLower(strings.TrimSpace(cfg.Compiler.CoverageBackend))
if cfg.Compiler.CoverageBackend == "" {
cfg.Compiler.CoverageBackend = "gcc"
}- Ensure
internal/config/config.goimports thestringspackage, e.g.:- Add
stringsto the existing import block:import ( ... "strings" ).
- Add
- Confirm that any call to
validateCoverageBackend(cfg)inLoadConfighappens after this normalization block so that validation always sees a normalizedCoverageBackendvalue.
| // returning its stdout. llvm-cxxfilt and c++filt both accept names as args and | ||
| // print one demangled name per line. | ||
| func runDemangler(command string, names []string) (string, error) { | ||
| cmd := osexec.Command(command, names...) |
There was a problem hiding this comment.
security (go.lang.security.audit.dangerous-exec-command): Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement LLVM/Clang coverage backend as an alternative to the existing GCC gcov/gcovr implementation. The new backend uses:
-fprofile-instr-generate -fcoverage-mappingfor instrumented buildsllvm-profdata merge+llvm-cov exportfor JSON coverage reports.llfiles) for CFG-guided basic block selectionKey changes:
LLVMCoveragetype implementingCoverage+FilteredLineExtractorLLVMReportwith canonical on-disk line-set formatFilteredLineExtractorinterface to decouple engine from backend typecoverage_backendfield ("gcc"default,"llvm"optional)NewAnalyzerFromCFGFunctionsto reuse existing BB selection logicSummary by Sourcery
Add an LLVM-based coverage backend alongside the existing GCC/gcov implementation and integrate it into the fuzzing pipeline.
New Features:
Enhancements:
Tests: