[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(S): JDK-8181503 "Can't compile hotspot with c++11"
From: Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date: 2018-01-31 18:05:00
Message-ID: CAA-vtUxEeerKnAU7ifoTBsnSxy-2TaAN62ZjihmkmZAmUJ6U7A () mail ! gmail ! com
[Download RAW message or body]
On Wed, Jan 31, 2018 at 5:20 PM, <coleen.phillimore@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@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@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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic