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

Thomas Stüfe thomas.stuefe at gmail.com
Wed Jan 31 18:05:00 UTC 2018 

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

>
>
> On 1/31/18 9:40 AM, Thomas Stüfe wrote:
>
> 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?
>
>
> Yes, I agree this would be a lot better and save so much confusion.  I'm
> going to cut/paste your mail into an RFE.
>
>
Great, thanks! This was one of the things nagging me from time to time but
I never quite got around to fix it.

Thanks, Thomas

thanks,
> Coleen
>
>
> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>
>

More information about the hotspot-runtime-dev mailing list