Subject: bug#41748: closed (Re: [bug#41748] [PATCH 0/1] Fix JamVM to work with current gcc and glibc)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 22 Jun 2020 20:38:02 +0000

Your bug report

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

which was filed against the guix-patches package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
41748: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=41748
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [bug#41748] [PATCH 0/1] Fix JamVM to work with current gcc and glibc
From: Marius Bakke <[email-masked]>
To: Simon South <[email-masked]>, [email-masked]
Cc: Simon South <[email-masked]>
Date: Mon, 22 Jun 2020 22:37:03 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Simon South <[email-masked]> writes:

> 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/

Subject: [PATCH 0/1] Fix JamVM to work with current gcc and glibc
From: Simon South <[email-masked]>
To: [email-masked]
Cc: Simon South <[email-masked]>
Date: Sun, 7 Jun 2020 11:17:09 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

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
[email-masked]

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