Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sign extend 32bit offsets in inlineIntrinsicInflate #20970

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

klangman
Copy link
Contributor

Without sign extending the 32bit offsets, it is possible for the upper 32bits of the offset registers to contain garbage bits that will cause the address calculations to produce incorrect (unaddressable) results.

Without sign extending the 32bit offsets, it is possible for the upper 32bits of the offset registers to contain garbage bits that will cause the address calculations to produce incorrect (unaddressable) results.
@klangman
Copy link
Contributor Author

@IBMJimmyk @zl-wang This fixes the problem that we talked about a few weeks back. My personal build (for all POWER platforms) passed.

@IBMJimmyk
Copy link
Contributor

It looks good to me.

@zl-wang
Copy link
Contributor

zl-wang commented Jan 17, 2025

Did you find out the specific situation where garbage upper 32bits made its way into register?
This PR certainly fixed your particular issue on hand, but it might miss the real culprit at large. Appreciate it if that can be found out. I think codegen should be expected to produce correct values in full register.

@klangman
Copy link
Contributor Author

klangman commented Jan 17, 2025

Did you find out the specific situation where garbage upper 32bits made its way into register?

In the failing case the caller of String.getChars() calculates the 1st parameter (srcBegin) to be the return from String.lastIndexOf() using a parameter string that is not contained in the 'this' String, then it adds one. Since lastIndexOf() returns -1 (0xffffffff), and then we add 1 we get (0x100000000) in the 64bit register. This value is passed to String.getChars() which needs to be 32bit sign extended to be a proper 64bit 0.

Here is the test-case:

public class getCharsCrash {
   public static void main(String argv[]){
      for( int i=0 ; i< 5000 ; i++ ) {
         test(argv[0]);
      }
   }

   static void test(String str) {
      char dest[] = new char[128];
      int begin = str.lastIndexOf("missing");
      System.out.println("begin: " + begin);
      begin++;
      str.getChars(begin, str.length()-1, dest, 0);
      System.out.println("dest: " + new String(dest));
   }
}

With the right inlining restrictions this test-case crashes in String.getChars().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants