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

Wed Jan 31 14:40:05 UTC 2018

Hi Coleen,

On Wed, Jan 31, 2018 at 3:06 PM, <coleen.phillimore at oracle.com> wrote:

>
>
> On 1/31/18 2:46 AM, Thomas Stüfe wrote:
>
>> 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.
>>
>
> What are the values in your port?  We should use the same.
>
>
  enum ErrorType {
    // internal_error are errors which are not crashes - have no
    // associated signal number or exception id.
    internal_error = 0xffffffffe0000000ULL,
    oom_error      = 0xffffffffe0000001ULL,
  };

So, we extended them to 64bit and made the corresponding _id field 64bit
unsigned. I think the reason was that we had clashes with third party DLLs
using these SEH codes. So I just extended them to 64bit to avoid clashes
with any possible Windows SEH codes.

But I do not particularly like this approach now. A cleaner fix would be to
separate signal number resp. SEH code from our own error id like this:

  enum ErrorType {
    // Crash. See _signo for signal number / SEH code.
    crash = 1,
    // internal_error are errors which are not crashes - have no
    // associated signal number or exception id.
    internal_error = 2
    oom_error      = 3
    ...
  };

and have a second member like _signo to hold the signal number on Linux,
SEH code on Windows. Ideally that one should be unsigned 32bit to hold both
worlds (int signals and DWORD SEH codes).

That way we do not have to think about intersecting value ranges of _id.
What do you think?

Kind Regards, Thomas

> Thanks,
> Coleen
>
>
>
>> 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/confl
>>>> uence/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
>>>>
>>>>
>>>>
>>>>
>>>>
>