JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Patric Hedlin
Dear all,

I would like to ask for help to review the following change/update:

Issue:  https://bugs.openjdk.java.net/browse/JDK-8172232

Webrev: http://cr.openjdk.java.net/~phedlin/tr8172232/


8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

     Subsumes (duplicate) JDK-8186579: VM_Version::platform_features()
needs update on linux-sparc.


Caveat:

     This update will introduce some redundancies into the code base,
features and definitions
     currently not used, addressed by subsequent bug or feature
updates/patches. Fujitsu HW is
     treated very conservatively.


Testing:

     JDK9/JDK10 local jtreg/hotspot


Thanks to Adrian for additional test (and review) support.

Tested-By: John Paul Adrian Glaubitz <[hidden email]>


Best regards,
Patric

Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Vladimir Kozlov
In general it is fine. Few notes.
You use ifdef DEBUG_SPARC_CAPS which is undefed at the beginning. Is it set by gcc by default?

Coding style for methods definitions - open parenthesis should be on the same line:

+  bool match(const char* s) const
+  {

Thanks,
Vladimir

On 9/29/17 6:08 AM, Patric Hedlin wrote:

> Dear all,
>
> I would like to ask for help to review the following change/update:
>
> Issue:  https://bugs.openjdk.java.net/browse/JDK-8172232
>
> Webrev: http://cr.openjdk.java.net/~phedlin/tr8172232/
>
>
> 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).
>
>      Subsumes (duplicate) JDK-8186579: VM_Version::platform_features() needs update on linux-sparc.
>
>
> Caveat:
>
>      This update will introduce some redundancies into the code base, features and definitions
>      currently not used, addressed by subsequent bug or feature updates/patches. Fujitsu HW is
>      treated very conservatively.
>
>
> Testing:
>
>      JDK9/JDK10 local jtreg/hotspot
>
>
> Thanks to Adrian for additional test (and review) support.
>
> Tested-By: John Paul Adrian Glaubitz <[hidden email]>
>
>
> Best regards,
> Patric
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Vladimir Kozlov
On 10/2/17 8:52 AM, Patric Hedlin wrote:

> Hi Vladimir,
>
>
> On 09/29/2017 08:56 PM, Vladimir Kozlov wrote:
>> In general it is fine. Few notes.
>> You use ifdef DEBUG_SPARC_CAPS which is undefed at the beginning. Is it set by gcc by default?
>>
> I have not noticed any (obvious) convention in the code base for this case, when I have a entirely (file-) local, typically debug, definition that makes no sense to define except within a particular
> file. I usually list those as undefines in the beginning of the file to make sure they are not exposed to the command line (the rationale being that they should not be of use if you are not actively
> making changes to the particular file). And it sort of works as part of the local docs.

Got it. But in such situation we have other mechanisms to print information about CPUs.
I would suggest to use unified logging currently we use for this: -Xlog:os+cpu

http://hg.openjdk.java.net/jdk10/hs/file/58931d9b2260/src/hotspot/share/runtime/vm_version.cpp#l300

There are different levels of output and for your case you can use Debug or Trace level (default is Info).

Thanks,
Vladimir

>
> Would it be an acceptable approach to add a comment like this:
>
> /* NOTE: Enable the local define 'DEBUG_LINUX_SPARC_CAPS' below (or define it
>   *       from the command line) as an aid when updating the feature table.
> #define DEBUG_LINUX_SPARC_CAPS
>   */
>
> Close to its first use (?). (I changed the name since it will be exposed outside the file.)
>
>> Coding style for methods definitions - open parenthesis should be on the same line:
>>
>> +  bool match(const char* s) const
>> +  {
>>
>
> Old habits die hard... and it's so much more readable ;)
>
> /Patric
>> Thanks,
>> Vladimir
>>
>> On 9/29/17 6:08 AM, Patric Hedlin wrote:
>>> Dear all,
>>>
>>> I would like to ask for help to review the following change/update:
>>>
>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8172232
>>>
>>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8172232/
>>>
>>>
>>> 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).
>>>
>>>      Subsumes (duplicate) JDK-8186579: VM_Version::platform_features() needs update on linux-sparc.
>>>
>>>
>>> Caveat:
>>>
>>>      This update will introduce some redundancies into the code base, features and definitions
>>>      currently not used, addressed by subsequent bug or feature updates/patches. Fujitsu HW is
>>>      treated very conservatively.
>>>
>>>
>>> Testing:
>>>
>>>      JDK9/JDK10 local jtreg/hotspot
>>>
>>>
>>> Thanks to Adrian for additional test (and review) support.
>>>
>>> Tested-By: John Paul Adrian Glaubitz <[hidden email]>
>>>
>>>
>>> Best regards,
>>> Patric
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Patric Hedlin
In reply to this post by Vladimir Kozlov
Thanks for reviewing Vladimir.

On 09/29/2017 08:56 PM, Vladimir Kozlov wrote:
> In general it is fine. Few notes.
> You use ifdef DEBUG_SPARC_CAPS which is undefed at the beginning. Is
> it set by gcc by default?
>
Removed.

> Coding style for methods definitions - open parenthesis should be on
> the same line:
>
> +  bool match(const char* s) const
> +  {
>
Updated/re-formated.

Refreshed webrev.

@Adrian: Please validate.

Best regards,
Patric

> Thanks,
> Vladimir
>
> On 9/29/17 6:08 AM, Patric Hedlin wrote:
>> Dear all,
>>
>> I would like to ask for help to review the following change/update:
>>
>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8172232
>>
>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8172232/
>>
>>
>> 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on
>> Linux).
>>
>>      Subsumes (duplicate) JDK-8186579:
>> VM_Version::platform_features() needs update on linux-sparc.
>>
>>
>> Caveat:
>>
>>      This update will introduce some redundancies into the code base,
>> features and definitions
>>      currently not used, addressed by subsequent bug or feature
>> updates/patches. Fujitsu HW is
>>      treated very conservatively.
>>
>>
>> Testing:
>>
>>      JDK9/JDK10 local jtreg/hotspot
>>
>>
>> Thanks to Adrian for additional test (and review) support.
>>
>> Tested-By: John Paul Adrian Glaubitz <[hidden email]>
>>
>>
>> Best regards,
>> Patric
>>

Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

John Paul Adrian Glaubitz
On 10/04/2017 11:04 AM, Patric Hedlin wrote:
> Refreshed webrev.
>
> @Adrian: Please validate.
Done. Both the server and the zero variant build fine on linux-sparc
with the updated webrev, hence:

Tested-By: John Paul Adrian Glaubitz <[hidden email]>

Adrian

--
  .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - [hidden email]
`. `'   Freie Universitaet Berlin - [hidden email]
   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Patric Hedlin
Thanks Adrian.

/Patric


On 10/04/2017 11:39 AM, John Paul Adrian Glaubitz wrote:

> On 10/04/2017 11:04 AM, Patric Hedlin wrote:
>> Refreshed webrev.
>>
>> @Adrian: Please validate.
> Done. Both the server and the zero variant build fine on linux-sparc
> with the updated webrev, hence:
>
> Tested-By: John Paul Adrian Glaubitz <[hidden email]>
>
> Adrian
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

John Paul Adrian Glaubitz
On 10/04/2017 11:39 AM, Patric Hedlin wrote:
> Thanks Adrian.

Thank you for your work on this :-).

Hope this gets merged soon. After that, the linux-sparc builds
won't need any external patches downstream anymore.

Adrian

--
  .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - [hidden email]
`. `'   Freie Universitaet Berlin - [hidden email]
   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

John Paul Adrian Glaubitz
Hi Patric!

On 10/04/2017 11:58 AM, John Paul Adrian Glaubitz wrote:
> Hope this gets merged soon. After that, the linux-sparc builds
> won't need any external patches downstream anymore.

Any news on this?

Thanks,
Adrian

--
  .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - [hidden email]
`. `'   Freie Universitaet Berlin - [hidden email]
   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
Reply | Threaded
Open this post in threaded view
|

Re: JDK10/RFR(M): 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on Linux).

Nils Eliasson
In reply to this post by Patric Hedlin
Thanks for fixing Patric,

Looks good!

Regards,
Nils

On 2017-10-04 11:04, Patric Hedlin wrote:

> Thanks for reviewing Vladimir.
>
> On 09/29/2017 08:56 PM, Vladimir Kozlov wrote:
>> In general it is fine. Few notes.
>> You use ifdef DEBUG_SPARC_CAPS which is undefed at the beginning. Is
>> it set by gcc by default?
>>
> Removed.
>
>> Coding style for methods definitions - open parenthesis should be on
>> the same line:
>>
>> +  bool match(const char* s) const
>> +  {
>>
> Updated/re-formated.
>
> Refreshed webrev.
>
> @Adrian: Please validate.
>
> Best regards,
> Patric
>
>> Thanks,
>> Vladimir
>>
>> On 9/29/17 6:08 AM, Patric Hedlin wrote:
>>> Dear all,
>>>
>>> I would like to ask for help to review the following change/update:
>>>
>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8172232
>>>
>>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8172232/
>>>
>>>
>>> 8172232: SPARC ISA/CPU feature detection is broken/insufficient (on
>>> Linux).
>>>
>>>      Subsumes (duplicate) JDK-8186579:
>>> VM_Version::platform_features() needs update on linux-sparc.
>>>
>>>
>>> Caveat:
>>>
>>>      This update will introduce some redundancies into the code
>>> base, features and definitions
>>>      currently not used, addressed by subsequent bug or feature
>>> updates/patches. Fujitsu HW is
>>>      treated very conservatively.
>>>
>>>
>>> Testing:
>>>
>>>      JDK9/JDK10 local jtreg/hotspot
>>>
>>>
>>> Thanks to Adrian for additional test (and review) support.
>>>
>>> Tested-By: John Paul Adrian Glaubitz <[hidden email]>
>>>
>>>
>>> Best regards,
>>> Patric
>>>
>