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

[David Holmes]

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
>>>>