ARM: Rewrite disassembler to use hsdis
Xerxes Rånby
xerxes at zafena.se
Mon Jan 23 05:00:34 PST 2012
2012-01-20 16:50, Andrew Haley skrev:
> This patch rewrites the ARM disassembler to use HSDIS rather than call
> binutils directly. This removes the build dependency on
> binutils-devel. We have to do this because binutils is GPLv3+ and
> HotSpot is GPLv2 only. I had to patch HSDIS itself to make it work on
> ARM; I'll send that patch upstream as well as to IcedTea.
minor note: doko pointed out that you may compile and link the code as is using an old gplv2 licensed binutils-dev.
>
> I also took the opportunity to use the global PrintAssembly setting,
> but the old T2EE_PRINT_DISASS environment variable stil works if you
> want to use that.
>
> This is for trunk and branch.
>
> Andrew.
>
>
Thank you for bringing the asm port closer to regular hotspot implementation.
I like this patch in large, some minor changes needed, comments inline.
When those are fixed, ok for inclusion into icedtea6 head and 1.11 release branch.
>
> diff -r 8c63d311e066 Makefile.am
> --- a/Makefile.am Thu Jan 19 07:16:03 2012 -0500
> +++ b/Makefile.am Fri Jan 20 10:43:58 2012 -0500
> @@ -399,6 +399,7 @@
> patches/openjdk/6296893-BMP_Writer_handles_TopDown_prop_incorrectly.patch \
> patches/openjdk/7103224-glibc_name_collision.patch \
> patches/arm-debug.patch \
> + patches/arm-hsdis.patch \
> patches/openjdk/7103610-_NET_WM_PID_and_WM_CLIENT_MACHINE_are_not_set.patch \
> patches/openjdk/7102369-7094468-rmiregistry.patch \
> patches/openjdk/6851973-kerberos.patch \
> diff -r 8c63d311e066 arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp
> --- a/arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp Thu Jan 19 07:16:03 2012 -0500
> +++ b/arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp Fri Jan 20 10:43:58 2012 -0500
> @@ -38,10 +38,6 @@
> static char *t2ee_print_statistics;
> #endif
>
> -#ifdef T2EE_PRINT_DISASS
> -static char *t2ee_print_disass;
> -#endif
> -
> #ifdef T2EE_PRINT_REGUSAGE
> static char *t2ee_print_regusage;
> #endif
> @@ -56,11 +52,6 @@
> #include "precompiled.hpp"
> #include "interpreter/bytecodes.hpp"
>
> -#ifdef T2EE_PRINT_DISASS
> -#include "dis-asm.h"
> -#include "bfd.h"
> -#endif
> -
> #define opc_nop 0x00
> #define opc_aconst_null 0x01
> #define opc_iconst_m1 0x02
> @@ -725,41 +716,44 @@
>
> #ifdef T2EE_PRINT_DISASS
>
> -class Opcodes {
> +class Hsdis {
> public:
> - typeof (::print_insn_little_arm) *print_insn_little_arm;
> - typeof (::init_disassemble_info) *init_disassemble_info;
> - typeof (::disassemble_init_for_target) *disassemble_init_for_target;
> -
> - // Load libopcodes.so lazily.
> - Opcodes()
> +
> + typedef void* (*decode_instructions_event_callback_ftype) (void*, const char*, void*);
> +
> + typedef void* (*decode_instructions_ftype)
> + (void* start, void* end,
> + decode_instructions_event_callback_ftype event_callback,
> + void* event_stream,
> + void* printf_callback,
> + void* printf_stream,
> + const char* options);
> +
> + decode_instructions_ftype decode_instructions;
> +
> + void *lib;
> +
> + // Load hsdis-arm.so lazily.
> + Hsdis()
> {
> - void *lib;
> - if (t2ee_print_disass) {
> - if (lib = dlopen("libopcodes.so", RTLD_NOW)) {
> - print_insn_little_arm
> - = (typeof print_insn_little_arm)dlsym(lib, "print_insn_little_arm");
> - init_disassemble_info
> - = (typeof init_disassemble_info)dlsym(lib, "init_disassemble_info");
> - disassemble_init_for_target
> - = (typeof disassemble_init_for_target)dlsym(lib, "disassemble_init_for_target");
> + if (PrintAssembly) {
> + if (lib = dlopen("hsdis-arm.so", RTLD_NOW)) {
> + decode_instructions
> + = (typeof decode_instructions)dlsym(lib, "decode_instructions");
> }
>
> - if (! (print_insn_little_arm
> - && init_disassemble_info
> - && disassemble_init_for_target))
> - {
> - fprintf (stderr, "The environment variable T2EE_PRINT_DISASS is set, but\n"
> - "libopcodes.so has not been found or is invalid. If you want to\n"
> - "see a disassembly, please ensure that a valid copy of\n"
> - "libopcodes.so is present somewhere in your library load path.\n");
> - abort();
> - }
> + if (! (decode_instructions)) {
> + fprintf (stderr, "PrintAssembly (or T2EE_PRINT_DISASS) is set, but\n"
> + "hsdis-arm.so has not been found or is invalid. If you want to\n"
> + "see a disassembly, please ensure that a valid copy of\n"
> + "hsdis-arm.so is present somewhere in your library load path.\n");
> + abort();
> + }
> }
> }
> };
>
> -static Opcodes opcodes;
> +static void *print_address(void *stream, const char *tag, void *data);
>
> void Thumb2_disass(Thumb2_Info *jinfo)
> {
> @@ -773,10 +767,11 @@
> int start_b, end_b;
> unsigned nodisass;
>
> - struct disassemble_info info;
> unsigned short *codebuf = jinfo->codebuf->codebuf;
> unsigned idx, compiled_len;
>
> + static Hsdis hsdis;
> +
> #if 0
> printf("Local Variable Usage\n");
> printf("====================\n");
> @@ -794,16 +789,6 @@
> fflush(stdout);
> fflush(stderr);
>
> - opcodes.init_disassemble_info(&info, stdout, (fprintf_ftype)fprintf);
> - info.arch = bfd_arch_arm;
> - opcodes.disassemble_init_for_target(&info);
> - info.endian = BFD_ENDIAN_LITTLE;
> - info.endian_code = BFD_ENDIAN_LITTLE;
> - info.buffer = (bfd_byte *)codebuf;
> - info.buffer_vma = (bfd_vma)codebuf;
> - info.buffer_length = jinfo->codebuf->idx * sizeof(short);
> - info.disassembler_options = (char *)"force-thumb";
> -
> compiled_len = jinfo->codebuf->idx * 2;
> for (idx = 0; idx < compiled_len; ) {
> nodisass = 0;
ok
> @@ -867,13 +852,14 @@
> low++;
> }
> bci += len;
> - for (i = 0; i < 4; i++) {
> + {
> printf("0x%08x:\t", (int)codebuf+idx);
> {
> - int len = opcodes.print_insn_little_arm((bfd_vma)codebuf+idx, &info);
> - if (len == -1) len = 2;
> - idx += len;
> - putchar('\n');
> + unsigned short *p = codebuf + idx/2;
> + hsdis.decode_instructions((char*)p, (char *)p + 14,
> + print_address, NULL, NULL, stdout,
> + "force-thumb");
> + idx += 14;
> }
> }
> for (i = 0; i < n; i++) {
Please add a comment that reflect the magic number 14.
(11.33.07) xranby: aph: why the magic number 14 ?
(11.39.20) aph: xranby: cos that's how long the instruction sequence is. I haven't changed that.
Also one level of {} can get removed in this block.
(11.22.30) xranby: a quick question.. of this block http://paste.ubuntu.com/814137/ why are the { } still needed?
(11.23.00) xranby: line 6 and line 19
(11.23.17) aph: xranby: there's a declaration in there
(11.23.50) xranby: ok then line 8 and line 18 are still needed
(11.23.53) aph: xranby: OH, ISWYM
(11.24.18) aph: sorry, I guess there's no need any more. I try to keep diffs to a minimum.
(11.24.45) aph: at least one pair of those "{" can go
> @@ -929,7 +915,6 @@
> }
> }
> if (!nodisass) {
> - printf("0x%08x:\t", (int)codebuf+idx);
> {
> int len;
> unsigned s1, s2;
> @@ -945,16 +930,26 @@
> len = 4;
> }
> } else {
> - len = opcodes.print_insn_little_arm((bfd_vma)codebuf+idx, &info);
> - if (len == -1) len = 2;
> - idx += len;
> + char *p = (char*)codebuf + idx;
> + len = 2;
> + while (len + idx < compiled_len
> + && start_bci[(len + idx)/2] == -1)
> + len += 2;
> + hsdis.decode_instructions((char*)p, (char*)p + len,
> + print_address, NULL, NULL, stdout,
> + "force-thumb");
> }
> - putchar('\n');
> + idx += len;
> }
> }
> }
> fflush(stdout);
> }
> +// where
> +static void *print_address(void *, const char *tag, void *data) {
> + if (strcmp(tag, "insn") == 0)
> + printf("0x%08x:\t", data);
> +}
ok, I have seen your updated patch for this block on the list that return NULL.
> #endif
>
> #define BCI(len, pop, push, special, islocal, islocal_n, isstore, local_n, local_type) \
> @@ -7128,7 +7123,7 @@
> #endif
>
> #ifdef T2EE_PRINT_COMPILATION
> - if (t2ee_print_compilation) {
> + if (t2ee_print_compilation || PrintAssembly) {
> fprintf(stderr, "Compiling %d %c%c %s\n",
> compiled_methods,
> method->is_synchronized() ? 'S' : ' ',
> @@ -7205,7 +7200,7 @@
>
> #ifdef T2EE_PRINT_DISASS
> if (DISASS_AFTER == 0 || compiled_methods >= DISASS_AFTER)
> - if (t2ee_print_disass)
> + if (PrintAssembly)
> Thumb2_disass(&jinfo_str);
> #endif
>
> @@ -7380,7 +7375,7 @@
> t2ee_print_statistics = getenv("T2EE_PRINT_STATISTICS");
> #endif
> #ifdef T2EE_PRINT_DISASS
> - t2ee_print_disass = getenv("T2EE_PRINT_DISASS");
> + PrintAssembly |= getenv("T2EE_PRINT_DISASS") != NULL;
> #endif
> #ifdef T2EE_PRINT_REGUSAGE
> t2ee_print_regusage = getenv("T2EE_PRINT_REGUSAGE");
> @@ -7772,33 +7767,19 @@
> mov_imm(&codebuf, ARM_R3, (u32)Thumb2_Handle_Exception_NoRegs);
> mov_reg(&codebuf, ARM_PC, ARM_R3);
>
> + // Disassemble the codebuf we just created. For debugging
> + if (PrintAssembly) {
> + Hsdis hsdis;
> + hsdis.decode_instructions(cb->hp, cb->hp + codebuf.idx * 2,
> + print_address, NULL, NULL, stdout,
> + "force-thumb");
> + putchar('\n');
> + }
> +
> Thumb2_Clear_Cache(cb->hp, cb->hp + codebuf.idx * 2);
> cb->hp += codebuf.idx * 2;
>
> thumb2_codebuf = cb;
> -
> -#if 0 // Disassemble the codebuf we just created. For debugging
> - Opcodes opcodes;
> - if (t2ee_print_disass) {
> - struct disassemble_info info;
> - info.arch = bfd_arch_arm;
> - opcodes.disassemble_init_for_target(&info);
> - opcodes.init_disassemble_info(&info, stdout, (fprintf_ftype)fprintf);
> - info.endian = BFD_ENDIAN_LITTLE;
> - info.endian_code = BFD_ENDIAN_LITTLE;
> - info.buffer = (bfd_byte *)codebuf.codebuf;
> - info.buffer_vma = (bfd_vma)codebuf.codebuf;
> - info.buffer_length = codebuf.idx * sizeof(short);
> - info.disassembler_options = (char *)"force-thumb";
> - int len;
> - for (unsigned int i = 0; i < codebuf.idx * sizeof(short); i += len) {
> - printf("0x%08x:\t", ((int)codebuf.codebuf)+i);
> - len = opcodes.print_insn_little_arm(((bfd_vma)codebuf.codebuf)+i, &info);
> - if (len == -1) len = 2;
> - putchar('\n');
> - }
> - }
> -#endif
> }
>
> #endif // THUMB2EE
ok
> diff -r 8c63d311e066 patches/arm-hsdis.patch
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/patches/arm-hsdis.patch Fri Jan 20 10:43:58 2012 -0500
> @@ -0,0 +1,90 @@
> +2012-01-20 Andrew Haley <aph at redhat.com>
> +
> + * Makefile (ARCH): Add entry for armv7l.
> + (CFLAGS): Enable debugging.
> + ($(LIBRARIES)): Pass CFLAGS to sub-make.
> + * hsdis.c: Include string.h.
> + (parse_caller_options): Fix strncmp bug.
> + (native_arch_name): Add entry for ARM.
The makefile check for arm can get improved , apart from that this new patch looks ok see below:
> +
> +diff -u openjdk/hotspot/src/share/tools/hsdis/hsdis.c ../icedtea6/openjdk/hotspot/src/share/tools/hsdis/hsdis.c
> +--- openjdk/hotspot/src/share/tools/hsdis/hsdis.c 2011-11-14 17:07:33.000000000 -0500
> ++++ openjdk/hotspot/src/share/tools/hsdis/hsdis.c 2012-01-20 10:21:28.000000000 -0500
> +@@ -22,8 +22,6 @@
> + *
> + */
> +
> +-#include "precompiled.hpp"
> +-
> + /* hsdis.c -- dump a range of addresses as native instructions
> + This implements the plugin protocol required by the
> + HotSpot PrintAssembly option.
> +@@ -37,6 +35,8 @@
> + #include <dis-asm.h>
> + #include <inttypes.h>
> +
> ++#include <string.h>
> ++
> + #ifndef bool
> + #define bool int
> + #define true 1
> +@@ -358,7 +358,7 @@
> + if (plen > mach_size) plen = mach_size;
> + strncpy(mach_option, p, plen);
> + mach_option[plen] = '\0';
> +- } else if (plen > 6 && strncmp(p, "hsdis-", 6)) {
> ++ } else if (plen > 6 && !strncmp(p, "hsdis-", 6)) {
> + // do not pass these to the next level
> + } else {
> + /* just copy it; {i386,sparc}-dis.c might like to see it */
> +@@ -420,6 +420,9 @@
> + #ifdef LIBARCH_sparcv9
> + res = "sparc:v9b";
> + #endif
> ++#ifdef LIBARCH_arm
> ++ res = "arm";
> ++#endif
> + if (res == NULL)
> + res = "architecture not set in Makefile!";
> + return res;
> +diff -u openjdk/hotspot/src/share/tools/hsdis/Makefile openjdk/hotspot/src/share/tools/hsdis/Makefile
ok
> +--- openjdk/hotspot/src/share/tools/hsdis/Makefile 2011-11-14 17:07:33.000000000 -0500
> ++++ openjdk/hotspot/src/share/tools/hsdis/Makefile 2012-01-20 10:20:32.000000000 -0500
> +@@ -68,14 +68,18 @@
> + CONFIGURE_ARGS= --host=$(MINGW) --target=$(MINGW)
> + else
> + CPU = $(shell uname -m)
> ++ifeq ($(CPU),armv7l)
What about other arm CPUs?
How about ifneq ($(findstring arm,$(CPU)),)
> ++ARCH=arm
> ++else
> + ARCH1=$(CPU:x86_64=amd64)
> + ARCH=$(ARCH1:i686=i386)
> + CFLAGS/i386 += -m32
> + CFLAGS/sparc += -m32
> + CFLAGS/sparcv9 += -m64
> + CFLAGS/amd64 += -m64
> ++endif
> + CFLAGS += $(CFLAGS/$(ARCH))
> +-CFLAGS += -fPIC
> ++CFLAGS += -fPIC -g
> + OS = linux
> + LIB_EXT = .so
> + CC = gcc
> +@@ -118,7 +122,7 @@
> + BINUTILSDIR = $(shell cd $(BINUTILS);pwd)
> + endif
> +
> +-CPPFLAGS += -I$(BINUTILSDIR)/include -I$(BINUTILS)/bfd -I$(TARGET_DIR)/bfd
> ++CPPFLAGS += -I$(BINUTILSDIR)/include -I$(BINUTILSDIR)/bfd -I$(TARGET_DIR)/bfd
> + CPPFLAGS += -DLIBARCH_$(LIBARCH) -DLIBARCH=\"$(LIBARCH)\" -DLIB_EXT=\"$(LIB_EXT)\"
> +
> + TARGET_DIR = build/$(OS)-$(JDKARCH)
> +@@ -145,7 +149,7 @@
> + demo: $(TARGET) $(DEMO_TARGET)
> +
> + $(LIBRARIES): $(TARGET_DIR) $(TARGET_DIR)/Makefile
> +- if [ ! -f $@ ]; then cd $(TARGET_DIR); make all-opcodes; fi
> ++ if [ ! -f $@ ]; then cd $(TARGET_DIR); make all-opcodes "CFLAGS=$(CFLAGS)"; fi
> +
> + $(TARGET_DIR)/Makefile:
> + (cd $(TARGET_DIR); CC=$(CC) CFLAGS="$(CFLAGS)" $(BINUTILSDIR)/configure --disable-nls $(CONFIGURE_ARGS))
ok
More information about the distro-pkg-dev
mailing list