From a72723433b4d0b3a34d0ec85a2a78d7ccf7ac843 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Fri, 5 Jun 2020 20:22:40 +0200 Subject: [PATCH] proc: better support for C pointers (#1997) - treat C pointers as arrays - print 'char *' variables as strings --- Documentation/cli/expr.md | 4 + _fixtures/testvariablescgo/test.c | 38 ++++++ .../testvariablescgo/testvariablescgo.go | 11 ++ pkg/proc/eval.go | 42 +++++-- pkg/proc/proc_general_test.go | 70 +++++++++++ pkg/proc/variables.go | 111 +++++++++++++++++- service/test/variables_test.go | 43 +++++++ 7 files changed, 308 insertions(+), 11 deletions(-) create mode 100644 _fixtures/testvariablescgo/test.c create mode 100644 _fixtures/testvariablescgo/testvariablescgo.go diff --git a/Documentation/cli/expr.md b/Documentation/cli/expr.md index 0b0105b5..fcf027f0 100644 --- a/Documentation/cli/expr.md +++ b/Documentation/cli/expr.md @@ -106,3 +106,7 @@ Packages with the same name can be disambiguated by using the full package path. (dlv) p "some/package".A (dlv) p "some/other/package".A ``` + +# Pointers in Cgo + +Char pointers are always treated as NUL terminated strings, both indexing and the slice operator can be applied to them. Other C pointers can also be used similarly to Go slices, with indexing and the slice operator. In both of these cases it is up to the user to respect array bounds. diff --git a/_fixtures/testvariablescgo/test.c b/_fixtures/testvariablescgo/test.c new file mode 100644 index 00000000..2299e142 --- /dev/null +++ b/_fixtures/testvariablescgo/test.c @@ -0,0 +1,38 @@ +#include +#include +#include + +#ifdef __amd64__ +#define BREAKPOINT asm("int3;") +#elif __i386__ +#define BREAKPOINT asm("int3;") +#elif __aarch64__ +#define BREAKPOINT asm("brk 0;") +#endif + +#define N 100 + +struct align_check { + int a; + char b; +}; + +void testfn(void) { + const char *s0 = "a string"; + const char *longstring = "averylongstring0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g0123456789h0123456789"; + int *v = malloc(sizeof(int) * N); + struct align_check *v_align_check = malloc(sizeof(struct align_check) * N); + + for (int i = 0; i < N; i++) { + v[i] = i; + v_align_check[i].a = i; + v_align_check[i].b = i; + } + + char *s = malloc(strlen(s0) + 1); + strcpy(s, s0); + + BREAKPOINT; + + printf("%s %s %p %p\n", s, longstring, v, v_align_check); +} diff --git a/_fixtures/testvariablescgo/testvariablescgo.go b/_fixtures/testvariablescgo/testvariablescgo.go new file mode 100644 index 00000000..be4e199b --- /dev/null +++ b/_fixtures/testvariablescgo/testvariablescgo.go @@ -0,0 +1,11 @@ +package main + +// #cgo CFLAGS: -g -Wall -O0 -std=gnu99 +/* +extern void testfn(void); +*/ +import "C" + +func main() { + C.testfn() +} diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index d541bb87..f43ee5b2 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -1214,7 +1214,9 @@ func (scope *EvalScope) evalIndex(node *ast.IndexExpr) (*Variable, error) { return nil, xev.Unreadable } - xev = xev.maybeDereference() + if xev.Flags&VariableCPtr == 0 { + xev = xev.maybeDereference() + } idxev, err := scope.evalAST(node.Index) if err != nil { @@ -1228,11 +1230,13 @@ func (scope *EvalScope) evalIndex(node *ast.IndexExpr) (*Variable, error) { if xev == nilVariable { return nil, cantindex } - _, isarrptr := xev.RealType.(*godwarf.PtrType).Type.(*godwarf.ArrayType) - if !isarrptr { - return nil, cantindex + if xev.Flags&VariableCPtr == 0 { + _, isarrptr := xev.RealType.(*godwarf.PtrType).Type.(*godwarf.ArrayType) + if !isarrptr { + return nil, cantindex + } + xev = xev.maybeDereference() } - xev = xev.maybeDereference() fallthrough case reflect.Slice, reflect.Array, reflect.String: @@ -1309,6 +1313,11 @@ func (scope *EvalScope) evalReslice(node *ast.SliceExpr) (*Variable, error) { return nil, fmt.Errorf("map index out of bounds") } return xev, nil + case reflect.Ptr: + if xev.Flags&VariableCPtr != 0 { + return xev.reslice(low, high) + } + fallthrough default: return nil, fmt.Errorf("can not slice \"%s\" (type %s)", exprToString(node.X), xev.TypeString()) } @@ -1844,7 +1853,13 @@ func sameType(t1, t2 godwarf.Type) bool { } func (v *Variable) sliceAccess(idx int) (*Variable, error) { - if idx < 0 || int64(idx) >= v.Len { + wrong := false + if v.Flags&VariableCPtr == 0 { + wrong = idx < 0 || int64(idx) >= v.Len + } else { + wrong = idx < 0 + } + if wrong { return nil, fmt.Errorf("index out of bounds") } mem := v.mem @@ -1889,7 +1904,18 @@ func (v *Variable) mapAccess(idx *Variable) (*Variable, error) { } func (v *Variable) reslice(low int64, high int64) (*Variable, error) { - if low < 0 || low >= v.Len || high < 0 || high > v.Len { + wrong := false + cptrNeedsFakeSlice := false + if v.Flags&VariableCPtr == 0 { + wrong = low < 0 || low >= v.Len || high < 0 || high > v.Len + } else { + wrong = low < 0 || high < 0 + if high == 0 { + high = low + } + cptrNeedsFakeSlice = v.Kind != reflect.String + } + if wrong { return nil, fmt.Errorf("index out of bounds") } @@ -1901,7 +1927,7 @@ func (v *Variable) reslice(low int64, high int64) (*Variable, error) { } typ := v.DwarfType - if _, isarr := v.DwarfType.(*godwarf.ArrayType); isarr { + if _, isarr := v.DwarfType.(*godwarf.ArrayType); isarr || cptrNeedsFakeSlice { typ = fakeSliceType(v.fieldType) } diff --git a/pkg/proc/proc_general_test.go b/pkg/proc/proc_general_test.go index 2e35469d..9ff21608 100644 --- a/pkg/proc/proc_general_test.go +++ b/pkg/proc/proc_general_test.go @@ -24,3 +24,73 @@ func TestIssue554(t *testing.T) { t.Fatalf("should be false") } } + +type dummyMem struct { + t *testing.T + mem []byte + base uint64 + reads []memRead +} + +type memRead struct { + addr uint64 + size int +} + +func (dm *dummyMem) ReadMemory(buf []byte, addr uintptr) (int, error) { + dm.t.Logf("read addr=%#x size=%#x\n", addr, len(buf)) + dm.reads = append(dm.reads, memRead{uint64(addr), len(buf)}) + a := int64(addr) - int64(dm.base) + if a < 0 { + panic("reading below base") + } + if int(a)+len(buf) > len(dm.mem) { + panic("reading beyond end of mem") + } + copy(buf, dm.mem[a:]) + return len(buf), nil +} + +func (dm *dummyMem) WriteMemory(uintptr, []byte) (int, error) { + panic("not supported") +} + +func TestReadCStringValue(t *testing.T) { + const tgt = "a test string" + const maxstrlen = 64 + + dm := &dummyMem{t: t} + dm.mem = make([]byte, maxstrlen) + copy(dm.mem, tgt) + + for _, tc := range []struct { + base uint64 + numreads int + }{ + {0x5000, 1}, + {0x5001, 1}, + {0x4fff, 2}, + {uint64(0x5000 - len(tgt) - 1), 1}, + {uint64(0x5000-len(tgt)-1) + 1, 2}, + } { + t.Logf("base is %#x\n", tc.base) + dm.base = tc.base + dm.reads = dm.reads[:0] + out, done, err := readCStringValue(dm, uintptr(tc.base), LoadConfig{MaxStringLen: maxstrlen}) + if err != nil { + t.Errorf("base=%#x readCStringValue: %v", tc.base, err) + } + if !done { + t.Errorf("base=%#x expected done but wasn't", tc.base) + } + if out != tgt { + t.Errorf("base=%#x got %q expected %q", tc.base, out, tgt) + } + if len(dm.reads) != tc.numreads { + t.Errorf("base=%#x wrong number of reads %d (expected %d)", tc.base, len(dm.reads), tc.numreads) + } + if tc.base == 0x4fff && dm.reads[0].size != 1 { + t.Errorf("base=%#x first read in not of one byte", tc.base) + } + } +} diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index 51a695bf..97b8157f 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -75,6 +75,8 @@ const ( // the variable is the return value of a function call and allocated on a // frame that no longer exists) VariableFakeAddress + // VariableCPrt means the variable is a C pointer + VariableCPtr ) // Variable represents a variable. It contains the address, name, @@ -612,6 +614,18 @@ func newVariable(name string, addr uintptr, dwarfType godwarf.Type, bi *BinaryIn v.Kind = reflect.Ptr if _, isvoid := t.Type.(*godwarf.VoidType); isvoid { v.Kind = reflect.UnsafePointer + } else if isCgoType(bi, t) { + v.Flags |= VariableCPtr + v.fieldType = t.Type + v.stride = alignAddr(v.fieldType.Size(), v.fieldType.Align()) + v.Len = 0 + if isCgoCharPtr(bi, t) { + v.Kind = reflect.String + } + if v.Addr != 0 { + n, err := readUintRaw(v.mem, v.Addr, int64(v.bi.Arch.PtrSize())) + v.Base, v.Unreadable = uintptr(n), err + } } case *godwarf.ChanType: v.Kind = reflect.Chan @@ -656,6 +670,14 @@ func newVariable(name string, addr uintptr, dwarfType godwarf.Type, bi *BinaryIn } case *godwarf.IntType: v.Kind = reflect.Int + case *godwarf.CharType: + // Rest of the code assumes that Kind == reflect.Int implies RealType == + // godwarf.IntType. + v.RealType = &godwarf.IntType{BasicType: t.BasicType} + v.Kind = reflect.Int + case *godwarf.UcharType: + v.RealType = &godwarf.IntType{BasicType: t.BasicType} + v.Kind = reflect.Int case *godwarf.UintType: v.Kind = reflect.Uint case *godwarf.FloatType: @@ -1204,7 +1226,18 @@ func (v *Variable) loadValueInternal(recurseLevel int, cfg LoadConfig) { case reflect.String: var val string - val, v.Unreadable = readStringValue(DereferenceMemory(v.mem), v.Base, v.Len, cfg) + if v.Flags&VariableCPtr == 0 { + val, v.Unreadable = readStringValue(DereferenceMemory(v.mem), v.Base, v.Len, cfg) + } else { + var done bool + val, done, v.Unreadable = readCStringValue(DereferenceMemory(v.mem), v.Base, cfg) + if v.Unreadable == nil { + v.Len = int64(len(val)) + if !done { + v.Len++ + } + } + } v.Value = constant.MakeString(val) case reflect.Slice, reflect.Array: @@ -1341,9 +1374,50 @@ func readStringValue(mem MemoryReadWriter, addr uintptr, strlen int64, cfg LoadC return "", fmt.Errorf("could not read string at %#v due to %s", addr, err) } - retstr := *(*string)(unsafe.Pointer(&val)) + return string(val), nil +} - return retstr, nil +func readCStringValue(mem MemoryReadWriter, addr uintptr, cfg LoadConfig) (string, bool, error) { + buf := make([]byte, cfg.MaxStringLen) // + val := buf[:0] // part of the string we've already read + + for len(buf) > 0 { + // Reads some memory for the string but (a) never more than we would + // need (considering cfg.MaxStringLen), and (b) never cross a page boundary + // until we're sure we have to. + // The page check is needed to avoid getting an I/O error for reading + // memory we don't even need. + // We don't know how big a page is but 1024 is a reasonable minimum common + // divisor for all architectures. + curaddr := addr + uintptr(len(val)) + maxsize := int(alignAddr(int64(curaddr+1), 1024) - int64(curaddr)) + size := len(buf) + if size > maxsize { + size = maxsize + } + + _, err := mem.ReadMemory(buf[:size], curaddr) + if err != nil { + return "", false, fmt.Errorf("could not read string at %#v due to %s", addr, err) + } + + done := false + for i := 0; i < size; i++ { + if buf[i] == 0 { + done = true + size = i + break + } + } + + val = val[:len(val)+size] + buf = buf[size:] + if done { + return string(val), true, nil + } + } + + return string(val), false, nil } const ( @@ -2157,6 +2231,37 @@ func popcnt(x uint64) int { return int(x) & (1<<7 - 1) } +func isCgoType(bi *BinaryInfo, typ godwarf.Type) bool { + cu := bi.Images[typ.Common().Index].findCompileUnitForOffset(typ.Common().Offset) + if cu == nil { + return false + } + return !cu.isgo +} + +func isCgoCharPtr(bi *BinaryInfo, typ *godwarf.PtrType) bool { + if !isCgoType(bi, typ) { + return false + } + + fieldtyp := typ.Type +resolveQualTypedef: + for { + switch t := fieldtyp.(type) { + case *godwarf.QualType: + fieldtyp = t.Type + case *godwarf.TypedefType: + fieldtyp = t.Type + default: + break resolveQualTypedef + } + } + + _, ischar := fieldtyp.(*godwarf.CharType) + _, isuchar := fieldtyp.(*godwarf.UcharType) + return ischar || isuchar +} + func (cm constantsMap) Get(typ godwarf.Type) *constantType { ctyp := cm[dwarfRef{typ.Common().Index, typ.Common().Offset}] if ctyp == nil { diff --git a/service/test/variables_test.go b/service/test/variables_test.go index 61b544b6..657c81dc 100644 --- a/service/test/variables_test.go +++ b/service/test/variables_test.go @@ -1519,3 +1519,46 @@ func TestPluginVariables(t *testing.T) { assertVariable(t, vb, varTest{"b", true, `github.com/go-delve/delve/_fixtures/internal/pluginsupport.SomethingElse(*github.com/go-delve/delve/_fixtures/plugin2.asomethingelse) *{x: 1, y: 4}`, ``, `github.com/go-delve/delve/_fixtures/internal/pluginsupport.SomethingElse`, nil}) }) } + +func TestCgoEval(t *testing.T) { + testcases := []varTest{ + {"s", true, `"a string"`, `"a string"`, "*char", nil}, + {"longstring", true, `"averylongstring0123456789a0123456789b0123456789c0123456789d01234...+1 more"`, `"averylongstring0123456789a0123456789b0123456789c0123456789d01234...+1 more"`, "*const char", nil}, + {"longstring[64:]", false, `"56789e0123456789f0123456789g0123456789h0123456789"`, `"56789e0123456789f0123456789g0123456789h0123456789"`, "*const char", nil}, + {"s[3]", false, "116", "116", "char", nil}, + {"v", true, "*0", "(*int)(…", "*int", nil}, + {"v[1]", false, "1", "1", "int", nil}, + {"v[90]", false, "90", "90", "int", nil}, + {"v[:5]", false, "[]int len: 5, cap: 5, [0,1,2,3,4]", "[]int len: 5, cap: 5, [...]", "[]int", nil}, + {"v_align_check", true, "*align_check {a: 0, b: 0}", "(*struct align_check)(…", "*struct align_check", nil}, + {"v_align_check[1]", false, "align_check {a: 1, b: 1}", "align_check {a: 1, b: 1}", "align_check", nil}, + {"v_align_check[90]", false, "align_check {a: 90, b: 90}", "align_check {a: 90, b: 90}", "align_check", nil}, + } + + protest.AllowRecording(t) + withTestProcess("testvariablescgo/", t, func(p *proc.Target, fixture protest.Fixture) { + assertNoError(p.Continue(), t, "Continue() returned an error") + for _, tc := range testcases { + variable, err := evalVariable(p, tc.name, pnormalLoadConfig) + if err != nil && err.Error() == "evaluating methods not supported on this version of Go" { + // this type of eval is unsupported with the current version of Go. + continue + } + if tc.err == nil { + assertNoError(err, t, fmt.Sprintf("EvalExpression(%s) returned an error", tc.name)) + assertVariable(t, variable, tc) + variable, err := evalVariable(p, tc.name, pshortLoadConfig) + assertNoError(err, t, fmt.Sprintf("EvalExpression(%s, pshortLoadConfig) returned an error", tc.name)) + assertVariable(t, variable, tc.alternateVarTest()) + } else { + if err == nil { + t.Fatalf("Expected error %s, got no error (%s)", tc.err.Error(), tc.name) + } + if tc.err.Error() != err.Error() { + t.Fatalf("Unexpected error. Expected %s got %s", tc.err.Error(), err.Error()) + } + } + + } + }) +}