RFR(M): 8140594: Various minor code improvements (compiler)

[Tobias Hartmann]

Hi Goetz,

On 09.05.2016 15:11, Lindenmaier, Goetz wrote:
> Sorry for the double failure!
> I think this really works now, Christoph verified it with VS2010, thanks!
> http://cr.openjdk.java.net/~goetz/wr16/8140594-covCo/webrev.03/
> 
> I just removed prefix_len, it's added to newbuf_len, below anyways.

Looks good! The build completed on all platforms. PIT is running.

Best regards,
Tobias

> Best regards,
>   Goetz.
> 
> 
> 
>> -----Original Message-----
>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>> Sent: Montag, 9. Mai 2016 13:42
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot compiler
>> <hotspot-compiler-dev at openjdk.java.net>
>> Subject: Re: RFR(M): 8140594: Various minor code improvements (compiler)
>>
>> Hi Goetz,
>>
>> On 09.05.2016 12:30, Lindenmaier, Goetz wrote:
>>> Hi Tobias,
>>>
>>> sorry for that!  No, it's not just missing include, windows does not
>>> implement snprintf before Visual Studio 2015.
>>> http://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-
>> 2010
>>>
>>> As this is only a build-time tool, I would prefer skipping this altogether
>>> above adding windows specific code:
>>> http://cr.openjdk.java.net/~goetz/wr16/8140594-covCo/webrev.02/
>>
>> That's fine with me but unfortunately the build still fails on Windows:
>>
>> c:/jprt/T/P1/105246.tohartma/s/hotspot/src/share/vm/logging/logTagSet.cp
>> p(112) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of
>> data
>>
>> Best regards,
>> Tobias
>>>
>>> Best regrds,
>>>  Goetz.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>>>> Sent: Montag, 9. Mai 2016 11:08
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot compiler
>>>> <hotspot-compiler-dev at openjdk.java.net>
>>>> Subject: Re: RFR(M): 8140594: Various minor code improvements
>> (compiler)
>>>>
>>>> Hi Goetz,
>>>>
>>>> the build failed on Windows with:
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(721)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(723)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(728)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(730)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(740)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>> c:/jprt/T/P1/083823.tohartma/s/hotspot/src/share/vm/adlc/dfa.cpp(742)
>> :
>>>> error C3861: 'snprintf': identifier not found
>>>>
>>>> Looks like a missing include.
>>>>
>>>> Best regards,
>>>> Tobias
>>>>
>>>> On 09.05.2016 10:40, Tobias Hartmann wrote:
>>>>> Hi Goetz,
>>>>>
>>>>> thanks for the explanation, this looks good to me!
>>>>>
>>>>> I'm running the hs-comp PIT now and will push the fix afterwards, if
>> there
>>>> are no objections.
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> On 09.05.2016 10:30, Lindenmaier, Goetz wrote:
>>>>>> Hi Tobias,
>>>>>>
>>>>>>> I can sponsor your fix but since we are close to JDK 9 FC, we are not
>>>> allowed
>>>>>>> to push enhancements to hs-comp. Shouldn't this be a "bug" anyway?
>>>>>> Thanks for reviewing and the offer to sponsor!
>>>>>> I changed the Jira issue to bug.  Sorry for being that close to FC with my
>>>> RFR.
>>>>>>
>>>>>>>> - size of pointer passed to jio_snprintf()
>>>>>>> Please also fix the indentation in line 6004.
>>>>>> Fixed.
>>>>>>
>>>>>>>> classLoader.cpp
>>>>>>>> - jio_snprintf does null termination. But it might return -1 if
>> truncated,
>>>>>>>>   in this case array access at -1.
>>>>>>> You can remove "int n;"
>>>>>> Fixed.
>>>>>>
>>>>>>>> generateOopMap.cpp
>>>>>>>> - Remaining fields not initialized.
>>>>>>> I would put the loop bodies in a new line.
>>>>>> Fixed.
>>>>>>
>>>>>>>> relocator.cpp
>>>>>>>> - delta might be -4 ... assert returns.
>>>>>>> I don't understand this change. If delta is -4, the assert in the baseline
>>>> version
>>>>>>> is triggered. With your fix, the assert is triggered as well.
>>>>>>
>>>>>> Sorry, that's a bad comment.  I meant that the assert does not
>>>>>> protect against getting -4 in that line, because coverity knows
>>>>>> about -XX:SuppressErrorAt.  But that's pointless, as assert goes
>>>>>> away in product builds anyways.
>>>>>>
>>>>>> The assert shall be triggered, therefore I moved it out of the if().
>>>>>> But the if() now protects against oob with -4 or smaller.
>>>>>>
>>>>>> I also rebased the change, and added you as reviewer.  Find the new
>>>>>> Webrev here:
>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8140594-covCo/webrev.01/
>>>>>> And the incremental patch:
>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8140594-
>>>> covCo/webrev.01/incremental_diff.patch
>>>>>>
>>>>>> Best regards,
>>>>>>   Goetz.
>>>>>>