[9] RFR: 8044538: assert(which != imm_operand) failed: instruction is not a movq reg, imm64

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 6/10/14 8:04 AM, Tobias Hartmann wrote:
> Hi Chris,
>
>> Your @run command uses the wrong class name:
>>
>>    28  * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp -XX:+PrintRelocations Test8044538
>>    35 public class TestPrintRelocations {
>
> Thanks, I missed that while renaming. Should have tested it..
>
> New webrev: http://cr.openjdk.java.net/~thartmann/8044538/webrev.04/
>
> Best,
> Tobias
>
>>
>> On Jun 9, 2014, at 11:10 PM, Tobias Hartmann <tobias.hartmann at oracle.com <mailto:tobias.hartmann at oracle.com>> wrote:
>>
>>>>>
>>>>> Hi Chris,
>>>>>
>>>>>>> Hi Vladimir,
>>>>>>>
>>>>>>> thanks for the review.
>>>>>>>
>>>>>>>> Looks good. PrintRelocations flag is used very rare, that is why it rotted. Please, add jreg test (use
>>>>>>>> -XX:+IgnoreUnrecognizedVMOptions).
>>>>>>> I added a jtreg test to test/compiler/8044538/ that uses -Xcomp and -XX:+PrintRecolations. It fails without my
>>>>>>> changes.
>>>>>> We are trying to not use bug id directories anymore.  Please move the test to a more meaningful directory with a
>>>>>> meaningful name.
>>>>> Okay, makes sense. I moved the test to test/compiler/PrintRelocations/TestPrintRelocations.java
>>>> Thanks but that’s too fine grained.  I doubt there will be any other PrintRelocations tests.  So maybe
>>>> test/compiler/relocations/ makes more sense.
>>>
>>> Okay, I moved it to test/compiler/relocations.
>>>
>>> New webrev:http://cr.openjdk.java.net/~thartmann/8044538/webrev.03/
>>> <http://cr.openjdk.java.net/%7Ethartmann/8044538/webrev.03/>
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>
>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8044538/webrev.02/
>>>>> <http://cr.openjdk.java.net/%7Ethartmann/8044538/webrev.02/>
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8044538/webrev.01/
>>>>>>> <http://cr.openjdk.java.net/%7Ethartmann/8044538/webrev.01/>
>>>>>>>
>>>>>>>> Since this debug flag almost never used backporting the fix only into 8u may be enough.
>>>>>>> Okay.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tobias
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 6/4/14 1:30 AM, Tobias Hartmann wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> please review the following patch for bug 8044538.
>>>>>>>>>
>>>>>>>>> *Problem*
>>>>>>>>> Executing a debug build of HotSpot with the flags -XX:+PrintRelocations
>>>>>>>>> -Xcomp hits a ShouldNotReachHere() or an assert in
>>>>>>>>> Assembler::locate_operand(..) stating that the instruction for which we
>>>>>>>>> try to find the operand is not valid.
>>>>>>>>>
>>>>>>>>> The problem occurs while printing the relocation entries for a C2
>>>>>>>>> compiled function. The C2 compiler adds internal_word_type relocations
>>>>>>>>> for the jump table entries in the constant section of a method (see
>>>>>>>>> Compile::ConstantTable::fill_jump_table(...)). These relocations are
>>>>>>>>> processed by RelocIterator::print_current(...) and
>>>>>>>>> internal_word_Relocation::target().
>>>>>>>>> Relocation::pd_get_address_from_code() then tries to retrieve the
>>>>>>>>> address from an instruction but fails because the relocation points into
>>>>>>>>> the constant section only containing the target address.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8044538
>>>>>>>>>
>>>>>>>>> *Solution*
>>>>>>>>> The implementation of internal_word_Relocation::target() is changed to
>>>>>>>>> check if the relocation points into the constant section and if so
>>>>>>>>> directly returns the target address instead of trying to retrieve it
>>>>>>>>> from an instruction. The same is already done in
>>>>>>>>> internal_word_Relocation::fix_relocation_after_move(..).
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8044538/webrev.00/
>>>>>>>>> <http://cr.openjdk.java.net/%7Ethartmann/8044538/webrev.00/>
>>>>>>>>>
>>>>>>>>> *Tests*
>>>>>>>>> Failing configuration, JPRT
>>>>>>>>>
>>>>>>>>> Apparently this did not show up for any of our tests. Do we need an
>>>>>>>>> additional test for this?
>>>>>>>>> Since it already fails for JDK 7 and 8. Should we backport the patch?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Tobias
>>
>