From 8edde922ee5189f1d049edb9487e6090dd9d45bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Fri, 7 May 2021 23:24:41 +0100 Subject: [PATCH] remove unused code spotted by -coverprofile Remove some asthelper APIs that haven't been used for some time. They can be recovered from the git history if needed again. One type assertion in the literals package is always true. Embedded field objects are handled near the top of transformGo, so the extra !obj.Embedded() check was always true. Remove it. We always obfuscate standalone funcs now, so the obfuscatedTypesPackage check is no longer necessary. This was necessary when we used to not obfuscate func names when they were used in linkname directives. The workaround for test package imports in obfuscatedTypesPackage I had to add a few commits ago no longer seems to be necessary. This might be thanks to the simplification with functions in the paragraph just above. It's impossible to run garble without -trimpath nowadays, as we error before the build even starts: $ go build -toolexec=garble go tool compile: exit status 1 cannot open shared file, this is most likely due to not running "garble [command]" When run as "garble build", the trimpath flag is always set. So the check in alterTrimpath never triggers anymore, and couldn't be tested. Finally, simplify the handling of comment syntax in printFile, and add a few TODOs for other code paths not covered by our existing tests. Total code coverage is up from 90.3% to 91.0%. --- internal/asthelper/asthelper.go | 25 ----------- internal/literals/literals.go | 6 +-- main.go | 80 +++++++++------------------------ position.go | 7 +-- reverse.go | 1 + 5 files changed, 26 insertions(+), 93 deletions(-) diff --git a/internal/asthelper/asthelper.go b/internal/asthelper/asthelper.go index 83ccb1c..c941713 100644 --- a/internal/asthelper/asthelper.go +++ b/internal/asthelper/asthelper.go @@ -26,22 +26,6 @@ func IntLit(value int) *ast.BasicLit { } } -// Float32Lit returns an ast.BasicLit of kind FLOAT, 32 bit -func Float32Lit(value float32) *ast.BasicLit { - return &ast.BasicLit{ - Kind: token.FLOAT, - Value: strconv.FormatFloat(float64(value), 'f', -1, 32), - } -} - -// Float64Lit returns an ast.BasicLit of kind FLOAT, 64 bit -func Float64Lit(value float64) *ast.BasicLit { - return &ast.BasicLit{ - Kind: token.FLOAT, - Value: strconv.FormatFloat(value, 'f', -1, 64), - } -} - // IndexExpr "name[index]" func IndexExpr(name string, index ast.Expr) *ast.IndexExpr { return &ast.IndexExpr{ @@ -81,15 +65,6 @@ func ReturnStmt(results ...ast.Expr) *ast.ReturnStmt { } } -// BoundsCheck "_ = name[pos]" -func BoundsCheck(name string, pos int) *ast.AssignStmt { - return &ast.AssignStmt{ - Lhs: []ast.Expr{ast.NewIdent("_")}, - Tok: token.ASSIGN, - Rhs: []ast.Expr{IndexExpr("data", IntLit(pos))}, - } -} - // BlockStmt a block of multiple statments e.g. a function body func BlockStmt(stmts ...ast.Stmt) *ast.BlockStmt { return &ast.BlockStmt{List: stmts} diff --git a/internal/literals/literals.go b/internal/literals/literals.go index 335a75a..92e4c51 100644 --- a/internal/literals/literals.go +++ b/internal/literals/literals.go @@ -42,11 +42,7 @@ func Obfuscate(file *ast.File, info *types.Info, fset *token.FileSet, ignoreObj return true } for _, spec := range x.Specs { - spec, ok := spec.(*ast.ValueSpec) - if !ok { - return false - } - + spec := spec.(*ast.ValueSpec) // guaranteed for Tok==CONST if len(spec.Values) == 0 { // skip constants with inferred values return false diff --git a/main.go b/main.go index a03c219..23c85ac 100644 --- a/main.go +++ b/main.go @@ -137,19 +137,6 @@ func obfuscatedTypesPackage(path string) *types.Package { panic("called obfuscatedTypesPackage on the current package?") } entry, ok := importCfgEntries[path] - - // A "test/bar_test [test/bar.test]" package can try to look at - // "test/bar [test/bar.test]", for some reason. - // That really shouldn't happen, because external test packages are - // meant to just import the original non-test package. - // For now, correct for this weirdness by stripping the suffix. - // Without this change, test.txt fails. - // TODO(mvdan): figure out the cause of this. - if !ok && strings.HasSuffix(path, ".test]") { - path = path[:strings.IndexByte(path, ' ')] - entry, ok = importCfgEntries[path] - } - if !ok { // Handle the case where the name is defined in an indirectly // imported package. Since only direct imports show up in our @@ -281,6 +268,7 @@ func goVersionOK() bool { startDateIdx := strings.IndexByte(commitAndDate, ' ') + 1 if startDateIdx < 0 { // Custom version; assume the user knows what they're doing. + // TODO: cover this in a test return true } @@ -510,11 +498,7 @@ func transformAsm(args []string) ([]string, error) { flags = flagSetValue(flags, "-p", curPkg.obfuscatedImportPath()) } - var err error - flags, err = alterTrimpath(flags) - if err != nil { - return nil, err - } + flags = alterTrimpath(flags) // We need to replace all function references with their obfuscated name // counterparts. @@ -643,10 +627,7 @@ func transformCompile(args []string) ([]string, error) { } } - flags, err = alterTrimpath(flags) - if err != nil { - return nil, err - } + flags = alterTrimpath(flags) newImportCfg, err := processImportCfg(flags) if err != nil { @@ -1253,27 +1234,25 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File { // If the struct of this field was not obfuscated, do not obfuscate // any of that struct's fields. - if !obj.Embedded() { - parent, ok := cursor.Parent().(*ast.SelectorExpr) - if !ok { - break - } - named := namedType(tf.info.TypeOf(parent.X)) - if named == nil { - break // TODO(mvdan): add a test - } - if name := named.Obj().Name(); strings.HasPrefix(name, "_Ctype") { - // A field accessor on a cgo type, such as a C struct. - // We're not obfuscating cgo names. + parent, ok := cursor.Parent().(*ast.SelectorExpr) + if !ok { + break + } + named := namedType(tf.info.TypeOf(parent.X)) + if named == nil { + break // TODO(mvdan): add a test + } + if name := named.Obj().Name(); strings.HasPrefix(name, "_Ctype") { + // A field accessor on a cgo type, such as a C struct. + // We're not obfuscating cgo names. + return true + } + if path != curPkg.ImportPath { + obfPkg := obfuscatedTypesPackage(path) + if obfPkg.Scope().Lookup(named.Obj().Name()) != nil { + tf.recordIgnore(named, path) return true } - if path != curPkg.ImportPath { - obfPkg := obfuscatedTypesPackage(path) - if obfPkg.Scope().Lookup(named.Obj().Name()) != nil { - tf.recordIgnore(named, path) - return true - } - } } case *types.TypeName: if parentScope != pkg.Scope() { @@ -1306,17 +1285,6 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File { if strings.HasPrefix(node.Name, "Test") && isTestSignature(sign) { return true // don't break tests } - - // If this is an imported func that was linknamed to a - // different symbol name, the imported package did not - // obfuscate the original func name. - // Don't do it here either. - if path != curPkg.ImportPath { - obfPkg := obfuscatedTypesPackage(path) - if obfPkg.Scope().Lookup(obj.Name()) != nil { - return true - } - } default: return true // we only want to rename the above } @@ -1511,19 +1479,15 @@ func splitFlagsFromArgs(all []string) (flags, args []string) { return all, nil } -func alterTrimpath(flags []string) ([]string, error) { +func alterTrimpath(flags []string) []string { // If the value of -trimpath doesn't contain the separator ';', the 'go // build' command is most likely not using '-trimpath'. trimpath := flagValue(flags, "-trimpath") - if !strings.Contains(trimpath, ";") { - return nil, fmt.Errorf("-toolexec=garble should be used alongside -trimpath") - } // Add our temporary dir to the beginning of -trimpath, so that we don't // leak temporary dirs. Needs to be at the beginning, since there may be // shorter prefixes later in the list, such as $PWD if TMPDIR=$PWD/tmp. - flags = flagSetValue(flags, "-trimpath", sharedTempDir+"=>;"+trimpath) - return flags, nil + return flagSetValue(flags, "-trimpath", sharedTempDir+"=>;"+trimpath) } // buildFlags is obtained from 'go help build' as of Go 1.15. diff --git a/position.go b/position.go index d55324c..d13048b 100644 --- a/position.go +++ b/position.go @@ -135,17 +135,14 @@ func printFile(file1 *ast.File) ([]byte, error) { buf2.Write(src[copied:comment.offset]) copied = comment.offset + // We assume that all comments are of the form "/*text*/". // Make sure there is whitespace at either side of a comment. // Otherwise, we could change the syntax of the program. // Inserting "/*text*/" in "a/b" // must be "a/ /*text*/ b", // as "a//*text*/b" is tokenized as a "//" comment. buf2.WriteByte(' ') buf2.WriteString(comment.text) - if strings.HasPrefix(comment.text, "//") { - buf2.WriteByte('\n') - } else { - buf2.WriteByte(' ') - } + buf2.WriteByte(' ') } buf2.Write(src[copied:]) return buf2.Bytes(), nil diff --git a/reverse.go b/reverse.go index 82ce387..ce713c4 100644 --- a/reverse.go +++ b/reverse.go @@ -149,6 +149,7 @@ func commandReverse(args []string) error { } return nil } + // TODO: cover this code in the tests too anyModified := false for _, path := range args { f, err := os.Open(path)