RFR(S): JDK-8181503 "Can't compile hotspot with c++11"

[Gerard Ziemski]

Thank you!

> On Jan 31, 2018, at 3:13 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Jan 30, 2018, at 1:26 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>> 
>> Hi,
>> 
>> Please review the fixes for the following issues when compiling on Xcode9.2 with c++14. There are 5 unique issues fixed here:
>> 
>> 
>> #1 "error: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wreserved-user-defined-literal]” 
>> 
>> Example:
>> 
>>  in: src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp
>> from: __asm__("mov %%"SPELL_REG_SP", %0":"=r"(esp));
>>  to: __asm__("mov %%" SPELL_REG_SP ", %0":"=r"(esp));
>> 
>> 
>> #2 "error: comparison between pointer and integer ('address' (aka 'unsigned char *') and 'int’)"
>> 
>>  in: src/hotspot/share/code/compiledIC.cpp
>> from: if (entry == false)
>>  to: if (entry == NULL)
>> 
>> 
>> #3 "error: case value evaluates to 3758096384, which cannot be narrowed to type 'int' [-Wc++11-narrowing]”
>> 
>> This one requires an explanation: in “debug.hpp" we have:
>> 
>>  enum VMErrorType {
>>    INTERNAL_ERROR   = 0xe0000000,
>>    OOM_MALLOC_ERROR = 0xe0000001,
>>    OOM_MMAP_ERROR   = 0xe0000002
>>  };
>> 
>> With “VMErrorType” being of type “unsigned int”, but in “vmError.hpp” we have:
>> 
>>  class VMError : public AllStatic {
>>  ...
>>    static int         _id;
>>  ...
>> 
>> So, with this code in “vmError.cpp”:
>> 
>>     switch(_id) {
>>       case OOM_MALLOC_ERROR:
>>       case OOM_MMAP_ERROR:
>> 	 ...
>>         break;
>>       case INTERNAL_ERROR:
>>       default:
>>         break;
>>     }
>> 
>> We get the error in the switch statement, because "unsigned int" value like 0xe0000000, doesn’t fit in signed “int”. The easiest and simplest way to fix this that I propose is to change the switch statement to:
>> 
>>  switch(static_cast<unsigned int>(_id)) {
>> 
>> There are other solutions, but they require much more pervasive changes.
>> 
>> We can’t easily change the “_id” type itself to “unsigned int”, because all of the platforms, except Windows, use signed “int”, so we would need to touch lots of code and then cast to “int” eventually.
>> 
>> We also we can’t cast INTERNAL_ERROR, OOM_MALLOC_ERROR and OOM_MMAP_ERROR to “int", ex:
>> 
>>  enum VMErrorType {
>>    INTERNAL_ERROR   = (int)0xe0000000,
>>    OOM_MALLOC_ERROR = (int)0xe0000001,
>>    OOM_MMAP_ERROR   = (int)0xe0000002
>>  };
>> 
>> Because we have code like:
>> 
>>  static bool should_report_bug(unsigned int id) {
>>    return (id != OOM_MALLOC_ERROR) && (id != OOM_MMAP_ERROR);
>>  }
>> 
>> Where we use “unsigned int” to compare against the errors, so we would get signed vs unsigned comparison warning, so again more code to touch.
>> 
>> 
>> #4 warning: explicit instantiation of 'arraycopy_conjoint<signed char>' that occurs after an explicit specialization has no effect [-Winstantiation-after-specialization] 
>> 
>> I’m not 100% sure about this, but I propose that we simply remove the explicit instantiations, i.e. simply delete lines like:
>> 
>> template void AccessInternal::arraycopy_conjoint<jbyte>(jbyte* src, jbyte* dst, size_t length); 
>> 
>> 
>> #5 warning: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Wvarargs] 
>> 
>> This one requires an explanation: in “logConfiguration.cpp" we have:
>> 
>>  void LogConfiguration::configure_stdout(LogLevelType level, bool exact_match, ...) {
>>    ...
>>    va_start(ap, exact_match);
>> 
>> According to https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start the 2nd argument to the “va_start” macro must have the same argument type as the one after default argument promotion, and in this case we have “bool” that gets promoted to “int”, so the types are mismatched.
>> 
>> The proposed fix is to change the type of “exact_match” from “bool” to “int”, which requires changing it in 3 places.
>> 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8181503
>> http://cr.openjdk.java.net/~gziemski/8181503_rev1
>> 
>> Passes mach5 hs-tier1,2,3
>> 
>> 
>> cheers
> 
> Looks good.
> 
> I think this solution to #3 is better than the previous proposal from back in June.
> 
> #5 is quite unfortunate; I don’t see a better solution.
> 
> I spent some time studying #4, reading relevant parts of the standard.  I think the change is okay,
> but can’t yet point to specific text to justify that.  I was amused to run across this gem again:
> 
> 14.7.3/7
> The placement of explicit specialization declarations … can affect whether a program is
> well-formed according to the relative positioning of the explicit specialization declarations and their points of
> instantiation in the translation unit as specified above and below. When writing a specialization, be careful
> about its location; or to make it compile will be such a trial as to kindle its self-immolation.
> 
> 
>