8203287: Zero fails to build after JDK-8199712 (Flight Recorder)

[John Paul Adrian Glaubitz]

I agree. Please don’t break portability of Zero here.

I haven’t had the time to look into this yet, but I will try to catch up the next days and see where I can help.

Adrian

> On May 18, 2018, at 11:24 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Severin,
> 
> It sounds like parts of os_perf should be conditional on INCLUDE_JFR (which I think replaced INCLUDE_TRACE). But I haven't been able to look at the Flight Recorder changes in detail.
> 
> You obviously need to do what you need to move forward but something seems wrong here.
> 
> David
> 
>> On 18/05/2018 6:20 PM, Severin Gehwolf wrote:
>> Hi David,
>>> On Fri, 2018-05-18 at 14:17 +1000, David Holmes wrote:
>>> It doesn't look right to me that vm_version_ext_zero.cpp is zero
>>> specific - it really should be CPU specific. The zero version is setting
>>> up a bunch of dummy values for threads/cores/sockets etc, but if
>>> something else uses them in calculations then you'd get some weird numbers.
>> Right. Do you expect this to be used outside JFR?
>> Note that Zero is supposed to be buildable on a wide range of
>> architectures. Basically all that have GCC and libffi. That would mean
>> we'd have to add CPU specific files for all such architectures.
>> Examples would be mips, ppc (32bit), s390 (31 bit) and others. It seems
>> more reasonable to add a zero specific version of it and produce dummy
>> values than requiring one to add CPU specific vm_version_ext files on
>> architectures where one happens to build Zero on just to get it to
>> build. What's more, it only seems to be used by JFR.
>> The trouble comes from os_perf_linux.cpp which got added in shared code
>> for JFR. I don't expect JFR to work on Zero, TBH. It seems not worth
>> having for Zero. But that's for later. Right now, the Zero build is
>> broken and we need to get it fixed. FWIW, I've tried this to disable
>> the jfr feature by default on Zero, but that still attempts to build
>> os_perf_linux.cpp:
>> diff --git a/make/autoconf/hotspot.m4 b/make/autoconf/hotspot.m4
>> --- a/make/autoconf/hotspot.m4
>> +++ b/make/autoconf/hotspot.m4
>> @@ -309,9 +309,11 @@
>>      AC_MSG_ERROR([Specified JVM feature 'cmsgc' requires feature 'serialgc'])
>>    fi
>>  -  # Enable JFR by default, except on linux-sparcv9 and on minimal.
>> -  if test "x$OPENJDK_TARGET_OS" != xlinux || test "x$OPENJDK_TARGET_CPU" != xsparcv9; then
>> -    NON_MINIMAL_FEATURES="$NON_MINIMAL_FEATURES jfr"
>> +  # Enable JFR by default, except for Zero, linux-sparcv9 and on minimal.
>> +  if test "x$HOTSPOT_TARGET_CPU" != xzero; then
>> +    if test "x$OPENJDK_TARGET_OS" != xlinux || test "x$OPENJDK_TARGET_CPU" != xsparcv9; then
>> +      NON_MINIMAL_FEATURES="$NON_MINIMAL_FEATURES jfr"
>> +    fi
>>    fi
>>      # Turn on additional features based on other parts of configure
>> Thoughts?
>> Thanks,
>> Severin
>>> David
>>> 
>>>> On 18/05/2018 12:38 AM, Severin Gehwolf wrote:
>>>> Hi Martin, Aleksey,
>>>> 
>>>> Ugh, sorry, my mistake :-/ Thanks for the reviews!
>>>> 
>>>> This is what I'm going to push once jdk-submit comes back clean:
>>>> 
>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203287/webrev.04/
>>>> 
>>>> Thanks,
>>>> Severin
>>>> 
>>>>> On Thu, 2018-05-17 at 14:17 +0000, Doerr, Martin wrote:
>>>>> Hi Severin,
>>>>> 
>>>>> looks good to me with Aleksey's patch. Thanks for waiting.
>>>>> 
>>>>> Best regards,
>>>>> Martin
>>>>> 
>>>>> 
>>>>> -----Original Message-----
>>>>> From: Aleksey Shipilev [mailto:shade at redhat.com]
>>>>> Sent: Donnerstag, 17. Mai 2018 16:14
>>>>> To: Severin Gehwolf <sgehwolf at redhat.com>; Doerr, Martin <martin.doerr at sap.com>; hotspot-dev <hotspot-dev at openjdk.java.net>
>>>>> Subject: Re: 8203287: Zero fails to build after JDK-8199712 (Flight Recorder)
>>>>> 
>>>>>> On 05/17/2018 04:02 PM, Severin Gehwolf wrote:
>>>>>>> On Thu, 2018-05-17 at 15:27 +0200, Aleksey Shipilev wrote:
>>>>>>>> On 05/17/2018 03:12 PM, Severin Gehwolf wrote:
>>>>>>>> Hi Martin, Aleksey,
>>>>>>>> 
>>>>>>>> Since JDK-8203288 has been pushed here is the latest webrev for Zero.
>>>>>>>> It no longer contains changes in os_perf_linux.cpp.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203287/webrev.02/
>>>>>>>> 
>>>>>>>> How does it look?
>>>>>>> 
>>>>>>> Looks good, but I used parentheses inconsistently with the rest of Hotspot in the original patch:
>>>>>>> 
>>>>>>>   #if (defined X86 && !defined ZERO)
>>>>>>> 
>>>>>>> Should be:
>>>>>>> 
>>>>>>>   #if defined(X86) && !defined (ZERO)
>>>>>> 
>>>>>> OK. Here it goes:
>>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203287/webrev.03/
>>>>> 
>>>>> Notice the parentheses, it is done like that in other places:
>>>>> 
>>>>>   #if defined(X86) && !defined(ZERO)
>>>>> 
>>>>> Apply this patch to correct:
>>>>>    http://cr.openjdk.java.net/~shade/8203287/parent.patch
>>>>> 
>>>>> I checked it builds with x86_64 and Zero.
>>>>> 
>>>>> -Aleksey
>>>>>