RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[Jini George]

Thank you very much, Chris, for taking a look.

Yes, I have the arch specific check since VM_Version::CPU_SHA is cpu 
specific. I chose one platform independent long constant -- 
"markOopDesc::hash_mask_in_place". This is the only platform independent 
long Constant with which the truncation issue can be observed. I added a 
platform dependent long constant also to get some extra testing done.

I will add the comments for the new code.

Thank you,
Jini.

On 5/12/2018 6:30 AM, Chris Plummer wrote:
> Hi Jini,
> 
> Why do you have the following:
> 
>    98         if (arch.equals("amd64") || arch.equals("i386") || 
> arch.equals("x86")) {
> 
> Is it because VM_Version::CPU_SHA is only expected on these platforms? 
> If so, is there a reason you didn't choose a long constant that exists 
> on all platforms?
> 
> And just one minor nit. I think the new code warrants some comments 
> indicating what it is trying to test for.
> 
> thanks,
> 
> Chris
> 
> On 5/11/18 5:11 AM, Jini George wrote:
>> Thank you, David. Could I get another pair of eyes to take a look at:
>>
>> http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
>>
>> Thanks,
>> Jini.
>>
>> On 5/11/2018 11:58 AM, David Holmes wrote:
>>> Forgot to add - no need to see an updated webrev.
>>>
>>> Thanks,
>>> David
>>>
>>> On 11/05/2018 4:22 PM, David Holmes wrote:
>>>> Hi Jini,
>>>>
>>>> On 11/05/2018 5:34 AM, Jini George wrote:
>>>>> Hi David,
>>>>>
>>>>> Thank you very much for the review and for the clarifications 
>>>>> offline! I have addressed your comments and have a new webrev at:
>>>>>
>>>>> http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
>>>>
>>>> Looks fine.
>>>>
>>>> A couple of minor comments on the test:
>>>>
>>>> You don't need:
>>>>
>>>>    79                 LingeredApp.stopApp(theApp);
>>>>
>>>> as it is done in the finally block.
>>>>
>>>> Can you add comments in checkForTruncation stating where the 
>>>> expected values were obtained from.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Jini.
>>>>>
>>>>>
>>>>> On 5/4/2018 12:23 PM, David Holmes wrote:
>>>>>> Hi Jini,
>>>>>>
>>>>>> On 4/05/2018 2:17 AM, Jini George wrote:
>>>>>>> Hello!
>>>>>>>
>>>>>>> Requesting reviews for:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8195613
>>>>>>>
>>>>>>> ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to 
>>>>>>> int)
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
>>>>>>
>>>>>> Actual fix seems fine.
>>>>>>
>>>>>> I have a few comments on the test ...
>>>>>>
>>>>>> - Why are you only testing on 64-bit? The truncation would happen 
>>>>>> on 32-bit as well.
>>>>>>
>>>>>> - CheckForTruncation seems rather complicated, not sure why we 
>>>>>> need to look for two different things with one being amd64 specific??
>>>>>>
>>>>>> - We've had discussions in the past about splitting strings on \n 
>>>>>> or \r\n depending on whether it's Windows or not. Better to use 
>>>>>> String.split("\R") regex function.
>>>>>>
>>>>>> - Exception message strings should not contain newline characters. 
>>>>>> Also you can simplify them:
>>>>>>
>>>>>> Reading XXX: got value NNN but expected MMM
>>>>>>
>>>>>> And you don't need to define a local variable "String message" you 
>>>>>> can just:
>>>>>>
>>>>>> throw new Exception("Reading XXX: got value " + value + " but 
>>>>>> expected " + expected);
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> Testing: The SA tests passed on Mach5.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jini.
>>>>>>>
>>>>>>>
> 
>