llcppdump: support PresumedFile - #699
Conversation
There was a problem hiding this comment.
Review: llcppdump PresumedFile support
This refactor cleanly replaces the lib/clang GoString helper with a generic String[T stringer] in clang/clang.go, drops unused SourceLocation accessors, and adds translation-unit / presumed-file helpers to support source-file filtering in llcppdump. The removal of lib/clang symbols is safe — the only importers (clang/clang.go, cmd/llcppdump/cppdump.go) are updated here, and the _xtool/internal/libclang copy that still uses GoString/File() is a separate package.
The main concern is a NULL-safety regression: the old GoString guarded CStr() against NULL before handing it to c.GoString, and the new code paths dropped that guard (see inline comments).
Minor (non-inline):
lib/clang/clang.go— the newly added exported wrappersTranslationUnit.File,TranslationUnit.Spelling, andFile.FileNamehave only//llgo:linkdirectives and no doc comment, unlike every other exported wrapper in the file. Worth a one-line doc, especially notingSpelling/FileNamereturn aStringthe caller mustDispose().lib/clang/clang.go— the newFile.FileNameuses//llgo:link(no space) while the also-newFile/Spellinguse// llgo:link(with space). The dominant convention in this file is the spaced form; consider aligning for consistency.
| return clang.GoString(v.String()) | ||
| str := v.String() | ||
| defer str.Dispose() | ||
| return c.GoString(str.CStr()) |
There was a problem hiding this comment.
[P1] clang.String drops nil-guard on CStr(); risks nil deref
clang.String replaces lib/clang.GoString, but the old helper guarded the C string before conversion:
cstr := clangStr.CStr()
if cstr != nil {
str = c.GoString(cstr)
}The new version calls c.GoString(str.CStr()) unconditionally. clang_getCString (CStr()) returns NULL for an invalid/default CXString, and c.GoString links straight to the llgo string routine with no NULL guard, so a NULL CStr() scans from a nil pointer instead of yielding the previous safe empty string. Suggest restoring the guard:
func String[T stringer](v T) string {
str := v.String()
defer str.Dispose()
if cstr := str.CStr(); cstr != nil {
return c.GoString(cstr)
}
return ""
}| if presumedFile != nil { | ||
| loc := cur.Location() | ||
| at := clang.PresumedFile(loc) | ||
| cmpf := c.Strcmp(at.CStr(), presumedFile) |
There was a problem hiding this comment.
[P1] strcmp on a possibly-NULL CStr is undefined behavior
clang.PresumedFile(loc) can return a CXString whose CStr() is NULL when the cursor's location is invalid (built-ins, macro-expansion artifacts, etc.). Passing NULL to c.Strcmp (C strcmp) is undefined behavior and typically segfaults. Since cur.Location() runs for every visited cursor during recursive traversal, this can be hit in practice. Guard at.CStr() (and the presumedFile operand from main) against NULL before the compare, or compare via the safe Go-string path. Note: the at.Dispose() ordering itself is fine — CStr() is consumed by Strcmp before disposal.
| clang.VisitChildren(node, func(cur, parent clang.Cursor) clang.ChildVisitResult { | ||
| if presumedFile != nil { | ||
| loc := cur.Location() | ||
| at := clang.PresumedFile(loc) |
There was a problem hiding this comment.
[P3] PresumedFile resolved per-node during full AST traversal
For every visited child this resolves clang_getPresumedLocation and allocates+frees a CXString, purely to filter by source file. On a real translation unit (system headers pulled in transitively) that is O(nodes) libclang calls and string allocations. Since this lives in the llcppdump debug tool the practical impact is bounded, but if a cheaper predicate fits the intent, clang_Location_isFromMainFile(loc) is a single call with no string allocation, or comparing CXFile handles avoids the strcmp entirely. Also consider filtering on cur.Kind (a free struct-field read) before doing the location resolution.
| */ | ||
| type SourceLocation = clang.SourceLocation | ||
|
|
||
| // PresumedFile returns the presumed file name for the given source location. |
There was a problem hiding this comment.
[P3] Document PresumedFile's dispose-ownership contract
PresumedFile returns an owning clang.String the caller must Dispose() (callers do so at cppdump.go:36 and :83). The sibling String[T] helper in this file disposes internally, so this function deviates from that pattern. A one-line doc noting the returned string must be disposed (and that an invalid location yields an empty string) would prevent future leaks.
No description provided.