[PATCH 0/1] Fix JamVM to work with current gcc and glibc

  • Done
  • quality assurance status badge
Details
2 participants
  • Marius Bakke
  • Simon South
Owner
unassigned
Submitted by
Simon South
Severity
normal
S
S
Simon South wrote on 7 Jun 2020 17:17
(address . guix-patches@gnu.org)(name . Simon South)(address . simon@simonsouth.net)
20200607151709.21821-1-simon@simonsouth.net
This patch allows JamVM 2.0.0 to be built without using outdated versions of
gcc and glibc, by disabling the branch-patching optimization JamVM normally
applies to code it has inlined.

I've successfully built IcedTea 1.13.13 on x86_64 with this patch applied,
without encountering any "Invalid instruction" errors. It also seems to work
fine on aarch64.

Lengthy explanation, for anyone interested in the details:

JamVM's branch-patching optimization tries to improve the performance of
Java's branching opcodes by replacing the load-operand-and-jump portion of
their implementation with a single jump instruction directly to the handler
for the opcode to which the operand points. However, the current
implementation

- Doesn't attempt much of an optimization, since it only replaces the final
jump instruction with another (the operand is still loaded in either case).

- Doesn't have any actual effect since the instruction it replaces is a
duplicate synthesized by gcc that is never executed anyway.

- Is prone to breakage, since it doesn't ensure the new jump instruction is
the same length as the original. (This is the specific reason for the
failure on x86_64: The replacement jump instruction is longer and clobbers
part of the following instruction, making it invalid.)

This patch simply disables the optimization within JamVM, leaving the other
optimizations intact. (gcc is smart enough to no longer generate duplicate
jump instructions in this case so it reduces the code size slightly, too.)

Alternate solutions I considered and rejected:

- Fixing the implementation. JamVM uses a label to mark within each opcode's
handler where the jump instruction should be placed, and moving this label
to the start of the load-and-jump sequence rather than the end appears (from
stepping through the code with gdb) to fix all the issues above. However,
JamVM then fails at startup, reporting an unhandle-able exception during
initialization. So presumably some other part of the code relies on the
current, broken implementation of this feature, and while it's probably
possible to fix _that_ as well I doubt it would be worth the effort.

- Disabling the optimization at runtime. Invoking JamVM with "-Xnopatching"
also solves the problem, and we could just update the bootstrap procedure to
do this. However JamVM 1.5.1 doesn't recognize this option and fails if it's
specified, so we'd need to change the ecj-javac-wrapper package to handle
the two interpreters differently or to accept an argument specifying the
flags to use, and in my opinion this would be a step backwards from what
exists currently.

Also, since JamVM would only ever work reliably when this option is
specified, it's not really much of an "option" and making its effect
permanent in the code seems like a more sensible approach.
--
Simon South
simon@simonsouth.net


Simon South (1):
gnu: jamvm: Fix to work with current gcc and glibc.

gnu/packages/java.scm | 7 ++---
.../jamvm-2.0.0-disable-branch-patching.patch | 27 +++++++++++++++++++
2 files changed, 29 insertions(+), 5 deletions(-)
create mode 100644 gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch

--
2.26.2
S
S
Simon South wrote on 7 Jun 2020 17:18
[PATCH 1/1] gnu: jamvm: Fix to work with current gcc and glibc.
(address . 41748@debbugs.gnu.org)(name . Simon South)(address . simon@simonsouth.net)
20200607151833.22052-1-simon@simonsouth.net
* gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch: New file.
* gnu/packages/java.scm (jamvm)[source]: Add patch.
(jamvm-1-bootstrap)[native-inputs]: Remove.
---
gnu/packages/java.scm | 7 ++---
.../jamvm-2.0.0-disable-branch-patching.patch | 27 +++++++++++++++++++
2 files changed, 29 insertions(+), 5 deletions(-)
create mode 100644 gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch

Toggle diff (60 lines)
diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index 43f0f37b91..9a6f6fe0df 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -254,11 +254,6 @@ language.")
("libffi" ,libffi)
("zip" ,zip)
("zlib" ,zlib)))
- ;; When built with a recent GCC and glibc the configure step of icedtea-6
- ;; fails with an invalid instruction error.
- (native-inputs
- `(("gcc" ,gcc-5)
- ("libc" ,glibc-2.28)))
(home-page "http://jamvm.sourceforge.net/")
(synopsis "Small Java Virtual Machine")
(description "JamVM is a Java Virtual Machine conforming to the JVM
@@ -708,6 +703,8 @@ machine.")))
(sha256
(base32
"1nl0zxz8y5x8gwsrm7n32bry4dx8x70p8z3s9jbdvs8avyb8whkn"))
+ (patches
+ (search-patches "jamvm-2.0.0-disable-branch-patching.patch"))
(snippet
'(begin
;; Remove precompiled software.
diff --git a/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch b/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch
new file mode 100644
index 0000000000..a99624280f
--- /dev/null
+++ b/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch
@@ -0,0 +1,27 @@
+From 8d10be7345d7fe139e619cc55a22dbc86f677543 Mon Sep 17 00:00:00 2001
+From: Simon South <simon@simonsouth.net>
+Date: Sat, 6 Jun 2020 18:56:56 -0400
+Subject: [PATCH] Disable branch-patching
+
+---
+ src/init.c | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/init.c b/src/init.c
+index 32539cf..38ad54b 100644
+--- a/src/init.c
++++ b/src/init.c
+@@ -72,8 +72,8 @@ void setDefaultInitArgs(InitArgs *args) {
+ #ifdef INLINING
+ args->replication_threshold = 10;
+ args->profile_threshold = 10;
+- args->branch_patching_dup = TRUE;
+- args->branch_patching = TRUE;
++ args->branch_patching_dup = FALSE;
++ args->branch_patching = FALSE;
+ args->print_codestats = FALSE;
+ args->join_blocks = TRUE;
+ args->profiling = TRUE;
+--
+2.25.2
+
--
2.26.2
S
S
Simon South wrote on 10 Jun 2020 17:03
Re: [PATCH 0/1] Fix JamVM to work with current gcc and glibc
(address . 41748@debbugs.gnu.org)
87a71botdg.fsf@simonsouth.net
Attached is an updated version of this patch that

- Adds a brief description to the patch it contains, indicating why it
is necessary; and

- Updates gnu/local.mk, as is apparently now required.

--
Simon South
simon@simonsouth.net
From 3dbc081418737b0901982f89b8eb260b6c8bd3f0 Mon Sep 17 00:00:00 2001
From: Simon South <simon@simonsouth.net>
Date: Sat, 6 Jun 2020 19:53:39 -0400
Subject: [PATCH] gnu: jamvm: Fix to work with current gcc and glibc.

* gnu/packages/java.scm (jamvm)[source]: Add patch.
(jamvm-1-bootstrap)[native-inputs]: Remove.
* gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
---
gnu/local.mk | 1 +
gnu/packages/java.scm | 7 ++---
.../jamvm-2.0.0-disable-branch-patching.patch | 31 +++++++++++++++++++
3 files changed, 34 insertions(+), 5 deletions(-)
create mode 100644 gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch

Toggle diff (76 lines)
diff --git a/gnu/local.mk b/gnu/local.mk
index 76d6b5deba..7d65fbb605 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1107,6 +1107,7 @@ dist_patch_DATA = \
%D%/packages/patches/irrlicht-use-system-libs.patch \
%D%/packages/patches/isl-0.11.1-aarch64-support.patch \
%D%/packages/patches/jacal-fix-texinfo.patch \
+ %D%/packages/patches/jamvm-2.0.0-disable-branch-patching.patch \
%D%/packages/patches/jamvm-arm.patch \
%D%/packages/patches/java-apache-ivy-port-to-latest-bouncycastle.patch \
%D%/packages/patches/java-commons-collections-fix-java8.patch \
diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index 43f0f37b91..9a6f6fe0df 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -254,11 +254,6 @@ language.")
("libffi" ,libffi)
("zip" ,zip)
("zlib" ,zlib)))
- ;; When built with a recent GCC and glibc the configure step of icedtea-6
- ;; fails with an invalid instruction error.
- (native-inputs
- `(("gcc" ,gcc-5)
- ("libc" ,glibc-2.28)))
(home-page "http://jamvm.sourceforge.net/")
(synopsis "Small Java Virtual Machine")
(description "JamVM is a Java Virtual Machine conforming to the JVM
@@ -708,6 +703,8 @@ machine.")))
(sha256
(base32
"1nl0zxz8y5x8gwsrm7n32bry4dx8x70p8z3s9jbdvs8avyb8whkn"))
+ (patches
+ (search-patches "jamvm-2.0.0-disable-branch-patching.patch"))
(snippet
'(begin
;; Remove precompiled software.
diff --git a/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch b/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch
new file mode 100644
index 0000000000..1352ed7803
--- /dev/null
+++ b/gnu/packages/patches/jamvm-2.0.0-disable-branch-patching.patch
@@ -0,0 +1,31 @@
+From d80cfc83325f8e95d35ecd9f15b36b96fa9ed3ee Mon Sep 17 00:00:00 2001
+From: Simon South <simon@simonsouth.net>
+Date: Sat, 6 Jun 2020 18:56:56 -0400
+Subject: [PATCH] Disable branch-patching
+
+This patch disables JamVM's branch-patching optimization, which tends
+to make JamVM fail with an "Illegal instruction" error on x86_64 (and
+possibly other architectures that use variable-length instructions)
+when built using modern versions of gcc and glibc.
+---
+ src/init.c | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/init.c b/src/init.c
+index 32539cf..38ad54b 100644
+--- a/src/init.c
++++ b/src/init.c
+@@ -72,8 +72,8 @@ void setDefaultInitArgs(InitArgs *args) {
+ #ifdef INLINING
+ args->replication_threshold = 10;
+ args->profile_threshold = 10;
+- args->branch_patching_dup = TRUE;
+- args->branch_patching = TRUE;
++ args->branch_patching_dup = FALSE;
++ args->branch_patching = FALSE;
+ args->print_codestats = FALSE;
+ args->join_blocks = TRUE;
+ args->profiling = TRUE;
+--
+2.25.2
+
--
2.26.2
M
M
Marius Bakke wrote on 22 Jun 2020 22:37
Re: [bug#41748] [PATCH 0/1] Fix JamVM to work with current gcc and glibc
(name . Simon South)(address . simon@simonsouth.net)
87bllabzww.fsf@gnu.org
Simon South <simon@simonsouth.net> writes:

Toggle quote (8 lines)
> This patch allows JamVM 2.0.0 to be built without using outdated versions of
> gcc and glibc, by disabling the branch-patching optimization JamVM normally
> applies to code it has inlined.
>
> I've successfully built IcedTea 1.13.13 on x86_64 with this patch applied,
> without encountering any "Invalid instruction" errors. It also seems to work
> fine on aarch64.

Thanks a lot for this patch, and the brilliant analysis. I have applied
the updated version and will push shortly.

I modified it to also remove the (gnu packages gcc) import from
java.scm. \o/
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAl7xFm8ACgkQoqBt8qM6
VPpS6AgAxLCzzByJwscvZVkAF7DhFqsoPU55JrjAJqkTQKwK1WCMmEBSuIEsYrfx
scJvskqZcdbWOtdndiapzvgA3ifa36Oi5l/Lk383JIrMnuXACZIyLPEdQ13nyebs
gT2UHo+ebMG9a3dFjC5jRydCfMptD12YboqiKOS3/MwFDzfkIs1XDYfTEBm9dqFZ
Io7KBcBQ3xfpa3IE9lhcpEsl1a7FpZ4Skz/HKy8e/f6zgvwr36oysV/PSWZidSMv
wBuV8xTZK9iZh04l8bx6GT98FShqNVWJf5zeDV7d0V1L9GabwkcIaYY8eRSf35zk
yDYNq9Vx/coZlCxgXnsxTgKDoDP+kw==
=rbjk
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 41748@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 41748
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch