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

[Thomas Stüfe]

On Wed, Jan 31, 2018 at 2:07 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Gerard,
>
> These seem okay.
>
> I'm a little surprised that the LogConfiguration change doesn't cause
> complaints about passing true/false as int ??
>
> And I second Coleen's query re the VMError codes - there seems to be no
> reason for their strange values. They were added by JDK-4965918.
>
>

They look Windows-ish, like someone wanted them to mimic SEH codes - like
someone took them directly from the examples here:
https://msdn.microsoft.com/en-us/library/het71c37.aspx.

They bothered us for some reason I do not really remember anymore, so in
our port they long have had different values, with no adverse effects.

Regards, Thomas


> Thanks,
> David
>
>
> On 31/01/2018 4:26 AM, Gerard Ziemski 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 <
>> 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 <
>> https://bugs.openjdk.java.net/browse/JDK-8181503>
>> http://cr.openjdk.java.net/~gziemski/8181503_rev1 <
>> http://cr.openjdk.java.net/~gziemski/8181503_rev1>
>>
>> Passes mach5 hs-tier1,2,3
>>
>>
>> cheers
>>
>>
>>
>>