From 6599f2dea45892c452a0019fbf41471da9641c33 Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Wed, 30 Oct 2019 18:12:46 +0100 Subject: [PATCH 1/2] ProtoLog: Fix broken hash computation Fixes a bug caused by inconsistent paths - when transforming sources, we took the path of the file, while when generating the viewer json, we used the path relative to the root package. Test: enable protolog, verify winscope can parse it. Change-Id: Ib636515d70ca5b66da687542a84107465e51053d --- .../src/com/android/protolog/tool/ProtoLogTool.kt | 14 +++++++++----- .../android/protolog/tool/SourceTransformer.kt | 15 ++++++++++----- .../android/protolog/tool/ViewerConfigBuilder.kt | 12 ++++++++---- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt b/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt index 629f720314b2d..dda13118bb219 100644 --- a/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt +++ b/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt @@ -60,7 +60,7 @@ object ProtoLogTool { val outSrc = try { val code = tryParse(text, path) if (containsProtoLogText(text, command.protoLogClassNameArg)) { - transformer.processClass(text, path, code) + transformer.processClass(text, path, packagePath(file, code), code) } else { text } @@ -157,10 +157,7 @@ ${updates.replaceIndent(" ")} if (containsProtoLogText(text, command.protoLogClassNameArg)) { try { val code = tryParse(text, path) - val pack = if (code.packageDeclaration.isPresent) code.packageDeclaration - .get().nameAsString else "" - val newPath = pack.replace('.', '/') + '/' + file.name - builder.findLogCalls(code, newPath) + builder.findLogCalls(code, path, packagePath(file, code)) } catch (ex: ParsingException) { // If we cannot parse this file, skip it (and log why). Compilation will fail // in a subsequent build step. @@ -182,6 +179,13 @@ ${updates.replaceIndent(" ")} out.close() } + private fun packagePath(file: File, code: CompilationUnit): String { + val pack = if (code.packageDeclaration.isPresent) code.packageDeclaration + .get().nameAsString else "" + val packagePath = pack.replace('.', '/') + '/' + file.name + return packagePath + } + private fun read(command: CommandOptions) { LogParser(ViewerConfigParser()) .parse(FileInputStream(command.logProtofileArg), diff --git a/tools/protologtool/src/com/android/protolog/tool/SourceTransformer.kt b/tools/protologtool/src/com/android/protolog/tool/SourceTransformer.kt index 0ad8091f97a21..36ea41129450c 100644 --- a/tools/protologtool/src/com/android/protolog/tool/SourceTransformer.kt +++ b/tools/protologtool/src/com/android/protolog/tool/SourceTransformer.kt @@ -72,7 +72,7 @@ class SourceTransformer( } val ifStmt: IfStmt if (group.enabled) { - val hash = CodeUtils.hash(fileName, messageString, level, group) + val hash = CodeUtils.hash(packagePath, messageString, level, group) val newCall = call.clone() if (!group.textEnabled) { // Remove message string if text logging is not enabled by default. @@ -97,7 +97,7 @@ class SourceTransformer( if (argTypes.size != call.arguments.size - 2) { throw InvalidProtoLogCallException( "Number of arguments (${argTypes.size} does not mach format" + - " string in: $call", ParsingContext(fileName, call)) + " string in: $call", ParsingContext(path, call)) } val blockStmt = BlockStmt() if (argTypes.isNotEmpty()) { @@ -214,18 +214,23 @@ class SourceTransformer( StaticJavaParser.parseExpression(protoLogCacheClassName) private var processedCode: MutableList = mutableListOf() private var offsets: IntArray = IntArray(0) - private var fileName: String = "" + /** The path of the file being processed, relative to $ANDROID_BUILD_TOP */ + private var path: String = "" + /** The path of the file being processed, relative to the root package */ + private var packagePath: String = "" fun processClass( code: String, path: String, + packagePath: String, compilationUnit: CompilationUnit = StaticJavaParser.parse(code) ): String { - fileName = path + this.path = path + this.packagePath = packagePath processedCode = code.split('\n').toMutableList() offsets = IntArray(processedCode.size) - protoLogCallProcessor.process(compilationUnit, this, fileName) + protoLogCallProcessor.process(compilationUnit, this, path) return processedCode.joinToString("\n") } } diff --git a/tools/protologtool/src/com/android/protolog/tool/ViewerConfigBuilder.kt b/tools/protologtool/src/com/android/protolog/tool/ViewerConfigBuilder.kt index c1008263c083d..175c71ff810b1 100644 --- a/tools/protologtool/src/com/android/protolog/tool/ViewerConfigBuilder.kt +++ b/tools/protologtool/src/com/android/protolog/tool/ViewerConfigBuilder.kt @@ -46,7 +46,11 @@ class ViewerConfigBuilder( private val statements: MutableMap = mutableMapOf() private val groups: MutableSet = mutableSetOf() - fun findLogCalls(unit: CompilationUnit, fileName: String): List> { + fun findLogCalls( + unit: CompilationUnit, + path: String, + packagePath: String + ): List> { val calls = mutableListOf>() val visitor = object : ProtoLogCallVisitor { override fun processCall( @@ -55,12 +59,12 @@ class ViewerConfigBuilder( level: LogLevel, group: LogGroup ) { - val logCall = LogCall(messageString, level, group, fileName) - val context = ParsingContext(fileName, call) + val logCall = LogCall(messageString, level, group, packagePath) + val context = ParsingContext(path, call) calls.add(logCall to context) } } - processor.process(unit, visitor, fileName) + processor.process(unit, visitor, path) return calls } From a8685a37725e970e28341505ba76a8f86be37c30 Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Thu, 31 Oct 2019 13:33:53 +0100 Subject: [PATCH 2/2] ProtoLog: Add end-to-end test Adds a test that verifies we compute the correct hash end-to-end, i.e. including the actual path logic, which previously caused a divergence between the viewer config and the generated code. Test: atest protologtool-tests Change-Id: Ia2a414322aa6ccdaedda7f9754d903ec984902d6 --- .../com/android/protolog/tool/ProtoLogTool.kt | 64 +++++--- .../com/android/protolog/tool/EndToEndTest.kt | 144 ++++++++++++++++++ .../protolog/tool/SourceTransformerTest.kt | 16 +- 3 files changed, 196 insertions(+), 28 deletions(-) create mode 100644 tools/protologtool/tests/com/android/protolog/tool/EndToEndTest.kt diff --git a/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt b/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt index dda13118bb219..99a26dc80288e 100644 --- a/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt +++ b/tools/protologtool/src/com/android/protolog/tool/ProtoLogTool.kt @@ -24,6 +24,7 @@ import com.github.javaparser.ast.CompilationUnit import java.io.File import java.io.FileInputStream import java.io.FileOutputStream +import java.io.OutputStream import java.util.concurrent.ExecutorService import java.util.concurrent.Executors import java.util.jar.JarOutputStream @@ -42,9 +43,10 @@ object ProtoLogTool { } private fun processClasses(command: CommandOptions) { - val groups = ProtoLogGroupReader() - .loadFromJar(command.protoLogGroupsJarArg, command.protoLogGroupsClassNameArg) - val out = FileOutputStream(command.outputSourceJarArg) + val groups = injector.readLogGroups( + command.protoLogGroupsJarArg, + command.protoLogGroupsClassNameArg) + val out = injector.fileOutputStream(command.outputSourceJarArg) val outJar = JarOutputStream(out) val processor = ProtoLogCallProcessor(command.protoLogClassNameArg, command.protoLogGroupsClassNameArg, groups) @@ -56,7 +58,7 @@ object ProtoLogTool { val transformer = SourceTransformer(command.protoLogImplClassNameArg, command.protoLogCacheClassNameArg, processor) val file = File(path) - val text = file.readText() + val text = injector.readText(file) val outSrc = try { val code = tryParse(text, path) if (containsProtoLogText(text, command.protoLogClassNameArg)) { @@ -67,7 +69,7 @@ object ProtoLogTool { } catch (ex: ParsingException) { // If we cannot parse this file, skip it (and log why). Compilation will fail // in a subsequent build step. - println("\n${ex.message}\n") + injector.reportParseError(ex) text } path to outSrc @@ -142,8 +144,9 @@ ${updates.replaceIndent(" ")} } private fun viewerConf(command: CommandOptions) { - val groups = ProtoLogGroupReader() - .loadFromJar(command.protoLogGroupsJarArg, command.protoLogGroupsClassNameArg) + val groups = injector.readLogGroups( + command.protoLogGroupsJarArg, + command.protoLogGroupsClassNameArg) val processor = ProtoLogCallProcessor(command.protoLogClassNameArg, command.protoLogGroupsClassNameArg, groups) val builder = ViewerConfigBuilder(processor) @@ -153,7 +156,7 @@ ${updates.replaceIndent(" ")} command.javaSourceArgs.map { path -> executor.submitCallable { val file = File(path) - val text = file.readText() + val text = injector.readText(file) if (containsProtoLogText(text, command.protoLogClassNameArg)) { try { val code = tryParse(text, path) @@ -161,7 +164,7 @@ ${updates.replaceIndent(" ")} } catch (ex: ParsingException) { // If we cannot parse this file, skip it (and log why). Compilation will fail // in a subsequent build step. - println("\n${ex.message}\n") + injector.reportParseError(ex) null } } else { @@ -174,7 +177,7 @@ ${updates.replaceIndent(" ")} executor.shutdown() - val out = FileOutputStream(command.viewerConfigJsonArg) + val out = injector.fileOutputStream(command.viewerConfigJsonArg) out.write(builder.build().toByteArray()) out.close() } @@ -194,18 +197,9 @@ ${updates.replaceIndent(" ")} @JvmStatic fun main(args: Array) { - StaticJavaParser.setConfiguration(ParserConfiguration().apply { - setLanguageLevel(ParserConfiguration.LanguageLevel.RAW) - setAttributeComments(false) - }) - try { val command = CommandOptions(args) - when (command.command) { - CommandOptions.TRANSFORM_CALLS_CMD -> processClasses(command) - CommandOptions.GENERATE_CONFIG_CMD -> viewerConf(command) - CommandOptions.READ_LOG_CMD -> read(command) - } + invoke(command) } catch (ex: InvalidCommandException) { println("\n${ex.message}\n") showHelpAndExit() @@ -214,6 +208,36 @@ ${updates.replaceIndent(" ")} exitProcess(1) } } + + fun invoke(command: CommandOptions) { + StaticJavaParser.setConfiguration(ParserConfiguration().apply { + setLanguageLevel(ParserConfiguration.LanguageLevel.RAW) + setAttributeComments(false) + }) + + when (command.command) { + CommandOptions.TRANSFORM_CALLS_CMD -> processClasses(command) + CommandOptions.GENERATE_CONFIG_CMD -> viewerConf(command) + CommandOptions.READ_LOG_CMD -> read(command) + } + } + + var injector = object : Injector { + override fun fileOutputStream(file: String) = FileOutputStream(file) + override fun readText(file: File) = file.readText() + override fun readLogGroups(jarPath: String, className: String) = + ProtoLogGroupReader().loadFromJar(jarPath, className) + override fun reportParseError(ex: ParsingException) { + println("\n${ex.message}\n") + } + } + + interface Injector { + fun fileOutputStream(file: String): OutputStream + fun readText(file: File): String + fun readLogGroups(jarPath: String, className: String): Map + fun reportParseError(ex: ParsingException) + } } private fun ExecutorService.submitCallable(f: () -> T) = submit(f) diff --git a/tools/protologtool/tests/com/android/protolog/tool/EndToEndTest.kt b/tools/protologtool/tests/com/android/protolog/tool/EndToEndTest.kt new file mode 100644 index 0000000000000..dd8a0b1c50b46 --- /dev/null +++ b/tools/protologtool/tests/com/android/protolog/tool/EndToEndTest.kt @@ -0,0 +1,144 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.protolog.tool + +import org.junit.Assert +import org.junit.Assert.assertTrue +import org.junit.Test +import java.io.ByteArrayInputStream +import java.io.ByteArrayOutputStream +import java.io.File +import java.io.FileNotFoundException +import java.io.OutputStream +import java.util.jar.JarInputStream + +class EndToEndTest { + + @Test + fun e2e_transform() { + val output = run( + src = "frameworks/base/org/example/Example.java" to """ + package org.example; + import com.android.server.protolog.common.ProtoLog; + import static com.android.server.wm.ProtoLogGroup.GROUP; + + class Example { + void method() { + String argString = "hello"; + int argInt = 123; + ProtoLog.d(GROUP, "Example: %s %d", argString, argInt); + } + } + """.trimIndent(), + logGroup = LogGroup("GROUP", true, false, "TAG_GROUP"), + commandOptions = CommandOptions(arrayOf("transform-protolog-calls", + "--protolog-class", "com.android.server.protolog.common.ProtoLog", + "--protolog-impl-class", "com.android.server.protolog.ProtoLogImpl", + "--protolog-cache-class", + "com.android.server.protolog.ProtoLog${"\$\$"}Cache", + "--loggroups-class", "com.android.server.wm.ProtoLogGroup", + "--loggroups-jar", "not_required.jar", + "--output-srcjar", "out.srcjar", + "frameworks/base/org/example/Example.java")) + ) + val outSrcJar = assertLoadSrcJar(output, "out.srcjar") + assertTrue(" 2066303299," in outSrcJar["frameworks/base/org/example/Example.java"]!!) + } + + @Test + fun e2e_viewerConfig() { + val output = run( + src = "frameworks/base/org/example/Example.java" to """ + package org.example; + import com.android.server.protolog.common.ProtoLog; + import static com.android.server.wm.ProtoLogGroup.GROUP; + + class Example { + void method() { + String argString = "hello"; + int argInt = 123; + ProtoLog.d(GROUP, "Example: %s %d", argString, argInt); + } + } + """.trimIndent(), + logGroup = LogGroup("GROUP", true, false, "TAG_GROUP"), + commandOptions = CommandOptions(arrayOf("generate-viewer-config", + "--protolog-class", "com.android.server.protolog.common.ProtoLog", + "--loggroups-class", "com.android.server.wm.ProtoLogGroup", + "--loggroups-jar", "not_required.jar", + "--viewer-conf", "out.json", + "frameworks/base/org/example/Example.java")) + ) + val viewerConfigJson = assertLoadText(output, "out.json") + assertTrue("\"2066303299\"" in viewerConfigJson) + } + + private fun assertLoadSrcJar( + outputs: Map, + path: String + ): Map { + val out = outputs[path] ?: fail("$path not in outputs (${outputs.keys})") + + val sources = mutableMapOf() + JarInputStream(ByteArrayInputStream(out)).use { jarStream -> + var entry = jarStream.nextJarEntry + while (entry != null) { + if (entry.name.endsWith(".java")) { + sources[entry.name] = jarStream.reader().readText() + } + entry = jarStream.nextJarEntry + } + } + return sources + } + + private fun assertLoadText(outputs: Map, path: String): String { + val out = outputs[path] ?: fail("$path not in outputs (${outputs.keys})") + return out.toString(Charsets.UTF_8) + } + + fun run( + src: Pair, + logGroup: LogGroup, + commandOptions: CommandOptions + ): Map { + val outputs = mutableMapOf() + + ProtoLogTool.injector = object : ProtoLogTool.Injector { + override fun fileOutputStream(file: String): OutputStream = + ByteArrayOutputStream().also { outputs[file] = it } + + override fun readText(file: File): String { + if (file.path == src.first) { + return src.second + } + throw FileNotFoundException("expected: ${src.first}, but was $file") + } + + override fun readLogGroups(jarPath: String, className: String) = mapOf( + logGroup.name to logGroup) + + override fun reportParseError(ex: ParsingException) = throw AssertionError(ex) + } + + ProtoLogTool.invoke(commandOptions) + + return outputs.mapValues { it.value.toByteArray() } + } + + fun fail(message: String): Nothing = Assert.fail(message) as Nothing +} diff --git a/tools/protologtool/tests/com/android/protolog/tool/SourceTransformerTest.kt b/tools/protologtool/tests/com/android/protolog/tool/SourceTransformerTest.kt index 6f5955cd030bf..4f2be328fc8ac 100644 --- a/tools/protologtool/tests/com/android/protolog/tool/SourceTransformerTest.kt +++ b/tools/protologtool/tests/com/android/protolog/tool/SourceTransformerTest.kt @@ -186,7 +186,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -228,7 +228,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE_MULTICALLS, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE_MULTICALLS, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -266,7 +266,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -303,7 +303,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE_NO_PARAMS, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE_NO_PARAMS, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -337,7 +337,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -375,7 +375,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -413,7 +413,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java) @@ -439,7 +439,7 @@ class SourceTransformerTest { invocation.arguments[0] as CompilationUnit } - val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, code) + val out = sourceJarWriter.processClass(TEST_CODE_MULTILINE, PATH, PATH, code) code = StaticJavaParser.parse(out) val ifStmts = code.findAll(IfStmt::class.java)