proc: better support for C pointers (#1997)

- treat C pointers as arrays
- print 'char *' variables as strings

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.

_fixtures/testvariablescgo/test.c

@@ -0,0 +1,38 @@
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+
+#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);
+}

_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()
+}

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 xev.Flags&VariableCPtr == 0 {
-		if !isarrptr {
+			_, isarrptr := xev.RealType.(*godwarf.PtrType).Type.(*godwarf.ArrayType)
-			return nil, cantindex
+			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)
 	}
 

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)
+		}
+	}
+}

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 {

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())
+				}
+			}
+
+		}
+	})
+}