RFR (S) 8235765: Use of the long type should be avoided in shared code

David Holmes david.holmes at oracle.com
Fri Aug 14 23:58:03 UTC 2020 

Hi Coleen,

I suggest just leaving the elfFile stuff alone. AFAICS the underlying 
offset is 32-bit on 32-bit and 64-bit on 64-bit and on the platforms we 
use ElfFile the use of "long" matches that (ILP32 and LP64). This code 
should probably be using some of the Elf types directly, hidden by a 
typedef to hide the 32-bit versus 64-bit nomenclature.

David

On 15/08/2020 9:19 am, David Holmes wrote:
> On 13/08/2020 10:20 pm, Coleen Phillimore wrote:
>> On 8/13/20 12:07 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> And it seemed so simple when you started :)
>>
>> Oh gosh no, this sort of change is never simple!
>>>
>>> On 13/08/2020 6:43 am, Coleen Phillimore wrote:
>>>>
>>>> Hi Vladimir,  Thank you for looking at this change.
>>>>
>>>> On 8/12/20 1:25 PM, Vladimir Kozlov wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> I think it is safe to use 'uint' (uint32_t) for all counts in sweeper.
>>>
>>> "long" is only 32-bit on 64-bit Windows, so if we don't see any 
>>> issues on Windows then there is a case to be made that all these long 
>>> fields would appear to be fine if only 32-bit ... that said some of 
>>> them "obviously" look like they should be size_t as you have made them.
>>>
>>>>> What is a story about using int64_t vs jlong? And others *_t vs j* 
>>>>> types.
>>>>
>>>> jlong is a signed type (either long or long long) so in mutex, even 
>>>> > though uint64_t makes more sense, I used int64_t so that they'd be
>>>> convertible to jlong in the PlatformMutex layer.  I didn't want to 
>>>> pull the string of this sweater even further to convert the jlong to 
>>>> uint64_t in that layer. (If that's even the right thing to do).  We 
>>>> have been trying to avoid using java types like jint, jlong etc, in 
>>>> shared code, but they're pretty much everywhere.
>>>
>>> We've been avoiding unnecessary use of j* types in the VM (shared or 
>>> not) but when the values represent values to/from Java code then the 
>>> j* types are appropriate. The multiple abstractions 
>>> Parker/ParkEvent/PlatformEvent/PlatformParker/PlatformMonitor/Monitor 
>>> make it hard to see exactly how values flow through, and which ones 
>>> come direct from Java. We should keep the jlong at the Java-connected 
>>> api level and use uint64_t elsewhere. Separate RFE of course. :)
>>
>> Yes, another sort of hard-to-impossible to completely fix RFE.  I 
>> really think we should just fix individual occurrences as they're 
>> found and discourage new uses.  Ideally we should translate all j* 
>> types to their appropriate int types once they get passed into the VM 
>> implementation code.
>>>
>>>
>>> Looking at webrev v02:
>>>
>>> src/hotspot/share/memory/filemap.hpp
>>>
>>> You changed the field from long to int64_t but you didn't change the 
>>> accessor:
>>>
>>> 87   long   filesize()  const { return _filesize; }
>>
>> Rats missed one.  I wish the compiler would have complained about 
>> this. Nothing calls this function so I removed it (_filesize is used 
>> directly).
> 
> Ok.
> 
>>>
>>> that could give a truncation warning on Windows. That field is set 
>>> from the stat st_size field, which is defined as type off_t on 
>>> non-Windows and as ... okay I can't make sense of the win32 docs to 
>>> figure out whether a plain stat will be a 64-bit or 32-bit [1]. So 
>>> not clear what the right type is here - but the field and accessor 
>>> should match.
>>>
>>> [1] 
>>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions?view=vs-2019 
>>>
>>>
>>> ---
>>>
>>> src/hotspot/share/utilities/elfFile.cpp
>>>
>>> fseek is specified to take a long for the offset, so this change 
>>> would be a problem on 32-bit builds I think.
>>
>> These files aren't compiled on windows.  I compiled this on arm32. I 
>> think there "long" is 64 bits.
> 
> No "long" is 32-bit on all 32-bit platforms. So I would expect passing a 
> 64-bit value to a library function expecting a 32-bit argument should 
> generate a compiler warning at best. The fact it doesn't is a concern. 
> I'd be worried about the correctness of this code now.
> 
> David
> 
> 
>> #if !defined(_WINDOWS) && !defined(__APPLE__)
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>>
>>>>>
>>>>> Also you need to look on JFR code which collect these data:
>>>>>
>>>>> src/hotspot//share/jfr/periodic/jfrPeriodic.cpp: 
>>>>> event.set_methodReclaimedCount(NMethodSweeper::total_nof_methods_reclaimed()); 
>>>>>
>>>>>
>>>>> src/hotspot//share/jfr/metadata/metadata.xml:    <Field type="int" 
>>>>> name="methodReclaimedCount" label="Methods Reclaimed" />
>>>>>
>>>>> And I found that metadata.xml have several 'long' uses too:
>>>>>
>>>>> src/hotspot//share/jfr/metadata/metadata.xml:    <Field type="long" 
>>>>> contentType="millis" name="peakTimeSpent" label="Peak Time" />
>>>>>
>>>>> Looking on codecache code and sweeper and I see a lot of 
>>>>> inconsistencies in used types :(
>>>>> May be we need an other (compiler) RFE to clean that up.
>>>>
>>>> Yes, I agree. I'm going to revert sweeper, nmethod and vmStructs. 
>>>> It's better that 'long' is fixed individually in the sweeper and 
>>>> associated files.
>>>>
>>>> The JFR metadata.xml has a lot of "long" types declared in it. I'm 
>>>> going to revert compileBroker.* too.   This is going to have to be 
>>>> fixed a little at a time.
>>>>
>>>> I'm testing a new more limited version of this change now.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>>
>>>>> Regards,
>>>>> Vladimir K
>>>>>
>>>>> On 8/12/20 10:00 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>>
>>>>>> On 8/12/20 12:19 PM, Lois Foltan wrote:
>>>>>>> On 8/12/2020 11:21 AM, Coleen Phillimore wrote:
>>>>>>>> Summary: Changed some long declarations to uint64_t/int64_t or 
>>>>>>>> unsigned int, depending on context.
>>>>>>>>
>>>>>>>> There are still 'long' declarations left in the code, but they 
>>>>>>>> should be changed by developers when working in that code and 
>>>>>>>> not as a blanket change.  I didn't change a couple of longs in 
>>>>>>>> jfr/leakprofiler, for example. These are the ones I changed 
>>>>>>>> though with some explanation of why:
>>>>>>>>
>>>>>>>> src/hotspot/share/memory/filemap.hpp
>>>>>>>>
>>>>>>>> This can be negative so changed to int64_t.
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>>
>>>>>>>> The PlatformMutex code takes jlong, which is signed, so that's 
>>>>>>>> why I changed these to int64_t.
>>>>>>>>
>>>>>>>> runtime/interfaceSupport.inline.hpp
>>>>>>>>
>>>>>>>> These counters are actually intervals so I changed them to 
>>>>>>>> unsigned int.
>>>>>>>>
>>>>>>>> src/hotspot/share/compiler/compileBroker.hpp
>>>>>>>>
>>>>>>>> _peak_compilation_time is signed because it is compared with 
>>>>>>>> jlong which is signed.
>>>>>>>> Same with total_compilation_time - elapsedTimer.milliseconds() 
>>>>>>>> returns jlong.
>>>>>>>>
>>>>>>>> Tested with tier1-3.  Tier1 on linux-x64-debug, 
>>>>>>>> windows-x64-debug, macosx-x64-debug, linux-aarch64-debug. Also 
>>>>>>>> built on: 
>>>>>>>> linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
>>>>>>>>
>>>>>>>> open webrev at 
>>>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8235765.01/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8235765
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> Looks good.
>>>>>>>
>>>>>>> - runtime/sweeper.hpp
>>>>>>>   This is the only file that I wondered why you changed long to 
>>>>>>> int64_t for _total_nof_methods_reclaimed and 
>>>>>>> _total_nof_c2_methods_reclaimed.  Note that the method 
>>>>>>> NMethodSweeper::total_nof_methods_reclaimed returns an int. Could 
>>>>>>> both of these fields be changed to int instead?
>>>>>>
>>>>>> Hi Lois, Thank you for looking at this.  Unfortunately, this was 
>>>>>> an outdated webrev, can you hit reload?  I changed these fields to 
>>>>>> be uint64_t because they're never signed.  It's likely that the 
>>>>>> number of methods is never greater than an int, but since it was 
>>>>>> long to begin with, I kept 64 bit until someone decides an 'int' 
>>>>>> is better. Since number_of_codecache_sweeps is uint64_t, which 
>>>>>> seems like a lot too, there could be that many nmethods 
>>>>>> reclaimed.  I retested with windows just now to be sure.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Lois
>>>>>>
>>>>
>>

More information about the hotspot-dev mailing list