Groovy
  1. Groovy
  2. GROOVY-7841

Assert fails when accessing particular primitive values with @CompileStatic

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.5
    • Fix Version/s: 2.4.7
    • Component/s: None
    • Labels:
      None

      Description

      The following assert for a non-zero long value fails when annotated with CompileStatic. For example:

      def foo() {
        long value = 17179869184 
        assert value, "Foo is OK"
      }
      
      @groovy.transform.CompileStatic
      def bar() {
        long value = 17179869184 
        assert value, "Bar failed"
      }
      
      foo()
      bar()
      
      
      java.lang.AssertionError: Bar failed. Expression: value. Values: value = 17179869184
      	at ConsoleScript31.bar(ConsoleScript31:10)
      	at ConsoleScript31.run(ConsoleScript31:14)
      
      

        Issue Links

          Activity

          Hide
          Balachandran Sivakumar added a comment -

          This seems to happen only with that specific number (17179869184). It is working fine for other values that I checked randomly. That specific number exactly is 16 GB (16 * 1024 * 1024* 1024). I am not sure why it fails exactly for that.

          Show
          Balachandran Sivakumar added a comment - This seems to happen only with that specific number (17179869184). It is working fine for other values that I checked randomly. That specific number exactly is 16 GB (16 * 1024 * 1024* 1024). I am not sure why it fails exactly for that.
          Hide
          Paul King added a comment - - edited

          Any multiple of 4294967296 will have the same result. Looking at the bytecode, there is an L2I inserted for the @CompileStatic case which just keeps the lowest 32 bits. Workaround is to use the longhand equivalent assert value != 0.

          My suspicion is we are applying this optimization a bit too aggressively:
          https://github.com/apache/groovy/blob/master/src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java#L137

          Show
          Paul King added a comment - - edited Any multiple of 4294967296 will have the same result. Looking at the bytecode, there is an L2I inserted for the @CompileStatic case which just keeps the lowest 32 bits. Workaround is to use the longhand equivalent assert value != 0 . My suspicion is we are applying this optimization a bit too aggressively: https://github.com/apache/groovy/blob/master/src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java#L137
          Hide
          Jochen Theodorou added a comment -

          The usages of F2I and D2I look wrong too.

          Show
          Jochen Theodorou added a comment - The usages of F2I and D2I look wrong too.
          Hide
          Paul King added a comment -

          Indeed, both assert 0.1f and assert 0.1d also fail (within @CompileStatic) but should succeed.

          Show
          Paul King added a comment - Indeed, both assert 0.1f and assert 0.1d also fail (within @CompileStatic) but should succeed.
          Hide
          ASF GitHub Bot added a comment -

          GitHub user paulk-asert opened a pull request:

          https://github.com/apache/groovy/pull/338

          GROOVY-7841: Assert fails when accessing particular primitive values …

          …with @CompileStatic

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/paulk-asert/groovy groovy7841

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/groovy/pull/338.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #338


          commit c7d106315226667248dca01af793bb24b96d8d97
          Author: paulk <paulk@asert.com.au>
          Date: 2016-05-24T11:52:12Z

          GROOVY-7841: Assert fails when accessing particular primitive values with @CompileStatic


          Show
          ASF GitHub Bot added a comment - GitHub user paulk-asert opened a pull request: https://github.com/apache/groovy/pull/338 GROOVY-7841 : Assert fails when accessing particular primitive values … …with @CompileStatic You can merge this pull request into a Git repository by running: $ git pull https://github.com/paulk-asert/groovy groovy7841 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/groovy/pull/338.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #338 commit c7d106315226667248dca01af793bb24b96d8d97 Author: paulk <paulk@asert.com.au> Date: 2016-05-24T11:52:12Z GROOVY-7841 : Assert fails when accessing particular primitive values with @CompileStatic
          Hide
          ASF GitHub Bot added a comment -

          Github user shils commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/338#discussion_r64490605

          — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java —
          @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor)

          { // int on stack }

          else if (top.equals(ClassHelper.long_TYPE))

          { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); }

          else if (top.equals(ClassHelper.float_TYPE)) {
          MethodVisitor mv = controller.getMethodVisitor();

          • mv.visitInsn(F2I);
            + mv.visitInsn(F2D);
              • End diff –

          Why not use a compare float operation here?

          Show
          ASF GitHub Bot added a comment - Github user shils commented on a diff in the pull request: https://github.com/apache/groovy/pull/338#discussion_r64490605 — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java — @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor) { // int on stack } else if (top.equals(ClassHelper.long_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); } else if (top.equals(ClassHelper.float_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); mv.visitInsn(F2I); + mv.visitInsn(F2D); End diff – Why not use a compare float operation here?
          Hide
          ASF GitHub Bot added a comment -

          Github user paulk-asert commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/338#discussion_r64549052

          — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java —
          @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor)

          { // int on stack }

          else if (top.equals(ClassHelper.long_TYPE))

          { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); }

          else if (top.equals(ClassHelper.float_TYPE)) {
          MethodVisitor mv = controller.getMethodVisitor();

          • mv.visitInsn(F2I);
            + mv.visitInsn(F2D);
              • End diff –

          We probably could. In general, comparisons with floats are notoriously ugly, e.g. see:
          https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
          But given we are comparing against 0.0, it is probably an edge case that isn't as bad as the general case (but see 'Infernal zero' in the aforementioned link). A general recommendation is to use doubles instead of floats but I don't really know for sure whether it does in this case. My rationale was to look at what bytecode was produced from `assert floatValue != 0.0f` and then replicating that - which is where the `F2D` and `DCMPG` came from.

          Show
          ASF GitHub Bot added a comment - Github user paulk-asert commented on a diff in the pull request: https://github.com/apache/groovy/pull/338#discussion_r64549052 — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java — @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor) { // int on stack } else if (top.equals(ClassHelper.long_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); } else if (top.equals(ClassHelper.float_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); mv.visitInsn(F2I); + mv.visitInsn(F2D); End diff – We probably could. In general, comparisons with floats are notoriously ugly, e.g. see: https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ But given we are comparing against 0.0, it is probably an edge case that isn't as bad as the general case (but see 'Infernal zero' in the aforementioned link). A general recommendation is to use doubles instead of floats but I don't really know for sure whether it does in this case. My rationale was to look at what bytecode was produced from `assert floatValue != 0.0f` and then replicating that - which is where the `F2D` and `DCMPG` came from.
          Hide
          ASF GitHub Bot added a comment -

          Github user paulk-asert closed the pull request at:

          https://github.com/apache/groovy/pull/338

          Show
          ASF GitHub Bot added a comment - Github user paulk-asert closed the pull request at: https://github.com/apache/groovy/pull/338
          Hide
          Paul King added a comment -

          PR merged

          Show
          Paul King added a comment - PR merged
          Hide
          ASF GitHub Bot added a comment -

          Github user shils commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/338#discussion_r64561363

          — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java —
          @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor)

          { // int on stack }

          else if (top.equals(ClassHelper.long_TYPE))

          { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); }

          else if (top.equals(ClassHelper.float_TYPE)) {
          MethodVisitor mv = controller.getMethodVisitor();

          • mv.visitInsn(F2I);
            + mv.visitInsn(F2D);
              • End diff –

          Thanks for the explanation.

          Show
          ASF GitHub Bot added a comment - Github user shils commented on a diff in the pull request: https://github.com/apache/groovy/pull/338#discussion_r64561363 — Diff: src/main/org/codehaus/groovy/transform/sc/transformers/BooleanExpressionTransformer.java — @@ -134,15 +134,19 @@ public void visit(final GroovyCodeVisitor visitor) { // int on stack } else if (top.equals(ClassHelper.long_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); - mv.visitInsn(L2I); + mv.visitInsn(LCONST_0); + mv.visitInsn(LCMP); controller.getOperandStack().replace(ClassHelper.boolean_TYPE); } else if (top.equals(ClassHelper.float_TYPE)) { MethodVisitor mv = controller.getMethodVisitor(); mv.visitInsn(F2I); + mv.visitInsn(F2D); End diff – Thanks for the explanation.

            People

            • Assignee:
              Paul King
              Reporter:
              paolo di tommaso
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development