code review 43480049: debug/goobj: expand package prefix correctly

debug/goobj: expand package prefix correctly

[rsc, 2013-12-18 21:00:55]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-12-18 21:17:08]

No test?  I thought part of the whole linker+compiler in Go plan was, as
refactoring, to start adding tests?



On Thu, Dec 19, 2013 at 1:00 AM, <rsc@golang.org> wrote:

> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> debug/goobj: expand package prefix correctly
>
> Please review this at https://codereview.appspot.com/43480049/
>
> Affected files (+61, -10 lines):
>   M src/cmd/gc/subr.c
>   M src/cmd/ld/lib.c
>   M src/pkg/debug/goobj/read.go
>
>
> Index: src/cmd/gc/subr.c
> ===================================================================
> --- a/src/cmd/gc/subr.c
> +++ b/src/cmd/gc/subr.c
> @@ -3614,7 +3614,8 @@
>   * only in the last segment of the path, and it makes for happier
>   * users if we escape that as little as possible.
>   *
> - * If you edit this, edit ../ld/lib.c:/^pathtoprefix copy too.
> + * If you edit this, edit ../ld/lib.c:/^pathtoprefix too.
> + * If you edit this, edit ../../pkg/debug/goobj/read.go:/importPathToPrefix
> too.
>   */
>  static char*
>  pathtoprefix(char *s)
> Index: src/cmd/ld/lib.c
> ===================================================================
> --- a/src/cmd/ld/lib.c
> +++ b/src/cmd/ld/lib.c
> @@ -851,7 +851,8 @@
>   * Invalid bytes turn into %xx.         Right now the only bytes that need
>   * escaping are %, ., and ", but we escape all control characters too.
>   *
> - * Must be same as ../gc/subr.c:/^pathtoprefix.
> + * If you edit this, edit ../gc/subr.c:/^pathtoprefix too.
> + * If you edit this, edit ../../pkg/debug/goobj/read.go:/importPathToPrefix
> too.
>   */
>  static char*
>  pathtoprefix(char *s)
> Index: src/pkg/debug/goobj/read.go
> ===================================================================
> --- a/src/pkg/debug/goobj/read.go
> +++ b/src/pkg/debug/goobj/read.go
> @@ -16,6 +16,7 @@
>         "fmt"
>         "io"
>         "strconv"
> +       "strings"
>  )
>
>  // A SymKind describes the kind of memory represented by a symbol.
> @@ -180,14 +181,53 @@
>
>  // An objReader is an object file reader.
>  type objReader struct {
> -       p      *Package
> -       b      *bufio.Reader
> -       f      io.ReadSeeker
> -       err    error
> -       offset int64
> -       limit  int64
> -       tmp    [256]byte
> -       pkg    string
> +       p         *Package
> +       b         *bufio.Reader
> +       f         io.ReadSeeker
> +       err       error
> +       offset    int64
> +       limit     int64
> +       tmp       [256]byte
> +       pkg       string
> +       pkgprefix string
> +}
> +
> +// importPathToPrefix returns the prefix that will be used in the
> +// final symbol table for the given import path.
> +// We escape '%', '"', all control characters and non-ASCII bytes,
> +// and any '.' after the final slash.
> +//
> +// See ../../../cmd/ld/lib.c:/^pathtoprefix and
> +// ../../../cmd/gc/subr.c:/^pathtoprefix.
> +func importPathToPrefix(s string) string {
> +       // find first character past the last slash, if any, or else 0.
> +       l := strings.LastIndex(s, "/") + 1
> +
> +       // check for chars that need escaping
> +       n := 0
> +       for r := 0; r < len(s); r++ {
> +               if c := s[r]; c <= ' ' || (c == '.' && r >= l) || c == '%'
> || c == '"' || c >= 0x7F {
> +                       n++
> +               }
> +       }
> +
> +       // quick exit
> +       if n == 0 {
> +               return s
> +       }
> +
> +       // escape
> +       const hex = "0123456789abcdef"
> +       p := make([]byte, 0, len(s)+2*n)
> +       for r := 0; r < len(s); r++ {
> +               if c := s[r]; c <= ' ' || (c == '.' && r >= l) || c == '%'
> || c == '"' || c >= 0x7F {
> +                       p = append(p, '%', hex[c>>4], hex[c&0xF])
> +               } else {
> +                       p = append(p, c)
> +               }
> +       }
> +
> +       return string(p)
>  }
>
>  // init initializes r to read package p from f.
> @@ -198,6 +238,7 @@
>         r.limit, _ = f.Seek(0, 2)
>         f.Seek(r.offset, 0)
>         r.b = bufio.NewReader(f)
> +       r.pkgprefix = importPathToPrefix(p.ImportPath) + "."
>  }
>
>  // error records that an error occurred.
> @@ -296,6 +337,11 @@
>  func (r *objReader) readSymID() SymID {
>         name, vers := r.readString(), r.readInt()
>
> +       // In a symbol name in an object file, "". denotes the
> +       // prefix for the package in which the object file has been found.
> +       // Expand it.
> +       name = strings.Replace(name, `"".`, r.pkgprefix, -1)
> +
>         // An individual object file only records version 0 (extern) or 1
> (static).
>         // To make static symbols unique across all files being read, we
>         // replace version 1 with the version corresponding to the current
> @@ -346,6 +392,9 @@
>  // Parse parses an object file or archive from r,
>  // assuming that its import path is pkgpath.
>  func Parse(r io.ReadSeeker, pkgpath string) (*Package, error) {
> +       if pkgpath == "" {
> +               pkgpath = `""`
> +       }
>         p := new(Package)
>         p.ImportPath = pkgpath
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[rsc, 2013-12-18 21:48:59]

There really is a test! It's the only test in this whole package so far.
But I forgot the hg add.

[rsc, 2013-12-18 21:49:21]

Hello r@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[iant, 2013-12-18 21:51:13]

LGTM

[bradfitz, 2013-12-18 21:51:28]

LGTM

hah :)

[r, 2013-12-18 21:53:42]

https://codereview.appspot.com/43480049/diff/60001/src/pkg/debug/goobj/read.go
File src/pkg/debug/goobj/read.go (right):

https://codereview.appspot.com/43480049/diff/60001/src/pkg/debug/goobj/read.g...
src/pkg/debug/goobj/read.go:209: if c := s[r]; c <= ' ' || (c == '.' && r >= l)
|| c == '%' || c == '"' || c >= 0x7F {
i kept reading this as r >= 1.  maybe s/l/slash/

https://codereview.appspot.com/43480049/diff/60001/src/pkg/debug/goobj/read_t...
File src/pkg/debug/goobj/read_test.go (right):

https://codereview.appspot.com/43480049/diff/60001/src/pkg/debug/goobj/read_t...
src/pkg/debug/goobj/read_test.go:16: {"foo.bar/baz.quux", "foo.bar/baz%2equux"},
need more examples to cover things like % in the existing name, unlikely though
that is.
also i always always test the empty string in string to string routines.

[rsc, 2013-12-18 21:59:50]

Hello r@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-12-18 22:05:48]

LGTM

[rsc, 2013-12-19 00:00:56]

*** Submitted as https://code.google.com/p/go/source/detail?r=8bb79d542532 ***

debug/goobj: expand package prefix correctly

R=r, bradfitz
CC=golang-dev
https://codereview.appspot.com/43480049