[PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Nick Gasson]

Hi,

These three patches fix a couple of issues I ran into while using the
jitdump JVMTI agent to profile the SPECjbb benchmark.

Tested with OpenJDK 8 and 14.

Thanks,
Nick


Nick Gasson (3):
  perf jvmti: Fix jitdump for methods without debug info
  perf jvmti: Do not report error when missing debug information
  perf jvmti: Fix demangling Java symbols

 tools/perf/jvmti/libjvmti.c           | 24 +++++++--------
 tools/perf/tests/Build                |  1 +
 tools/perf/tests/builtin-test.c       |  4 +++
 tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
 tools/perf/tests/tests.h              |  1 +
 tools/perf/util/demangle-java.c       | 13 +++++----
 6 files changed, 66 insertions(+), 19 deletions(-)
 create mode 100644 tools/perf/tests/demangle-java-test.c

-- 
2.26.1

[PATCH 1/3] perf jvmti: Fix jitdump for methods without debug info

[Nick Gasson]

If a Java class is compiled with -g:none to omit debug information, the
JVMTI plugin won't write jitdump entries for any method in this class
and prints a lot of errors like:

    java: GetSourceFileName failed with JVMTI_ERROR_ABSENT_INFORMATION

The call to GetSourceFileName is used to derive the file name `fn`, but
this value is not actually used since commit ca58d7e64bdf ("perf jvmti:
Generate correct debug information for inlined code") which moved the
file name lookup into fill_source_filenames(). So the call to
GetSourceFileName and related code can be safely removed.

Signed-off-by: Nick Gasson <nick.gasson@arm.com>
---
 tools/perf/jvmti/libjvmti.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
index c441a34cb1c0..50ef524b5cd4 100644
--- a/tools/perf/jvmti/libjvmti.c
+++ b/tools/perf/jvmti/libjvmti.c
@@ -246,8 +246,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
 	char *class_sign = NULL;
 	char *func_name = NULL;
 	char *func_sign = NULL;
-	char *file_name = NULL;
-	char fn[PATH_MAX];
 	uint64_t addr = (uint64_t)(uintptr_t)code_addr;
 	jvmtiError ret;
 	int nr_lines = 0; /* in line_tab[] */
@@ -282,12 +280,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
 		}
 	}
 
-	ret = (*jvmti)->GetSourceFileName(jvmti, decl_class, &file_name);
-	if (ret != JVMTI_ERROR_NONE) {
-		print_error(jvmti, "GetSourceFileName", ret);
-		goto error;
-	}
-
 	ret = (*jvmti)->GetClassSignature(jvmti, decl_class,
 					  &class_sign, NULL);
 	if (ret != JVMTI_ERROR_NONE) {
@@ -302,8 +294,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
 		goto error;
 	}
 
-	copy_class_filename(class_sign, file_name, fn, PATH_MAX);
-
 	/*
 	 * write source line info record if we have it
 	 */
@@ -323,7 +313,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
 	(*jvmti)->Deallocate(jvmti, (unsigned char *)func_name);
 	(*jvmti)->Deallocate(jvmti, (unsigned char *)func_sign);
 	(*jvmti)->Deallocate(jvmti, (unsigned char *)class_sign);
-	(*jvmti)->Deallocate(jvmti, (unsigned char *)file_name);
 	free(line_tab);
 	while (line_file_names && (nr_lines > 0)) {
 	    if (line_file_names[nr_lines - 1]) {
-- 
2.26.1

[PATCH 2/3] perf jvmti: Do not report error when missing debug information

[Nick Gasson]

If the Java sources are compiled with -g:none to disable debug
information the perf JVMTI plugin reports a lot of errors like:

  java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
  java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
  java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
  java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
  java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION

Instead if GetLineNumberTable returns JVMTI_ERROR_ABSENT_INFORMATION
simply skip emitting line number information for that method. Unlike the
previous patch these errors don't affect the jitdump generation, they
just generate a lot of noise.

Similarly for native methods which also don't have line tables.

Signed-off-by: Nick Gasson <nick.gasson@arm.com>
---
 tools/perf/jvmti/libjvmti.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
index 50ef524b5cd4..a9a056d68416 100644
--- a/tools/perf/jvmti/libjvmti.c
+++ b/tools/perf/jvmti/libjvmti.c
@@ -41,7 +41,11 @@ do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
 	jvmtiError ret;
 
 	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
-	if (ret != JVMTI_ERROR_NONE) {
+	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
+		/* No debug information for this method */
+		*nr = 0;
+		return JVMTI_ERROR_NONE;
+	} else if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "GetLineNumberTable", ret);
 		return ret;
 	}
@@ -93,6 +97,9 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
 					/* free what was allocated for nothing */
 					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
 					nr_total += (int)nr;
+				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
+					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
+					/* No debug information for this method */
 				} else {
 					print_error(jvmti, "GetLineNumberTable", ret);
 				}
@@ -262,7 +269,9 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
 	if (has_line_numbers && map && map_length) {
 		ret = get_line_numbers(jvmti, compile_info, &line_tab, &nr_lines);
 		if (ret != JVMTI_ERROR_NONE) {
-			warnx("jvmti: cannot get line table for method");
+			if (ret != JVMTI_ERROR_NOT_FOUND) {
+				warnx("jvmti: cannot get line table for method");
+			}
 			nr_lines = 0;
 		} else if (nr_lines > 0) {
 			line_file_names = malloc(sizeof(char*) * nr_lines);
-- 
2.26.1

[PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Nick Gasson]

For a Java method signature like:

    Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V

The demangler produces:

    void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)

The arguments should be (java.lang.String, int, int) but the demangler
interprets the "S" in String as the type code for "short". Correct this
and two other minor things:

- There is no "bool" type in Java, should be "boolean".

- The demangler prepends "class" to every Java class name. This is not
  standard Java syntax and it wastes a lot of horizontal space if the
  signature is long. Remove this as there isn't any ambiguity between
  class names and primitives.

Also added a test case.

Signed-off-by: Nick Gasson <nick.gasson@arm.com>
---
 tools/perf/tests/Build                |  1 +
 tools/perf/tests/builtin-test.c       |  4 +++
 tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
 tools/perf/tests/tests.h              |  1 +
 tools/perf/util/demangle-java.c       | 13 +++++----
 5 files changed, 55 insertions(+), 6 deletions(-)
 create mode 100644 tools/perf/tests/demangle-java-test.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 1692529639b0..2c45ac4a9581 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -55,6 +55,7 @@ perf-y += mem2node.o
 perf-y += maps.o
 perf-y += time-utils-test.o
 perf-y += genelf.o
+perf-y += demangle-java-test.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 54d9516c9839..03b362b37f97 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -309,6 +309,10 @@ static struct test generic_tests[] = {
 		.desc = "maps__merge_in",
 		.func = test__maps__merge_in,
 	},
+	{
+		.desc = "Demangle Java",
+		.func = test__demangle_java,
+	},
 	{
 		.func = NULL,
 	},
diff --git a/tools/perf/tests/demangle-java-test.c b/tools/perf/tests/demangle-java-test.c
new file mode 100644
index 000000000000..8f3b90832fb0
--- /dev/null
+++ b/tools/perf/tests/demangle-java-test.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include "tests.h"
+#include "session.h"
+#include "debug.h"
+#include "demangle-java.h"
+
+int test__demangle_java(struct test *test __maybe_unused, int subtest __maybe_unused)
+{
+	int ret = TEST_OK;
+	char *buf = NULL;
+	size_t i;
+
+	struct {
+		const char *mangled, *demangled;
+	} test_cases[] = {
+		{ "Ljava/lang/StringLatin1;equals([B[B)Z",
+		  "boolean java.lang.StringLatin1.equals(byte[], byte[])" },
+		{ "Ljava/util/zip/ZipUtils;CENSIZ([BI)J",
+		  "long java.util.zip.ZipUtils.CENSIZ(byte[], int)" },
+		{ "Ljava/util/regex/Pattern$BmpCharProperty;match(Ljava/util/regex/Matcher;ILjava/lang/CharSequence;)Z",
+		  "boolean java.util.regex.Pattern$BmpCharProperty.match(java.util.regex.Matcher, int, java.lang.CharSequence)" },
+		{ "Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V",
+		  "void java.lang.AbstractStringBuilder.appendChars(java.lang.String, int, int)" },
+		{ "Ljava/lang/Object;<init>()V",
+		  "void java.lang.Object<init>()" },
+	};
+
+	for (i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {
+		buf = java_demangle_sym(test_cases[i].mangled, 0);
+		if (strcmp(buf, test_cases[i].demangled)) {
+			pr_debug("FAILED: %s: %s != %s\n", test_cases[i].mangled,
+				 buf, test_cases[i].demangled);
+			ret = TEST_FAIL;
+		}
+		free(buf);
+	}
+
+	return ret;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 9a160fef47c9..49b791d978f6 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -111,6 +111,7 @@ int test__mem2node(struct test *t, int subtest);
 int test__maps__merge_in(struct test *t, int subtest);
 int test__time_utils(struct test *t, int subtest);
 int test__jit_write_elf(struct test *test, int subtest);
+int test__demangle_java(struct test *test, int subtest);
 
 bool test__bp_signal_is_supported(void);
 bool test__bp_account_is_supported(void);
diff --git a/tools/perf/util/demangle-java.c b/tools/perf/util/demangle-java.c
index 6fb7f34c0814..39c05200ed65 100644
--- a/tools/perf/util/demangle-java.c
+++ b/tools/perf/util/demangle-java.c
@@ -15,7 +15,7 @@ enum {
 	MODE_CLASS  = 1,
 	MODE_FUNC   = 2,
 	MODE_TYPE   = 3,
-	MODE_CTYPE  = 3, /* class arg */
+	MODE_CTYPE  = 4, /* class arg */
 };
 
 #define BASE_ENT(c, n)	[c - 'A']=n
@@ -27,7 +27,7 @@ static const char *base_types['Z' - 'A' + 1] = {
 	BASE_ENT('I', "int" ),
 	BASE_ENT('J', "long" ),
 	BASE_ENT('S', "short" ),
-	BASE_ENT('Z', "bool" ),
+	BASE_ENT('Z', "boolean" ),
 };
 
 /*
@@ -59,15 +59,16 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
 
 		switch (*q) {
 		case 'L':
-			if (mode == MODE_PREFIX || mode == MODE_CTYPE) {
-				if (mode == MODE_CTYPE) {
+			if (mode == MODE_PREFIX || mode == MODE_TYPE) {
+				if (mode == MODE_TYPE) {
 					if (narg)
 						rlen += scnprintf(buf + rlen, maxlen - rlen, ", ");
 					narg++;
 				}
-				rlen += scnprintf(buf + rlen, maxlen - rlen, "class ");
 				if (mode == MODE_PREFIX)
 					mode = MODE_CLASS;
+				else
+					mode = MODE_CTYPE;
 			} else
 				buf[rlen++] = *q;
 			break;
@@ -120,7 +121,7 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
 			if (mode != MODE_CLASS && mode != MODE_CTYPE)
 				goto error;
 			/* safe because at least one other char to process */
-			if (isalpha(*(q + 1)))
+			if (isalpha(*(q + 1)) && mode == MODE_CLASS)
 				rlen += scnprintf(buf + rlen, maxlen - rlen, ".");
 			if (mode == MODE_CLASS)
 				mode = MODE_FUNC;
-- 
2.26.1

Re: [PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Jiri Olsa]

On Mon, Apr 27, 2020 at 02:15:13PM +0800, Nick Gasson wrote:
> Hi,

adding Stephane to the loop

jirka

> 
> These three patches fix a couple of issues I ran into while using the
> jitdump JVMTI agent to profile the SPECjbb benchmark.
> 
> Tested with OpenJDK 8 and 14.
> 
> Thanks,
> Nick
> 
> 
> Nick Gasson (3):
>   perf jvmti: Fix jitdump for methods without debug info
>   perf jvmti: Do not report error when missing debug information
>   perf jvmti: Fix demangling Java symbols
> 
>  tools/perf/jvmti/libjvmti.c           | 24 +++++++--------
>  tools/perf/tests/Build                |  1 +
>  tools/perf/tests/builtin-test.c       |  4 +++
>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
>  tools/perf/tests/tests.h              |  1 +
>  tools/perf/util/demangle-java.c       | 13 +++++----
>  6 files changed, 66 insertions(+), 19 deletions(-)
>  create mode 100644 tools/perf/tests/demangle-java-test.c
> 
> -- 
> 2.26.1
> 

Re: [PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Nick Gasson]

On 04/27/20 18:35 pm, Jiri Olsa wrote:
>
> adding Stephane to the loop
>
> jirka
>
>> 
>> These three patches fix a couple of issues I ran into while using the
>> jitdump JVMTI agent to profile the SPECjbb benchmark.
>> 

Hi, any feedback on these patches?

Thanks,
Nick

>> 
>> 
>> Nick Gasson (3):
>>   perf jvmti: Fix jitdump for methods without debug info
>>   perf jvmti: Do not report error when missing debug information
>>   perf jvmti: Fix demangling Java symbols
>> 
>>  tools/perf/jvmti/libjvmti.c           | 24 +++++++--------
>>  tools/perf/tests/Build                |  1 +
>>  tools/perf/tests/builtin-test.c       |  4 +++
>>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
>>  tools/perf/tests/tests.h              |  1 +
>>  tools/perf/util/demangle-java.c       | 13 +++++----
>>  6 files changed, 66 insertions(+), 19 deletions(-)
>>  create mode 100644 tools/perf/tests/demangle-java-test.c
>> 
>> -- 
>> 2.26.1
>> 

Re: [PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Arnaldo Carvalho de Melo]

Em Thu, May 14, 2020 at 04:56:07PM +0800, Nick Gasson escreveu:
> On 04/27/20 18:35 pm, Jiri Olsa wrote:
> >
> > adding Stephane to the loop

Stephane, Ian, can you guys please take a look at this one and provide a
Reviewed-by or Acked-by?

Thanks,

- Arnaldo


> > jirka
> >
> >> 
> >> These three patches fix a couple of issues I ran into while using the
> >> jitdump JVMTI agent to profile the SPECjbb benchmark.
> >> 
> 
> Hi, any feedback on these patches?
> 
> Thanks,
> Nick
> 
> >> 
> >> 
> >> Nick Gasson (3):
> >>   perf jvmti: Fix jitdump for methods without debug info
> >>   perf jvmti: Do not report error when missing debug information
> >>   perf jvmti: Fix demangling Java symbols
> >> 
> >>  tools/perf/jvmti/libjvmti.c           | 24 +++++++--------
> >>  tools/perf/tests/Build                |  1 +
> >>  tools/perf/tests/builtin-test.c       |  4 +++
> >>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
> >>  tools/perf/tests/tests.h              |  1 +
> >>  tools/perf/util/demangle-java.c       | 13 +++++----
> >>  6 files changed, 66 insertions(+), 19 deletions(-)
> >>  create mode 100644 tools/perf/tests/demangle-java-test.c
> >> 
> >> -- 
> >> 2.26.1
> >> 
> 

-- 

- Arnaldo

Re: [PATCH 1/3] perf jvmti: Fix jitdump for methods without debug info

[Ian Rogers]

On Sun, Apr 26, 2020 at 11:16 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> If a Java class is compiled with -g:none to omit debug information, the
> JVMTI plugin won't write jitdump entries for any method in this class
> and prints a lot of errors like:
>
>     java: GetSourceFileName failed with JVMTI_ERROR_ABSENT_INFORMATION
>
> The call to GetSourceFileName is used to derive the file name `fn`, but
> this value is not actually used since commit ca58d7e64bdf ("perf jvmti:
> Generate correct debug information for inlined code") which moved the
> file name lookup into fill_source_filenames(). So the call to
> GetSourceFileName and related code can be safely removed.
>
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>

Reviewed-by: Ian Rogers <irogers@google.com>
Tested-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
>  tools/perf/jvmti/libjvmti.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index c441a34cb1c0..50ef524b5cd4 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -246,8 +246,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>         char *class_sign = NULL;
>         char *func_name = NULL;
>         char *func_sign = NULL;
> -       char *file_name = NULL;
> -       char fn[PATH_MAX];
>         uint64_t addr = (uint64_t)(uintptr_t)code_addr;
>         jvmtiError ret;
>         int nr_lines = 0; /* in line_tab[] */
> @@ -282,12 +280,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>                 }
>         }
>
> -       ret = (*jvmti)->GetSourceFileName(jvmti, decl_class, &file_name);
> -       if (ret != JVMTI_ERROR_NONE) {
> -               print_error(jvmti, "GetSourceFileName", ret);
> -               goto error;
> -       }
> -
>         ret = (*jvmti)->GetClassSignature(jvmti, decl_class,
>                                           &class_sign, NULL);
>         if (ret != JVMTI_ERROR_NONE) {
> @@ -302,8 +294,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>                 goto error;
>         }
>
> -       copy_class_filename(class_sign, file_name, fn, PATH_MAX);
> -
>         /*
>          * write source line info record if we have it
>          */
> @@ -323,7 +313,6 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>         (*jvmti)->Deallocate(jvmti, (unsigned char *)func_name);
>         (*jvmti)->Deallocate(jvmti, (unsigned char *)func_sign);
>         (*jvmti)->Deallocate(jvmti, (unsigned char *)class_sign);
> -       (*jvmti)->Deallocate(jvmti, (unsigned char *)file_name);
>         free(line_tab);
>         while (line_file_names && (nr_lines > 0)) {
>             if (line_file_names[nr_lines - 1]) {
> --
> 2.26.1
>

Re: [PATCH 2/3] perf jvmti: Do not report error when missing debug information

[Ian Rogers]

On Sun, Apr 26, 2020 at 11:16 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> If the Java sources are compiled with -g:none to disable debug
> information the perf JVMTI plugin reports a lot of errors like:
>
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>
> Instead if GetLineNumberTable returns JVMTI_ERROR_ABSENT_INFORMATION
> simply skip emitting line number information for that method. Unlike the
> previous patch these errors don't affect the jitdump generation, they
> just generate a lot of noise.
>
> Similarly for native methods which also don't have line tables.
>
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>

Reviewed-by: Ian Rogers <irogers@google.com>
Tested-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
>  tools/perf/jvmti/libjvmti.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index 50ef524b5cd4..a9a056d68416 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -41,7 +41,11 @@ do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
>         jvmtiError ret;
>
>         ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
> -       if (ret != JVMTI_ERROR_NONE) {
> +       if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
> +               /* No debug information for this method */
> +               *nr = 0;
> +               return JVMTI_ERROR_NONE;
> +       } else if (ret != JVMTI_ERROR_NONE) {
>                 print_error(jvmti, "GetLineNumberTable", ret);
>                 return ret;
>         }
> @@ -93,6 +97,9 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>                                         /* free what was allocated for nothing */
>                                         (*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
>                                         nr_total += (int)nr;
> +                               } else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
> +                                          || ret == JVMTI_ERROR_NATIVE_METHOD) {
> +                                       /* No debug information for this method */
>                                 } else {
>                                         print_error(jvmti, "GetLineNumberTable", ret);
>                                 }
> @@ -262,7 +269,9 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>         if (has_line_numbers && map && map_length) {
>                 ret = get_line_numbers(jvmti, compile_info, &line_tab, &nr_lines);
>                 if (ret != JVMTI_ERROR_NONE) {
> -                       warnx("jvmti: cannot get line table for method");
> +                       if (ret != JVMTI_ERROR_NOT_FOUND) {
> +                               warnx("jvmti: cannot get line table for method");
> +                       }
>                         nr_lines = 0;
>                 } else if (nr_lines > 0) {
>                         line_file_names = malloc(sizeof(char*) * nr_lines);
> --
> 2.26.1
>

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Ian Rogers]

On Sun, Apr 26, 2020 at 11:16 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> For a Java method signature like:
>
>     Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
>
> The demangler produces:
>
>     void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
>
> The arguments should be (java.lang.String, int, int) but the demangler
> interprets the "S" in String as the type code for "short". Correct this
> and two other minor things:
>
> - There is no "bool" type in Java, should be "boolean".
>
> - The demangler prepends "class" to every Java class name. This is not
>   standard Java syntax and it wastes a lot of horizontal space if the
>   signature is long. Remove this as there isn't any ambiguity between
>   class names and primitives.
>
> Also added a test case.
>
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>

Reviewed-by: Ian Rogers <irogers@google.com>
Tested-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
>  tools/perf/tests/Build                |  1 +
>  tools/perf/tests/builtin-test.c       |  4 +++
>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
>  tools/perf/tests/tests.h              |  1 +
>  tools/perf/util/demangle-java.c       | 13 +++++----
>  5 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100644 tools/perf/tests/demangle-java-test.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 1692529639b0..2c45ac4a9581 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -55,6 +55,7 @@ perf-y += mem2node.o
>  perf-y += maps.o
>  perf-y += time-utils-test.o
>  perf-y += genelf.o
> +perf-y += demangle-java-test.o
>
>  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>         $(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 54d9516c9839..03b362b37f97 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -309,6 +309,10 @@ static struct test generic_tests[] = {
>                 .desc = "maps__merge_in",
>                 .func = test__maps__merge_in,
>         },
> +       {
> +               .desc = "Demangle Java",
> +               .func = test__demangle_java,
> +       },
>         {
>                 .func = NULL,
>         },
> diff --git a/tools/perf/tests/demangle-java-test.c b/tools/perf/tests/demangle-java-test.c
> new file mode 100644
> index 000000000000..8f3b90832fb0
> --- /dev/null
> +++ b/tools/perf/tests/demangle-java-test.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include "tests.h"
> +#include "session.h"
> +#include "debug.h"
> +#include "demangle-java.h"
> +
> +int test__demangle_java(struct test *test __maybe_unused, int subtest __maybe_unused)
> +{
> +       int ret = TEST_OK;
> +       char *buf = NULL;
> +       size_t i;
> +
> +       struct {
> +               const char *mangled, *demangled;
> +       } test_cases[] = {
> +               { "Ljava/lang/StringLatin1;equals([B[B)Z",
> +                 "boolean java.lang.StringLatin1.equals(byte[], byte[])" },
> +               { "Ljava/util/zip/ZipUtils;CENSIZ([BI)J",
> +                 "long java.util.zip.ZipUtils.CENSIZ(byte[], int)" },
> +               { "Ljava/util/regex/Pattern$BmpCharProperty;match(Ljava/util/regex/Matcher;ILjava/lang/CharSequence;)Z",
> +                 "boolean java.util.regex.Pattern$BmpCharProperty.match(java.util.regex.Matcher, int, java.lang.CharSequence)" },
> +               { "Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V",
> +                 "void java.lang.AbstractStringBuilder.appendChars(java.lang.String, int, int)" },
> +               { "Ljava/lang/Object;<init>()V",
> +                 "void java.lang.Object<init>()" },
> +       };
> +
> +       for (i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {
> +               buf = java_demangle_sym(test_cases[i].mangled, 0);
> +               if (strcmp(buf, test_cases[i].demangled)) {
> +                       pr_debug("FAILED: %s: %s != %s\n", test_cases[i].mangled,
> +                                buf, test_cases[i].demangled);
> +                       ret = TEST_FAIL;
> +               }
> +               free(buf);
> +       }
> +
> +       return ret;
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 9a160fef47c9..49b791d978f6 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -111,6 +111,7 @@ int test__mem2node(struct test *t, int subtest);
>  int test__maps__merge_in(struct test *t, int subtest);
>  int test__time_utils(struct test *t, int subtest);
>  int test__jit_write_elf(struct test *test, int subtest);
> +int test__demangle_java(struct test *test, int subtest);
>
>  bool test__bp_signal_is_supported(void);
>  bool test__bp_account_is_supported(void);
> diff --git a/tools/perf/util/demangle-java.c b/tools/perf/util/demangle-java.c
> index 6fb7f34c0814..39c05200ed65 100644
> --- a/tools/perf/util/demangle-java.c
> +++ b/tools/perf/util/demangle-java.c
> @@ -15,7 +15,7 @@ enum {
>         MODE_CLASS  = 1,
>         MODE_FUNC   = 2,
>         MODE_TYPE   = 3,
> -       MODE_CTYPE  = 3, /* class arg */
> +       MODE_CTYPE  = 4, /* class arg */
>  };
>
>  #define BASE_ENT(c, n) [c - 'A']=n
> @@ -27,7 +27,7 @@ static const char *base_types['Z' - 'A' + 1] = {
>         BASE_ENT('I', "int" ),
>         BASE_ENT('J', "long" ),
>         BASE_ENT('S', "short" ),
> -       BASE_ENT('Z', "bool" ),
> +       BASE_ENT('Z', "boolean" ),
>  };
>
>  /*
> @@ -59,15 +59,16 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>
>                 switch (*q) {
>                 case 'L':
> -                       if (mode == MODE_PREFIX || mode == MODE_CTYPE) {
> -                               if (mode == MODE_CTYPE) {
> +                       if (mode == MODE_PREFIX || mode == MODE_TYPE) {
> +                               if (mode == MODE_TYPE) {
>                                         if (narg)
>                                                 rlen += scnprintf(buf + rlen, maxlen - rlen, ", ");
>                                         narg++;
>                                 }
> -                               rlen += scnprintf(buf + rlen, maxlen - rlen, "class ");
>                                 if (mode == MODE_PREFIX)
>                                         mode = MODE_CLASS;
> +                               else
> +                                       mode = MODE_CTYPE;
>                         } else
>                                 buf[rlen++] = *q;
>                         break;
> @@ -120,7 +121,7 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>                         if (mode != MODE_CLASS && mode != MODE_CTYPE)
>                                 goto error;
>                         /* safe because at least one other char to process */
> -                       if (isalpha(*(q + 1)))
> +                       if (isalpha(*(q + 1)) && mode == MODE_CLASS)
>                                 rlen += scnprintf(buf + rlen, maxlen - rlen, ".");
>                         if (mode == MODE_CLASS)
>                                 mode = MODE_FUNC;
> --
> 2.26.1
>

Re: [PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Ian Rogers]

On Thu, May 14, 2020 at 6:23 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, May 14, 2020 at 04:56:07PM +0800, Nick Gasson escreveu:
> > On 04/27/20 18:35 pm, Jiri Olsa wrote:
> > >
> > > adding Stephane to the loop
>
> Stephane, Ian, can you guys please take a look at this one and provide a
> Reviewed-by or Acked-by?
>
> Thanks,
>
> - Arnaldo
>
>
> > > jirka
> > >
> > >>
> > >> These three patches fix a couple of issues I ran into while using the
> > >> jitdump JVMTI agent to profile the SPECjbb benchmark.
> > >>
> >
> > Hi, any feedback on these patches?
> >
> > Thanks,
> > Nick
>

Thanks Nick,

If you are looking at this code I believe there is a bug in that the
loop handling jvmtiCompiledMethodLoadInlineRecord is writing out the
entire line number table before a pc and not just the line number
table at the pc. This loop in do_get_line_numbers:

if (loc_tab[i].start_location < bci) {
tab[lines].pc = (unsigned long)pc;
tab[lines].line_number = loc_tab[i].line_number;
tab[lines].discrim = 0; /* not yet used */
tab[lines].methodID = m;
lines++;
} else {

It could possibly make sense if it were iterating over the inline data
in the jvmtiCompiledMethodLoadInlineRecord rather than the line number
table.
Fixing this is toward the end of a list of things I need to look at.

Thanks,
Ian

> > >>
> > >>
> > >> Nick Gasson (3):
> > >>   perf jvmti: Fix jitdump for methods without debug info
> > >>   perf jvmti: Do not report error when missing debug information
> > >>   perf jvmti: Fix demangling Java symbols
> > >>
> > >>  tools/perf/jvmti/libjvmti.c           | 24 +++++++--------
> > >>  tools/perf/tests/Build                |  1 +
> > >>  tools/perf/tests/builtin-test.c       |  4 +++
> > >>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
> > >>  tools/perf/tests/tests.h              |  1 +
> > >>  tools/perf/util/demangle-java.c       | 13 +++++----
> > >>  6 files changed, 66 insertions(+), 19 deletions(-)
> > >>  create mode 100644 tools/perf/tests/demangle-java-test.c
> > >>
> > >> --
> > >> 2.26.1
> > >>
> >
>
> --
>
> - Arnaldo

Re: [PATCH 0/3] perf jvmti: Various fixes to JVMTI agent

[Nick Gasson]

On 05/15/20 06:41 am, Ian Rogers wrote:
>
> If you are looking at this code I believe there is a bug in that the
> loop handling jvmtiCompiledMethodLoadInlineRecord is writing out the
> entire line number table before a pc and not just the line number
> table at the pc. This loop in do_get_line_numbers:
>
> if (loc_tab[i].start_location < bci) {
> tab[lines].pc = (unsigned long)pc;
> tab[lines].line_number = loc_tab[i].line_number;
> tab[lines].discrim = 0; /* not yet used */
> tab[lines].methodID = m;
> lines++;
> } else {
>
> It could possibly make sense if it were iterating over the inline data
> in the jvmtiCompiledMethodLoadInlineRecord rather than the line number
> table.
> Fixing this is toward the end of a list of things I need to look at.
>

OK sure, I'll have a look at this. Thanks for the reviews.

--
Thanks,
Nick

Re: [PATCH 2/3] perf jvmti: Do not report error when missing debug information

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2020 at 02:15:15PM +0800, Nick Gasson escreveu:
> If the Java sources are compiled with -g:none to disable debug
> information the perf JVMTI plugin reports a lot of errors like:
> 
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
>   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> 
> Instead if GetLineNumberTable returns JVMTI_ERROR_ABSENT_INFORMATION
> simply skip emitting line number information for that method. Unlike the
> previous patch these errors don't affect the jitdump generation, they
> just generate a lot of noise.
> 
> Similarly for native methods which also don't have line tables.
> 
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>
> ---
>  tools/perf/jvmti/libjvmti.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index 50ef524b5cd4..a9a056d68416 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -41,7 +41,11 @@ do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
>  	jvmtiError ret;
>  
>  	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
> -	if (ret != JVMTI_ERROR_NONE) {
> +	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
> +		/* No debug information for this method */
> +		*nr = 0;
> +		return JVMTI_ERROR_NONE;
> +	} else if (ret != JVMTI_ERROR_NONE) {
>  		print_error(jvmti, "GetLineNumberTable", ret);
>  		return ret;
>  	}
> @@ -93,6 +97,9 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>  					/* free what was allocated for nothing */
>  					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
>  					nr_total += (int)nr;
> +				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
> +					   || ret == JVMTI_ERROR_NATIVE_METHOD) {

Please add the || operator at the end of the line next time, I'm fixing
this up this time,

Applied,

- Arnaldo

> +					/* No debug information for this method */
>  				} else {
>  					print_error(jvmti, "GetLineNumberTable", ret);
>  				}
> @@ -262,7 +269,9 @@ compiled_method_load_cb(jvmtiEnv *jvmti,
>  	if (has_line_numbers && map && map_length) {
>  		ret = get_line_numbers(jvmti, compile_info, &line_tab, &nr_lines);
>  		if (ret != JVMTI_ERROR_NONE) {
> -			warnx("jvmti: cannot get line table for method");
> +			if (ret != JVMTI_ERROR_NOT_FOUND) {
> +				warnx("jvmti: cannot get line table for method");
> +			}
>  			nr_lines = 0;
>  		} else if (nr_lines > 0) {
>  			line_file_names = malloc(sizeof(char*) * nr_lines);
> -- 
> 2.26.1
> 

-- 

- Arnaldo

Re: [PATCH 2/3] perf jvmti: Do not report error when missing debug information

[Arnaldo Carvalho de Melo]

Em Wed, May 27, 2020 at 11:07:53AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Apr 27, 2020 at 02:15:15PM +0800, Nick Gasson escreveu:
> > If the Java sources are compiled with -g:none to disable debug
> > information the perf JVMTI plugin reports a lot of errors like:
> > 
> >   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> >   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> >   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> >   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> >   java: GetLineNumberTable failed with JVMTI_ERROR_ABSENT_INFORMATION
> > 
> > Instead if GetLineNumberTable returns JVMTI_ERROR_ABSENT_INFORMATION
> > simply skip emitting line number information for that method. Unlike the
> > previous patch these errors don't affect the jitdump generation, they
> > just generate a lot of noise.
> > 
> > Similarly for native methods which also don't have line tables.
> > 
> > Signed-off-by: Nick Gasson <nick.gasson@arm.com>
> > ---
> >  tools/perf/jvmti/libjvmti.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> > index 50ef524b5cd4..a9a056d68416 100644
> > --- a/tools/perf/jvmti/libjvmti.c
> > +++ b/tools/perf/jvmti/libjvmti.c
> > @@ -41,7 +41,11 @@ do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
> >  	jvmtiError ret;
> >  
> >  	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
> > -	if (ret != JVMTI_ERROR_NONE) {
> > +	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
> > +		/* No debug information for this method */
> > +		*nr = 0;
> > +		return JVMTI_ERROR_NONE;
> > +	} else if (ret != JVMTI_ERROR_NONE) {
> >  		print_error(jvmti, "GetLineNumberTable", ret);
> >  		return ret;
> >  	}
> > @@ -93,6 +97,9 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
> >  					/* free what was allocated for nothing */
> >  					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
> >  					nr_total += (int)nr;
> > +				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
> > +					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
> 
> Please add the || operator at the end of the line next time, I'm fixing
> this up this time,
> 
> Applied,

This:

diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
index a9a056d68416..c5d30834a64c 100644
--- a/tools/perf/jvmti/libjvmti.c
+++ b/tools/perf/jvmti/libjvmti.c
@@ -97,8 +97,8 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
 					/* free what was allocated for nothing */
 					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
 					nr_total += (int)nr;
-				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
-					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
+				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION ||
+					   ret == JVMTI_ERROR_NATIVE_METHOD) {
 					/* No debug information for this method */
 				} else {
 					print_error(jvmti, "GetLineNumberTable", ret);

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2020 at 02:15:16PM +0800, Nick Gasson escreveu:
> For a Java method signature like:
> 
>     Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
> 
> The demangler produces:
> 
>     void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)

Cool, one more 'perf test' entry, thanks a lot!

Applied,

- Arnaldo
 
> The arguments should be (java.lang.String, int, int) but the demangler
> interprets the "S" in String as the type code for "short". Correct this
> and two other minor things:
> 
> - There is no "bool" type in Java, should be "boolean".
> 
> - The demangler prepends "class" to every Java class name. This is not
>   standard Java syntax and it wastes a lot of horizontal space if the
>   signature is long. Remove this as there isn't any ambiguity between
>   class names and primitives.
> 
> Also added a test case.
> 
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>
> ---
>  tools/perf/tests/Build                |  1 +
>  tools/perf/tests/builtin-test.c       |  4 +++
>  tools/perf/tests/demangle-java-test.c | 42 +++++++++++++++++++++++++++
>  tools/perf/tests/tests.h              |  1 +
>  tools/perf/util/demangle-java.c       | 13 +++++----
>  5 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100644 tools/perf/tests/demangle-java-test.c
> 
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 1692529639b0..2c45ac4a9581 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -55,6 +55,7 @@ perf-y += mem2node.o
>  perf-y += maps.o
>  perf-y += time-utils-test.o
>  perf-y += genelf.o
> +perf-y += demangle-java-test.o
>  
>  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>  	$(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 54d9516c9839..03b362b37f97 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -309,6 +309,10 @@ static struct test generic_tests[] = {
>  		.desc = "maps__merge_in",
>  		.func = test__maps__merge_in,
>  	},
> +	{
> +		.desc = "Demangle Java",
> +		.func = test__demangle_java,
> +	},
>  	{
>  		.func = NULL,
>  	},
> diff --git a/tools/perf/tests/demangle-java-test.c b/tools/perf/tests/demangle-java-test.c
> new file mode 100644
> index 000000000000..8f3b90832fb0
> --- /dev/null
> +++ b/tools/perf/tests/demangle-java-test.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include "tests.h"
> +#include "session.h"
> +#include "debug.h"
> +#include "demangle-java.h"
> +
> +int test__demangle_java(struct test *test __maybe_unused, int subtest __maybe_unused)
> +{
> +	int ret = TEST_OK;
> +	char *buf = NULL;
> +	size_t i;
> +
> +	struct {
> +		const char *mangled, *demangled;
> +	} test_cases[] = {
> +		{ "Ljava/lang/StringLatin1;equals([B[B)Z",
> +		  "boolean java.lang.StringLatin1.equals(byte[], byte[])" },
> +		{ "Ljava/util/zip/ZipUtils;CENSIZ([BI)J",
> +		  "long java.util.zip.ZipUtils.CENSIZ(byte[], int)" },
> +		{ "Ljava/util/regex/Pattern$BmpCharProperty;match(Ljava/util/regex/Matcher;ILjava/lang/CharSequence;)Z",
> +		  "boolean java.util.regex.Pattern$BmpCharProperty.match(java.util.regex.Matcher, int, java.lang.CharSequence)" },
> +		{ "Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V",
> +		  "void java.lang.AbstractStringBuilder.appendChars(java.lang.String, int, int)" },
> +		{ "Ljava/lang/Object;<init>()V",
> +		  "void java.lang.Object<init>()" },
> +	};
> +
> +	for (i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {
> +		buf = java_demangle_sym(test_cases[i].mangled, 0);
> +		if (strcmp(buf, test_cases[i].demangled)) {
> +			pr_debug("FAILED: %s: %s != %s\n", test_cases[i].mangled,
> +				 buf, test_cases[i].demangled);
> +			ret = TEST_FAIL;
> +		}
> +		free(buf);
> +	}
> +
> +	return ret;
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 9a160fef47c9..49b791d978f6 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -111,6 +111,7 @@ int test__mem2node(struct test *t, int subtest);
>  int test__maps__merge_in(struct test *t, int subtest);
>  int test__time_utils(struct test *t, int subtest);
>  int test__jit_write_elf(struct test *test, int subtest);
> +int test__demangle_java(struct test *test, int subtest);
>  
>  bool test__bp_signal_is_supported(void);
>  bool test__bp_account_is_supported(void);
> diff --git a/tools/perf/util/demangle-java.c b/tools/perf/util/demangle-java.c
> index 6fb7f34c0814..39c05200ed65 100644
> --- a/tools/perf/util/demangle-java.c
> +++ b/tools/perf/util/demangle-java.c
> @@ -15,7 +15,7 @@ enum {
>  	MODE_CLASS  = 1,
>  	MODE_FUNC   = 2,
>  	MODE_TYPE   = 3,
> -	MODE_CTYPE  = 3, /* class arg */
> +	MODE_CTYPE  = 4, /* class arg */
>  };
>  
>  #define BASE_ENT(c, n)	[c - 'A']=n
> @@ -27,7 +27,7 @@ static const char *base_types['Z' - 'A' + 1] = {
>  	BASE_ENT('I', "int" ),
>  	BASE_ENT('J', "long" ),
>  	BASE_ENT('S', "short" ),
> -	BASE_ENT('Z', "bool" ),
> +	BASE_ENT('Z', "boolean" ),
>  };
>  
>  /*
> @@ -59,15 +59,16 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>  
>  		switch (*q) {
>  		case 'L':
> -			if (mode == MODE_PREFIX || mode == MODE_CTYPE) {
> -				if (mode == MODE_CTYPE) {
> +			if (mode == MODE_PREFIX || mode == MODE_TYPE) {
> +				if (mode == MODE_TYPE) {
>  					if (narg)
>  						rlen += scnprintf(buf + rlen, maxlen - rlen, ", ");
>  					narg++;
>  				}
> -				rlen += scnprintf(buf + rlen, maxlen - rlen, "class ");
>  				if (mode == MODE_PREFIX)
>  					mode = MODE_CLASS;
> +				else
> +					mode = MODE_CTYPE;
>  			} else
>  				buf[rlen++] = *q;
>  			break;
> @@ -120,7 +121,7 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>  			if (mode != MODE_CLASS && mode != MODE_CTYPE)
>  				goto error;
>  			/* safe because at least one other char to process */
> -			if (isalpha(*(q + 1)))
> +			if (isalpha(*(q + 1)) && mode == MODE_CLASS)
>  				rlen += scnprintf(buf + rlen, maxlen - rlen, ".");
>  			if (mode == MODE_CLASS)
>  				mode = MODE_FUNC;
> -- 
> 2.26.1
> 

-- 

- Arnaldo

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2020 at 02:15:16PM +0800, Nick Gasson escreveu:
> For a Java method signature like:
> 
>     Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
> 
> The demangler produces:
> 
>     void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
> 
> The arguments should be (java.lang.String, int, int) but the demangler
> interprets the "S" in String as the type code for "short". Correct this
> and two other minor things:
> 
> - There is no "bool" type in Java, should be "boolean".
> 
> - The demangler prepends "class" to every Java class name. This is not
>   standard Java syntax and it wastes a lot of horizontal space if the
>   signature is long. Remove this as there isn't any ambiguity between
>   class names and primitives.
> 
> Also added a test case.

So, I took this and split into a patch for the new 'perf test java' and
then the fix, so that we can see the problem being detected and then
apply the fix and see it fixed, the last patch in this series thus
became:


commit 341e11c1d445999932da3f5d626c9fe096949ae3
Author: Nick Gasson <nick.gasson@arm.com>
Date:   Mon Apr 27 14:15:16 2020 +0800

    perf jvmti: Fix demangling Java symbols
    
    For a Java method signature like:
    
        Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
    
    The demangler produces:
    
        void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
    
    The arguments should be (java.lang.String, int, int) but the demangler
    interprets the "S" in String as the type code for "short". Correct this
    and two other minor things:
    
    - There is no "bool" type in Java, should be "boolean".
    
    - The demangler prepends "class" to every Java class name. This is not
      standard Java syntax and it wastes a lot of horizontal space if the
      signature is long. Remove this as there isn't any ambiguity between
      class names and primitives.
    
    Committer notes:
    
    This was split from a larger patch that also added a java demangler
    'perf test' entry, that, before this patch shows the error being fixed
    by it:
    
      $ perf test java
      65: Demangle Java                                         : FAILED!
      $ perf test -v java
      Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc
      65: Demangle Java                                         :
      --- start ---
      test child forked, pid 307264
      FAILED: Ljava/lang/StringLatin1;equals([B[B)Z: bool class java.lang.StringLatin1.equals(byte[], byte[]) != boolean java.lang.StringLatin1.equals(byte[], byte[])
      FAILED: Ljava/util/zip/ZipUtils;CENSIZ([BI)J: long class java.util.zip.ZipUtils.CENSIZ(byte[], int) != long java.util.zip.ZipUtils.CENSIZ(byte[], int)
      FAILED: Ljava/util/regex/Pattern$BmpCharProperty;match(Ljava/util/regex/Matcher;ILjava/lang/CharSequence;)Z: bool class java.util.regex.Pattern$BmpCharProperty.match(class java.util.regex.Matcher., int, class java.lang., charhar, shortequence) != boolean java.util.regex.Pattern$BmpCharProperty.match(java.util.regex.Matcher, int, java.lang.CharSequence)
      FAILED: Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V: void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int) != void java.lang.AbstractStringBuilder.appendChars(java.lang.String, int, int)
      FAILED: Ljava/lang/Object;<init>()V: void class java.lang.Object<init>() != void java.lang.Object<init>()
      test child finished with -1
      ---- end ----
      Demangle Java: FAILED!
      $
    
    After applying this patch:
    
      $ perf test  java
      65: Demangle Java                                         : Ok
      $
    
    Signed-off-by: Nick Gasson <nick.gasson@arm.com>
    Reviewed-by: Ian Rogers <irogers@google.com>
    Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Tested-by: Ian Rogers <irogers@google.com>
    Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Mark Rutland <mark.rutland@arm.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Link: http://lore.kernel.org/lkml/20200427061520.24905-4-nick.gasson@arm.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/demangle-java.c b/tools/perf/util/demangle-java.c
index 6fb7f34c0814..39c05200ed65 100644
--- a/tools/perf/util/demangle-java.c
+++ b/tools/perf/util/demangle-java.c
@@ -15,7 +15,7 @@ enum {
 	MODE_CLASS  = 1,
 	MODE_FUNC   = 2,
 	MODE_TYPE   = 3,
-	MODE_CTYPE  = 3, /* class arg */
+	MODE_CTYPE  = 4, /* class arg */
 };
 
 #define BASE_ENT(c, n)	[c - 'A']=n
@@ -27,7 +27,7 @@ static const char *base_types['Z' - 'A' + 1] = {
 	BASE_ENT('I', "int" ),
 	BASE_ENT('J', "long" ),
 	BASE_ENT('S', "short" ),
-	BASE_ENT('Z', "bool" ),
+	BASE_ENT('Z', "boolean" ),
 };
 
 /*
@@ -59,15 +59,16 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
 
 		switch (*q) {
 		case 'L':
-			if (mode == MODE_PREFIX || mode == MODE_CTYPE) {
-				if (mode == MODE_CTYPE) {
+			if (mode == MODE_PREFIX || mode == MODE_TYPE) {
+				if (mode == MODE_TYPE) {
 					if (narg)
 						rlen += scnprintf(buf + rlen, maxlen - rlen, ", ");
 					narg++;
 				}
-				rlen += scnprintf(buf + rlen, maxlen - rlen, "class ");
 				if (mode == MODE_PREFIX)
 					mode = MODE_CLASS;
+				else
+					mode = MODE_CTYPE;
 			} else
 				buf[rlen++] = *q;
 			break;
@@ -120,7 +121,7 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
 			if (mode != MODE_CLASS && mode != MODE_CTYPE)
 				goto error;
 			/* safe because at least one other char to process */
-			if (isalpha(*(q + 1)))
+			if (isalpha(*(q + 1)) && mode == MODE_CLASS)
 				rlen += scnprintf(buf + rlen, maxlen - rlen, ".");
 			if (mode == MODE_CLASS)
 				mode = MODE_FUNC;

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Arnaldo Carvalho de Melo]

Em Wed, May 27, 2020 at 11:20:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Apr 27, 2020 at 02:15:16PM +0800, Nick Gasson escreveu:
> > For a Java method signature like:
> > 
> >     Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
> > 
> > The demangler produces:
> > 
> >     void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
> > 
> > The arguments should be (java.lang.String, int, int) but the demangler
> > interprets the "S" in String as the type code for "short". Correct this
> > and two other minor things:
> > 
> > - There is no "bool" type in Java, should be "boolean".
> > 
> > - The demangler prepends "class" to every Java class name. This is not
> >   standard Java syntax and it wastes a lot of horizontal space if the
> >   signature is long. Remove this as there isn't any ambiguity between
> >   class names and primitives.
> > 
> > Also added a test case.
> 
> So, I took this and split into a patch for the new 'perf test java' and
> then the fix, so that we can see the problem being detected and then
> apply the fix and see it fixed, the last patch in this series thus
> became:

This is in my tmp.perf/core branch pending a round of testing, after
that it'll move to perf/core on its way to 5.8, thanks.

- Arnaldo
 
> 
> commit 341e11c1d445999932da3f5d626c9fe096949ae3
> Author: Nick Gasson <nick.gasson@arm.com>
> Date:   Mon Apr 27 14:15:16 2020 +0800
> 
>     perf jvmti: Fix demangling Java symbols
>     
>     For a Java method signature like:
>     
>         Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
>     
>     The demangler produces:
>     
>         void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
>     
>     The arguments should be (java.lang.String, int, int) but the demangler
>     interprets the "S" in String as the type code for "short". Correct this
>     and two other minor things:
>     
>     - There is no "bool" type in Java, should be "boolean".
>     
>     - The demangler prepends "class" to every Java class name. This is not
>       standard Java syntax and it wastes a lot of horizontal space if the
>       signature is long. Remove this as there isn't any ambiguity between
>       class names and primitives.
>     
>     Committer notes:
>     
>     This was split from a larger patch that also added a java demangler
>     'perf test' entry, that, before this patch shows the error being fixed
>     by it:
>     
>       $ perf test java
>       65: Demangle Java                                         : FAILED!
>       $ perf test -v java
>       Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc
>       65: Demangle Java                                         :
>       --- start ---
>       test child forked, pid 307264
>       FAILED: Ljava/lang/StringLatin1;equals([B[B)Z: bool class java.lang.StringLatin1.equals(byte[], byte[]) != boolean java.lang.StringLatin1.equals(byte[], byte[])
>       FAILED: Ljava/util/zip/ZipUtils;CENSIZ([BI)J: long class java.util.zip.ZipUtils.CENSIZ(byte[], int) != long java.util.zip.ZipUtils.CENSIZ(byte[], int)
>       FAILED: Ljava/util/regex/Pattern$BmpCharProperty;match(Ljava/util/regex/Matcher;ILjava/lang/CharSequence;)Z: bool class java.util.regex.Pattern$BmpCharProperty.match(class java.util.regex.Matcher., int, class java.lang., charhar, shortequence) != boolean java.util.regex.Pattern$BmpCharProperty.match(java.util.regex.Matcher, int, java.lang.CharSequence)
>       FAILED: Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V: void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int) != void java.lang.AbstractStringBuilder.appendChars(java.lang.String, int, int)
>       FAILED: Ljava/lang/Object;<init>()V: void class java.lang.Object<init>() != void java.lang.Object<init>()
>       test child finished with -1
>       ---- end ----
>       Demangle Java: FAILED!
>       $
>     
>     After applying this patch:
>     
>       $ perf test  java
>       65: Demangle Java                                         : Ok
>       $
>     
>     Signed-off-by: Nick Gasson <nick.gasson@arm.com>
>     Reviewed-by: Ian Rogers <irogers@google.com>
>     Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Tested-by: Ian Rogers <irogers@google.com>
>     Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>     Cc: Jiri Olsa <jolsa@redhat.com>
>     Cc: Mark Rutland <mark.rutland@arm.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Link: http://lore.kernel.org/lkml/20200427061520.24905-4-nick.gasson@arm.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/perf/util/demangle-java.c b/tools/perf/util/demangle-java.c
> index 6fb7f34c0814..39c05200ed65 100644
> --- a/tools/perf/util/demangle-java.c
> +++ b/tools/perf/util/demangle-java.c
> @@ -15,7 +15,7 @@ enum {
>  	MODE_CLASS  = 1,
>  	MODE_FUNC   = 2,
>  	MODE_TYPE   = 3,
> -	MODE_CTYPE  = 3, /* class arg */
> +	MODE_CTYPE  = 4, /* class arg */
>  };
>  
>  #define BASE_ENT(c, n)	[c - 'A']=n
> @@ -27,7 +27,7 @@ static const char *base_types['Z' - 'A' + 1] = {
>  	BASE_ENT('I', "int" ),
>  	BASE_ENT('J', "long" ),
>  	BASE_ENT('S', "short" ),
> -	BASE_ENT('Z', "bool" ),
> +	BASE_ENT('Z', "boolean" ),
>  };
>  
>  /*
> @@ -59,15 +59,16 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>  
>  		switch (*q) {
>  		case 'L':
> -			if (mode == MODE_PREFIX || mode == MODE_CTYPE) {
> -				if (mode == MODE_CTYPE) {
> +			if (mode == MODE_PREFIX || mode == MODE_TYPE) {
> +				if (mode == MODE_TYPE) {
>  					if (narg)
>  						rlen += scnprintf(buf + rlen, maxlen - rlen, ", ");
>  					narg++;
>  				}
> -				rlen += scnprintf(buf + rlen, maxlen - rlen, "class ");
>  				if (mode == MODE_PREFIX)
>  					mode = MODE_CLASS;
> +				else
> +					mode = MODE_CTYPE;
>  			} else
>  				buf[rlen++] = *q;
>  			break;
> @@ -120,7 +121,7 @@ __demangle_java_sym(const char *str, const char *end, char *buf, int maxlen, int
>  			if (mode != MODE_CLASS && mode != MODE_CTYPE)
>  				goto error;
>  			/* safe because at least one other char to process */
> -			if (isalpha(*(q + 1)))
> +			if (isalpha(*(q + 1)) && mode == MODE_CLASS)
>  				rlen += scnprintf(buf + rlen, maxlen - rlen, ".");
>  			if (mode == MODE_CLASS)
>  				mode = MODE_FUNC;

-- 

- Arnaldo

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Arnaldo Carvalho de Melo]

Em Wed, May 27, 2020 at 01:23:00PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, May 27, 2020 at 11:20:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Apr 27, 2020 at 02:15:16PM +0800, Nick Gasson escreveu:
> > > For a Java method signature like:
> > > 
> > >     Ljava/lang/AbstractStringBuilder;appendChars(Ljava/lang/String;II)V
> > > 
> > > The demangler produces:
> > > 
> > >     void class java.lang.AbstractStringBuilder.appendChars(class java.lang., shorttring., int, int)
> > > 
> > > The arguments should be (java.lang.String, int, int) but the demangler
> > > interprets the "S" in String as the type code for "short". Correct this
> > > and two other minor things:
> > > 
> > > - There is no "bool" type in Java, should be "boolean".
> > > 
> > > - The demangler prepends "class" to every Java class name. This is not
> > >   standard Java syntax and it wastes a lot of horizontal space if the
> > >   signature is long. Remove this as there isn't any ambiguity between
> > >   class names and primitives.
> > > 
> > > Also added a test case.
> > 
> > So, I took this and split into a patch for the new 'perf test java' and
> > then the fix, so that we can see the problem being detected and then
> > apply the fix and see it fixed, the last patch in this series thus
> > became:
> 
> This is in my tmp.perf/core branch pending a round of testing, after
> that it'll move to perf/core on its way to 5.8, thanks.

All tests passed, moved to perf/core.

- Arnaldo

Re: [PATCH 3/3] perf jvmti: Fix demangling Java symbols

[Nick Gasson]

On 05/28/20 06:34 AM, Arnaldo Carvalho de Melo wrote:
>> 
>> This is in my tmp.perf/core branch pending a round of testing, after
>> that it'll move to perf/core on its way to 5.8, thanks.
>
> All tests passed, moved to perf/core.
>

Great, thank you!

--
Nick