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

8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address() #3040

Closed
wants to merge 1 commit into from

Conversation

RealFYang
Copy link
Member

@RealFYang RealFYang commented Mar 17, 2021

Noticed this issue when I am trying to backport: https://bugs.openjdk.java.net/browse/JDK-8263425

Around line 180 we have:

     __ add(index, LIR_OprFact::intptrConst(large_disp), tmp);
     index = tmp;
   } else {
     __ move(tmp, LIR_OprFact::intptrConst(large_disp));      <========
     __ add(tmp, index, tmp);
     index = tmp;
   }

This is supposed to be calculating "tmp = large_disp" but it actually does "large_disp = tmp".
Looks like this is missed by JDK-8263425.
Tested tier1 with -XX:TieredStopAtLevel=1 on AArch64 Linux.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3040/head:pull/3040
$ git checkout pull/3040

To update a local copy of the PR:
$ git checkout pull/3040
$ git pull https://git.openjdk.java.net/jdk pull/3040/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 17, 2021

👋 Welcome back fyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 17, 2021
@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@RealFYang The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 17, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2021

Webrevs

@nick-arm
Copy link
Contributor

Yes I missed that one. I guess neither of those branches get executed normally.

Copy link
Contributor

@adinn adinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@RealFYang This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()

Reviewed-by: adinn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

  • d1baed6: 8263672: fatal error: no reachable node should have no use
  • 086a66a: 8261095: Add test for clhsdb "symbol" command
  • ec95a5c: 8263410: ListModel javadoc refers to non-existent interface
  • 7b9d256: 8261262: Kitchensink24HStress.java crashed with EXCEPTION_ACCESS_VIOLATION
  • d2144a5: 8263058: Optimize vector shift with zero shift count
  • dd6c911: 8263705: Two shenandoah tests fail due to can't find ClassFileInstaller
  • 4acb883: 8261666: [mlvm] Remove WhiteBoxHelper

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 17, 2021
@theRealAph
Copy link
Contributor

I think we're not being curious enough. It doesn't make sense for anyone to check in code they can't test, and from what I understand this code path can never have been executed.

The mistake seems to date from the very early days of the AArch64-port project. The logic seems to have been based on this code from the SPARC port:

  if (index->is_register()) {
    // apply the shift and accumulate the displacement
    if (shift > 0) {
      LIR_Opr tmp = new_pointer_register();
      __ shift_left(index, shift, tmp);
      index = tmp;
    }
    if (disp != 0) {
      LIR_Opr tmp = new_pointer_register();
      if (Assembler::is_simm13(disp)) {
        __ add(tmp, LIR_OprFact::intptrConst(disp), tmp);
        index = tmp;
      } else {
        __ move(LIR_OprFact::intptrConst(disp), tmp);
        __ add(tmp, index, tmp);
        index = tmp;
      }
      disp = 0;
    }
  } else if (disp != 0 && !Assembler::is_simm13(disp)) {
    // index is illegal so replace it with the displacement loaded into a register
    index = new_pointer_register();
    __ move(LIR_OprFact::intptrConst(disp), index);
    disp = 0;
  }

So the mistake is pretty obvious, but does it make any sense to correct some code that has never been executed without finding out why it is never executed? We shouldn't check in any changes we don't test.

@adinn
Copy link
Contributor

adinn commented Mar 17, 2021

The lack of exercise for this specific branch appears to be a fortuitous /contingency/.

Currently, generate_address is only called /directly/ with small values for disp e.g.

src/hotspot/share/gc/shared/c1/barrierSetC1.cpp:    addr_opr = LIR_OprFact::address(gen->generate_address(base.result(), offset, 0, 0, access.type()));
src/hotspot/share/c1/c1_LIRGenerator.cpp:    addr = generate_address(base_op, index_op, log2_scale, 0, dst_type);
. . .

Also, many of those uses requires GENERATE_ADDRESS_IS_PREFERRED to be defined on tghe compile line.

generate_address is also called indirectly from LIRGenerator::cmp_mem_int and LIRGenerator::cmp_reg_mem. These are only used in generic C1 code to plant range checks for arrays and nio buffers so, once again, the displacement is a known small constant.

However, none of that guarantees that future uses will be guaranteed to employ such small constant displacements. I suggest we push this patch just in case someone decides to use it with a larger displacement. An assert might otherwise be in order.

@theRealAph
Copy link
Contributor

The lack of exercise for this specific branch appears to be a fortuitous /contingency/.

Hmm. Do you suppose that perhaps the bug could be provoked actually to fire with a contrived test case? I guess a huge Object wouldn't be enough, because there already huge Object tests in jtreg.

@adinn
Copy link
Contributor

adinn commented Mar 17, 2021

Hmm. Do you suppose that perhaps the bug could be provoked actually to fire with a contrived test case? I guess a huge Object wouldn't be enough, because there already huge Object tests in jtreg.

I'm not sure you understood what I was saying. This is a contingency in the current implementation of C1, not an execution flow contingency dependent on present use cases. Nothing is going to exercise this without a code change in the C1 code because disp is always passed as a specific well known, small constant like the array header length offset or the offset of Java instance field ByteBuffer.length.

My concern here is that such a code change could be waiting to pounce around the next corner.

@theRealAph
Copy link
Contributor

Hmm. Do you suppose that perhaps the bug could be provoked actually to fire with a contrived test case? I guess a huge Object wouldn't be enough, because there already huge Object tests in jtreg.

I'm not sure you understood what I was saying. This is a contingency in the current implementation of C1, not an execution flow contingency dependent on present use cases. Nothing is going to exercise this without a code change in the C1 code because disp is always passed as a specific well known, small constant like the array header length offset or the offset of Java instance field ByteBuffer.length.

Ah. I read your quoted code and it appeared to be a field offset.

My concern here is that such a code change could be waiting to pounce around the next corner.

Sure. OK, but never-executed code will always give me shivers.

@RealFYang
Copy link
Member Author

Thanks for taking a further step to find it out. Based on the discussion, let's get this in. So that people won't get confused when they are looking at this part.

@RealFYang
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Mar 18, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 18, 2021
@openjdk
Copy link

openjdk bot commented Mar 18, 2021

@RealFYang Since your change was applied there have been 26 commits pushed to the master branch:

  • 9225a23: 8263108: Class initialization deadlock in java.lang.constant
  • 5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
  • 50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
  • 99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling
  • 26234b5: 8254979: Class.getSimpleName() returns non-empty for lambda and method
  • 83a49ef: 8263753: two new tests from JDK-8261671 fail with "Error. can not find ClassFileInstaller in test directory or libraries"
  • 24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
  • cdf78e4: 8262298: G1BarrierSetC2::step_over_gc_barrier fails with assert "bad barrier shape"
  • 7674da4: 8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode
  • 4f4ca0e: 8261671: X86 I2L conversion can be skipped for certain masked positive values
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/50697965cfde3c2349494b8ce0b922422307f9d8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 81ba578.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RealFYang RealFYang deleted the 8263676 branch March 24, 2021 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants