eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 720 forks source link

FIND_ARG_IN_VMARGS Returns invalid argument index #7667

Open AlenBadel opened 5 years ago

AlenBadel commented 5 years ago

Issue When -Xlp<size> is specified by the user, the correct index of the -Xlp argument is not being returned from FIND_ARG_IN_VMARGS. See https://github.com/eclipse/openj9/blob/86f128ca839256f15d30d7178fcd3d256963e265/runtime/compiler/control/J9Options.cpp#L1742

FIND_ARG_IN_VMARGS is a macro defined call to findArgInVMArgs See https://github.com/eclipse/openj9/blob/8ecd11892bc38f416ba474076ca6c815da699e8f/runtime/util/vmargs.c#L82

Instead of getting the correct argument index it always returns 1 on the first invocation of findArgInVMArgs. However, on every further invocation we get the right index of the argument. It seems that this problem has always existed, and was exposed by https://github.com/eclipse/openj9/commit/bba5a43eb390b012fdf4a675a838baf822a2d06d. Before this commit, we were making a useless invocation of findArgInVMArgs, so when invoked again it always returned the expected result.

Environment Fails on PPC64LE Linux/Ubuntu Fails with OpenJ9 Java 11 Fails Using GCC-7.3 No other architecture, JDK version, or GCC version was tested. So this issue may be fixed in later versions of GCC up to 9.2.

Analysis to follow.

AlenBadel commented 5 years ago

Analysis I've went ahead and created two invocations of findArgInVMArgs.

      else if (lpArgIndex = FIND_ARG_IN_VMARGS(EXACT_MEMORY_MATCH, "-Xlp", NULL) >= 0)
         {
         printf("IpArgIndex:%d\n", lpArgIndex);
         int32_t lpArgIndex2 = FIND_ARG_IN_VMARGS(EXACT_MEMORY_MATCH, "-Xlp", NULL);
         printf("lpArgIndex2:%d\n", lpArgIndex2);
         ...
         }

Printing the result of findArgInVMArgs right before returning on both calls looks to be returning the right index of the argument both times.

I can confirm this by taking a look at the assembly of findArgInVMArgs. The correct index is being stored into r3 which is the register that the caller expects the result to be stored in when returned.

Now taking a look at the assembly of the caller fePreProcess. Comparing the assembly of the two calls upon returning from findArgInVMArgs the problem becomes obvious.

Problematic Call

=> 0x7ffff5f33c9c <_ZN2J97Options12fePreProcessEPv+4668>   addis   r3,r2,-34
  0x7ffff5f33ca0 <_ZN2J97Options12fePreProcessEPv+4672>   addi    r28,r1,272
   0x7ffff5f33ca4 <_ZN2J97Options12fePreProcessEPv+4676>   li      r4,1
   0x7ffff5f33ca8 <_ZN2J97Options12fePreProcessEPv+4680>   addi    r3,r3,7680
   0x7ffff5f33cac <_ZN2J97Options12fePreProcessEPv+4684>   bl      0x7ffff5e92844 <00000289.plt_call.printf@@GLIBC_2.17>

Successful Call

  0x7ffff5f33ce8 <_ZN2J97Options12fePreProcessEPv+4744>:       ld      r2,24(r1)
   0x7ffff5f33cec <_ZN2J97Options12fePreProcessEPv+4748>:       extsw   r4,r3
   0x7ffff5f33cf0 <_ZN2J97Options12fePreProcessEPv+4752>:       addis   r3,r2,-34
=> 0x7ffff5f33cf4 <_ZN2J97Options12fePreProcessEPv+4756>:       addi    r3,r3,7696
   0x7ffff5f33cf8 <_ZN2J97Options12fePreProcessEPv+4760>:       bl      0x7ffff5e92844 <00000289.plt_call.printf@@GLIBC_2.17>

So, in general to make the printf call there are two arguments, the address of the string to be printed which is stored into r3, and the value to be printed which is stored into r4.

In both cases, the address of the string portion is successfully moved into r3.

With the proper call, the value returned from findArgInVMArgs is moved from r3 to r4 using the extsw instruction. The extsw instruction copies the lower order 32-bits from r3 into r4. extsw r4,r3

With the problematic call we can see that we're just loading 1 into r4, hence why we just see 1 being returned. li r4,1

Simply, GCC seems to be inserting the wrong instruction. Using li instead of extsw.

pshipton commented 5 years ago

Preferably we should just write code to work around the gcc problem. There aren't any plans to upgrade the compiler version.

AlenBadel commented 5 years ago

I'll work on finding a workaround but we need to determine if this was resolved in later versions gcc otherwise this needs to be reported. I'll try to cut this down to a simple test case.

pshipton commented 5 years ago

There is a gcc 7.4, it would be interesting to determine if this version fixes the problem. Loading the gcc7 package should get you that one.

AlenBadel commented 5 years ago

Would there be any particular reason this would be resolved in gcc 7.4?

We'll undoubtedly run into issues building openj9 on ppc64le with newer versions of gcc. If we're able to create a simple test case to produce this failure, then we can definitely try out the GCC head 9.2, or even V10.

I wasn't able to find anything that relates to this issue on GCC's issue tracker. https://gcc.gnu.org/bugzilla/

pshipton commented 5 years ago

Problems were resolved in 7.4 since 7.3 https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=RESOLVED&resolution=FIXED&target_milestone=7.4

The gcc version OpenJ9 uses is tied to what OpenJDK supports. https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms

If we thought it worth the effort, we could try to move from 7.3 to 7.4 (in fact zLinux already uses 7.4) after doing some perf analysis. Moving to 9 or 10 is more unlikely.

AlenBadel commented 5 years ago

@pshipton I've created a workaround for this for the time being. Not sure if we have sufficient testing coverage that can tell us if any other option may be experiencing the same issue.

pshipton commented 5 years ago

I was reminded that we expect to update to gcc 7.5 once it is available, as it should contain a fix we want to speed up the interpreter.