Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Mon, 7 Dec 2020 15:50:55 GMT, Harold Seigel wrote: >> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). >> >> Development has been broken into 5 tasks, each with its own JBS issue: >> - Deprecate wrapper class constructors for removal (rriggs) >> - Revise "value-based class" & apply to wrappers (dlsmith) >> - Define & apply annotation jdk.internal.ValueBased (rriggs) >> - Add 'lint' warning for @ValueBased classes (sadayapalam) >> - Diagnose synchronization on @ValueBased classes (lfoltan) >> >> All changes have been previously reviewed and integrated into the [`jep390` >> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` >> repository. See the subtasks of the 5 JBS issues for these changes, >> including discussion and links to reviews. (Reviewers: mchung, dlsmith, >> jlaskey, rriggs, lfoltan, fparain, hseigel.) >> >> CSRs have also been completed or are nearly complete: >> >> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for >> wrapper class constructor deprecation >> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for >> revisions to "value-based class" >> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new >> `javac` lint warnings >> >> Here's an overview of the files changed: >> >> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to >> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances >> previous work that produced such diagnostics for the primitive wrapper >> classes. >> >> - `src/java.base/share/classes/java/lang`: deprecating for removal the >> wrapper class constructors; revising the definition of "value-based class" >> in `ValueBased.html` and the description used by linking classes; applying >> "value-based class" to the primitive wrapper classes; marking value-based >> classes with `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/java/lang/constant`: no longer designating >> these classes as "value-based", since they rely heavily on field inheritance. >> >> - `src/java.base/share/classes/java/time`: revising the description used to >> link to `ValueBased.html`; marking value-based classes with >> `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/java/util`: revising the description used to >> link to `ValueBased.html`; marking value-based classes with >> `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/jdk/internal`: define the >> `@jdk.internal.ValueBased` annotation. >> >> - `src/java.management.rmi`: removing uses of wrapper class constructors. >> >> - `src/java.xml`: removing uses of wrapper class constructors. >> >> - `src/jdk.compiler`: implementing the `synchronization` lint category, >> which reports attempts to synchronize on classes and interfaces annotated >> with `@jdk.internal.ValueBased`. >> >> - `src/jdk.incubator.foreign`: revising the description used to link to >> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a >> special module export, which was not deemed necessary for an incubating API.) >> >> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and >> synchronization warnings in tests >> >> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics >> >> - `test`: testing new `javac` and HotSpot behavior; removing usages of >> deprecated wrapper class constructors from tests, or suppressing deprecation >> warnings; revising the description used to link to `ValueBased.html`. > > The hotspot changes look good. In a future change, could you add additional > classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. > Currently, it only tests primitive classes. @hseigel thank you for the review. I have created https://bugs.openjdk.java.net/browse/JDK-8257836 as an RFE to address additional testing. Lois - PR: https://git.openjdk.java.net/jdk/pull/1636
Re: (11) RFR (S) JDK-8198640: VS2017 (LNK4281) Link Warning Against Missed ASLR Optimization
Thanks Erik! Lois On 2/23/2018 4:08 PM, Erik Joelsson wrote: Looks good. /Erik On 2018-02-23 12:11, Lois Foltan wrote: Please review this fix to ignore linker warning (LNK4281). This is a new linker warning generated by VS2017 v15.5 to "to point out any 64-bit image specified to link with a lower than 4GB base address doesn't get best ASLR optimization". The Hotspot jvm.dll is specifically linked with -base:0x800. As recommended by https://developercommunity.visualstudio.com/content/problem/160970/upgrading-from-154-to-155-throw-lnk4281-warning.html, this linker warning can be suppressed with -ignore. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198640/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198640 Testing (hs-tier1-3 & jdk-tier1-3) in progress Thanks, Lois
Re: (11) RFR (S) JDK-8198640: VS2017 (LNK4281) Link Warning Against Missed ASLR Optimization
Thank you for the review Christian! I have gone forward and created the RFE at https://bugs.openjdk.java.net/browse/JDK-8198652. Lois On 2/23/2018 3:54 PM, Christian Tornqvist wrote: Forgot to say that the webrev looks good! On Feb 23, 2018, at 3:53 PM, Christian Tornqvist <christian.tornqv...@oracle.com> wrote: Sounds like a good plan :) Thanks, Christian On Feb 23, 2018, at 3:22 PM, Lois Foltan <lois.fol...@oracle.com> wrote: On 2/23/2018 3:18 PM, Christian Tornqvist wrote: Hi Lois, Why do we link jvm.dll with -base? Hi Christian, It is not clear to me why we do, so I was going to follow up with an RFE to investigate & suggest the removal of -base if unnecessary. Lois Thanks, Christian On Feb 23, 2018, at 3:11 PM, Lois Foltan <lois.fol...@oracle.com> wrote: Please review this fix to ignore linker warning (LNK4281). This is a new linker warning generated by VS2017 v15.5 to "to point out any 64-bit image specified to link with a lower than 4GB base address doesn't get best ASLR optimization". The Hotspot jvm.dll is specifically linked with -base:0x800. As recommended by https://developercommunity.visualstudio.com/content/problem/160970/upgrading-from-154-to-155-throw-lnk4281-warning.html, this linker warning can be suppressed with -ignore. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198640/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198640 Testing (hs-tier1-3 & jdk-tier1-3) in progress Thanks, Lois
Re: (11) RFR (S) JDK-8198640: VS2017 (LNK4281) Link Warning Against Missed ASLR Optimization
On 2/23/2018 3:18 PM, Christian Tornqvist wrote: Hi Lois, Why do we link jvm.dll with -base? Hi Christian, It is not clear to me why we do, so I was going to follow up with an RFE to investigate & suggest the removal of -base if unnecessary. Lois Thanks, Christian On Feb 23, 2018, at 3:11 PM, Lois Foltan <lois.fol...@oracle.com> wrote: Please review this fix to ignore linker warning (LNK4281). This is a new linker warning generated by VS2017 v15.5 to "to point out any 64-bit image specified to link with a lower than 4GB base address doesn't get best ASLR optimization". The Hotspot jvm.dll is specifically linked with -base:0x800. As recommended by https://developercommunity.visualstudio.com/content/problem/160970/upgrading-from-154-to-155-throw-lnk4281-warning.html, this linker warning can be suppressed with -ignore. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198640/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198640 Testing (hs-tier1-3 & jdk-tier1-3) in progress Thanks, Lois
(11) RFR (S) JDK-8198640: VS2017 (LNK4281) Link Warning Against Missed ASLR Optimization
Please review this fix to ignore linker warning (LNK4281). This is a new linker warning generated by VS2017 v15.5 to "to point out any 64-bit image specified to link with a lower than 4GB base address doesn't get best ASLR optimization". The Hotspot jvm.dll is specifically linked with -base:0x800. As recommended by https://developercommunity.visualstudio.com/content/problem/160970/upgrading-from-154-to-155-throw-lnk4281-warning.html, this linker warning can be suppressed with -ignore. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198640/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198640 Testing (hs-tier1-3 & jdk-tier1-3) in progress Thanks, Lois
Re: (11) RFR (S) JDK-8198312: VS2017: Upgrade HOTSPOT_BUILD_COMPILER in vm_version.cpp
On 2/23/2018 2:31 PM, Erik Joelsson wrote: On 2018-02-23 11:16, Lois Foltan wrote: On 2/23/2018 1:05 PM, Erik Joelsson wrote: Hello Lois, This looks good, but I would suggest to also add 1900 for VS2015, for completeness. Thanks for the review Erik! I have updated the webrev to add 1900, however, I couldn't find a release # for VS2015, since all documentation I could find seemed to indicated that only 2015 and updates 1-3 were released. If you have more info on this let me know! My installation of 2015 was put in "Microsoft Visual Studio 14.0" following the pattern of previous versions (12.0, 11.0, 10.0 etc), so I think that would be the appropriate number here. Otherwise I think this looks good. Got it, hopefully final webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312.2/webrev/ Thanks again! Lois /Erik http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312.1/webrev/ Thanks, Lois /Erik On 2018-02-23 09:54, Lois Foltan wrote: Please review this small fix to set HOTSPOT_BUILD_COMPILER correctly for VS2017. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198312 Testing: hs-tier(1-3), jdk-tier(1-3) complete Thanks, Lois
Re: (11) RFR (S) JDK-8198312: VS2017: Upgrade HOTSPOT_BUILD_COMPILER in vm_version.cpp
Thanks again for the review Erik! Lois On 2/23/2018 2:44 PM, Erik Joelsson wrote: Looks good! /Erik On 2018-02-23 11:39, Lois Foltan wrote: On 2/23/2018 2:31 PM, Erik Joelsson wrote: On 2018-02-23 11:16, Lois Foltan wrote: On 2/23/2018 1:05 PM, Erik Joelsson wrote: Hello Lois, This looks good, but I would suggest to also add 1900 for VS2015, for completeness. Thanks for the review Erik! I have updated the webrev to add 1900, however, I couldn't find a release # for VS2015, since all documentation I could find seemed to indicated that only 2015 and updates 1-3 were released. If you have more info on this let me know! My installation of 2015 was put in "Microsoft Visual Studio 14.0" following the pattern of previous versions (12.0, 11.0, 10.0 etc), so I think that would be the appropriate number here. Otherwise I think this looks good. Got it, hopefully final webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312.2/webrev/ Thanks again! Lois /Erik http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312.1/webrev/ Thanks, Lois /Erik On 2018-02-23 09:54, Lois Foltan wrote: Please review this small fix to set HOTSPOT_BUILD_COMPILER correctly for VS2017. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198312 Testing: hs-tier(1-3), jdk-tier(1-3) complete Thanks, Lois
Re: (11) RFR (S) JDK-8198312: VS2017: Upgrade HOTSPOT_BUILD_COMPILER in vm_version.cpp
On 2/23/2018 1:05 PM, Erik Joelsson wrote: Hello Lois, This looks good, but I would suggest to also add 1900 for VS2015, for completeness. Thanks for the review Erik! I have updated the webrev to add 1900, however, I couldn't find a release # for VS2015, since all documentation I could find seemed to indicated that only 2015 and updates 1-3 were released. If you have more info on this let me know! http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312.1/webrev/ Thanks, Lois /Erik On 2018-02-23 09:54, Lois Foltan wrote: Please review this small fix to set HOTSPOT_BUILD_COMPILER correctly for VS2017. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198312 Testing: hs-tier(1-3), jdk-tier(1-3) complete Thanks, Lois
(11) RFR (S) JDK-8198312: VS2017: Upgrade HOTSPOT_BUILD_COMPILER in vm_version.cpp
Please review this small fix to set HOTSPOT_BUILD_COMPILER correctly for VS2017. open webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8198312/webrev/ bug link https://bugs.openjdk.java.net/browse/JDK-8198312 Testing: hs-tier(1-3), jdk-tier(1-3) complete Thanks, Lois
RFR (XS) JDK-8054355: ENFORCE_CC_COMPILER_REV needs to be updated to Solaris C++ 12u3 for JDK 9
Hi Everyone, Please review the following fix: Webrev: http://cr.openjdk.java.net/~lfoltan/bug_jdk8054355/ Bug: ENFORCE_CC_COMPILER_REV needs to be updated to Solaris C++ 12u3 for JDK 9 https://bugs.openjdk.java.net/browse/JDK-8054355 Summary of fix: Update of C++ validation check for JDK 9 on Solaris. Testing: Built various variants with Solaris C++ 12u3 to confirm that the warning concerning C++ version is no longer generated. Also built with versions other than 12u3 to further confirm that the warning is generated in those situations. Other testing in progress - jprt build to confirm builds are successful, quick run of Hotspot jtreg test/runtime in progress
Re: RFR (XS) JDK-8054355: ENFORCE_CC_COMPILER_REV needs to be updated to Solaris C++ 12u3 for JDK 9
Thanks Vladimir! Lois On 8/22/2014 3:01 PM, Vladimir Kozlov wrote: Good. Thanks, Vladimir On 8/22/14 11:35 AM, Lois Foltan wrote: Hi Everyone, Please review the following fix: Webrev: http://cr.openjdk.java.net/~lfoltan/bug_jdk8054355/ Bug: ENFORCE_CC_COMPILER_REV needs to be updated to Solaris C++ 12u3 for JDK 9 https://bugs.openjdk.java.net/browse/JDK-8054355 Summary of fix: Update of C++ validation check for JDK 9 on Solaris. Testing: Built various variants with Solaris C++ 12u3 to confirm that the warning concerning C++ version is no longer generated. Also built with versions other than 12u3 to further confirm that the warning is generated in those situations. Other testing in progress - jprt build to confirm builds are successful, quick run of Hotspot jtreg test/runtime in progress
Re: RFR: 8030350: Enable additional compiler warnings for GCC
Hi Mike, We need to be careful that these options are also supported on MacOS with the clang compiler in case there is a future migration to clang for JDK 9 or higher. Thanks, Lois On 12/17/2013 7:08 PM, Mike Duigou wrote: Hello all; This is a change which enables additional compiler warnings for native compilation when using GCC. The (-Wformat -Wformat-security) options are supported by GCC 3.0.4 (the earliest version I checked, c. February 2002) and later so we shouldn't see issues with incompatibility.- Wextra appears to have been added in GCC 3.4.X line (c. 2004) so it should also be reasonably well adopted and replaces -W. The core of the change is to add : -Wextra -Wno-unused-parameter -Wformat -Wformat-security for general C and CC++ compilations. For HotSpot C++ compiles a slightly less aggressive set is used: -Wformat -Wformat-security is used. Webrev here: http://cr.openjdk.java.net/~mduigou/JDK-8030350/0/webrev/ For the curious, yes, the additional checks do generate additional warnings. ;-) This change is targeted at the JDK 9 repos but could be backported to JDK 8 fairly easily/safely. Mike
RFR (XS) JDK-8025569: -XX:+CheckUnhandledOops crashes on Windows
Please review the following fix: Webrev: http://cr.openjdk.java.net/~coleenp/bug_jdk8025569/ Bug: -XX:+CheckUnhandledOops crashes on Windows https://bugs.openjdk.java.net/browse/JDK-8025569 Summary of fix: Until JDK-8024364, https://bugs.openjdk.java.net/browse/JDK-8024364 (Win/x64: os::current_frame() and os::fetch_frame_from_context() returns wrong frame pointer), is fixed, -XX:+CheckUnhandledOops can not be supported for Windows fastdebug builds due to the need to have a legitimate frame returned from os::current_frame() early on in universe_post_init() to register deregister oops. Tests: JTREG on Windows (in progress) Thank you, Lois
Re: FR (L) JDK-7195622 (round 2): CheckUnhandledOops has limited usefulness now
On 9/25/2013 3:06 PM, Christian Thalinger wrote: This looks good. Thank you for changing it to template methods instead of using macros. Great, thank you for taking the time to look at this given your week at JavaOne. Lois On Sep 23, 2013, at 2:17 PM, Lois Foltan lois.fol...@oracle.com wrote: Please review the following updated fix which has removed the CAST_*_OOP macro implementation in favor of two inlined template methods, cast_to_oop() cast_from_oop(). Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.1/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new inlined template methods, cast_*_oop(), were introduced in oops/oopsHierarchy.hpp 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Thank you Ron for your review! Lois On 9/24/2013 5:13 PM, Ron Durbin wrote: I am not an official open source code reviewer, but I did review the code and it looks good. Ron *From:*Lois Foltan *Sent:* Thursday, September 19, 2013 9:11 AM *To:* hotspot-runtime-...@openjdk.java.net; build-dev@openjdk.java.net *Subject:* RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ http://cr.openjdk.java.net/%7Ehseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications and was not concerned
FR (L) JDK-7195622 (round 2): CheckUnhandledOops has limited usefulness now
Please review the following updated fix which has removed the CAST_*_OOP macro implementation in favor of two inlined template methods, cast_to_oop() cast_from_oop(). Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.1/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new inlined template methods, cast_*_oop(), were introduced in oops/oopsHierarchy.hpp 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications and was not concerned. 6. In utilities/hashtable.cpp, required CHECK_UNHANDLED_OOPS conditional for the instantiation of template class Hashtableoop, mtSymbol. With CHECK_UHANDLED_OOPS specified for a fastdebug build, an unresolved symbol occurred.
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Thank you Magnus for reviewing the build piece. Lois On 9/20/2013 2:59 AM, Magnus Ihse Bursie wrote: On 2013-09-19 16:37, Lois Foltan wrote: Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ The build changes part of the fix looks unproblematic. (I have not looked at the rest.) /Magnus
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
On 9/19/2013 9:34 PM, Christian Thalinger wrote:
On Sep 19, 2013, at 6:06 PM, Lois Foltan lois.fol...@oracle.com
mailto:lois.fol...@oracle.com wrote:
On 9/19/2013 7:25 PM, Christian Thalinger wrote:
On Sep 19, 2013, at 4:22 PM, Lois Foltanlois.fol...@oracle.com wrote:
On 9/19/2013 6:27 PM, Christian Thalinger wrote:
On Sep 19, 2013, at 3:19 PM, Lois Foltanlois.fol...@oracle.com wrote:
On 9/19/2013 6:09 PM, Christian Thalinger wrote:
+ #define CAST_TO_OOP(value) ((oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value)))
+ #define CAST_FROM_OOP(new_type, value)
((new_type)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value)))
Could these two macros also be a method?
Hi Christian,
I assume by method you are implying methods within the oop class itself?
Not necessarily. We already have a couple of inline methods is
globalDefinitions.hpp. Would that work?
That would work in the case of CAST_TO_OOP, where the type to cast value to is
known, an oop. In the case of CAST_FROM_OOP, it wouldn't work as nicely
without having to introduce many different inline methods based on the
different types that an oop must be cast to.
How about a template method?
Hi Christian,
I had to prototype this idea, here's the implementation I came up with
template class T inline oop cast_to_oop(T value) {
return (oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))value);
}
template class T inline T cast_from_oop(oop o) {
return (T)(CHECK_UNHANDLED_OOPS_ONLY((void*))o);
}
The cast_from_oop() template method vs. the CAST_FROM_OOP() macro is
a wash, in that no extra copy construction was incurred. The
cast_to_oop() template method vs. the CAST_TO_OOP() macro was not.
There was one extra call to the (void *) conversion operator and an
extra copy construction. I believe this can be attributed to the
return of the oop since the temporary oop that was constructed could
not be returned by reference since it is a temporary, thus an extra
copy construction occurred to return it by value. Given the extra
copy construction, it is better to stick with the macros.
But this is only when CHECK_UNHANDLED_OOPS is on, right? In a product
there can't be a copy construction. If that's the case I'd say we go
with the template method because the tiny overhead in a fastdebug
build is negligible.
Hi Christian,
Okay, I concur. Seems like Coleen is okay with the template approach as
well. I will switch over to implement the template and send this out
for another review once I'm done.
Thanks,
Lois
Thanks,
Lois
Lois
That would work only in the case of fastdebug builds where an oop is defined as a
class. In non-fastdebug builds, an oop is a (oopDesc *). The macros provided a
way to preserve the existing cast to from an oop to a numerical type in all
builds, even non-fastdebug ones.
Thanks for the initial review,
Lois
On Sep 19, 2013, at 8:13 AM, Lois Foltanlois.fol...@oracle.com wrote:
Please review the following fix:
Webrev:
http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/
Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile
oop requires ...
CheckUnhandledOops has limited usefulness now bug links at:
https://bugs.openjdk.java.net/browse/JDK-7180556
https://bugs.openjdk.java.net/browse/JDK-7195622
Summary of fix:
Update the C++ oop structure definition in oopsHierarchy.hpp to solve several
problems with the current definition when compiled with various C++ compilers
across supported platforms. These changes initially address the problem
reported in JDK-7180556 and continue with additional improvements to allow
CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as
suggested in JDK-7195622. Several notes concerning this fix:
1. A review should start at understanding the changes made to
oopsHierarchy.hpp
a. Addition of a non-volatile copy constructor to address
compile time errors
reported in JDK-7180556 and also currently by g++
compilers on Linux.
b. Addition of member wise assignment operators to handle
many instances
of [non]volatile to [non]volatile oops within the JVM.
Note: Solaris compilers would not allow for the member
wise assignment operators
of every flavor of non-volatile to volatile
and vice versa. However, unlike g++ compilers,
Solaris compilers had no issue passing a volatile
this pointer to a non-volatile
assignment operator. So the g++ compilers
needed these different flavors
of the assignment operator and Solaris did
not.
d. For similar reasons as 1b, addition of a volatile explicit
conversion from oop - void *.
g++ specifically complained when trying to pass
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
On 9/20/2013 9:16 AM, Stefan Karlsson wrote:
On 09/20/2013 02:46 PM, Stefan Karlsson wrote:
On 09/19/2013 05:13 PM, Lois Foltan wrote:
Please review the following fix:
Webrev:
http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/
The changes looks good.
I forgot this rather benign bug:
http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/src/share/vm/oops/klass.inline.hpp.patch
inline bool check_klass_alignment(Klass* obj) {
- return (intptr_t)obj % KlassAlignmentInBytes == 0;
+ return CAST_FROM_OOP(intptr_t, obj) % KlassAlignmentInBytes == 0;
}
Hi Stefan,
Thank you, good catch. I will fix this and send it out for a second
round of review with my changes to do away of the CAST* macros in favor
of inline template methods based on feedback from Christian Thalinger.
Lois
obj is not an oop.
thanks,
StefanK
There are some changes that I don't fully agree with, but I will not
block the push because of them. I'm just going to mention them here
for the record:
1) I would personally prefer not to have the
CAST_TO_OOP/CAST_FROM_OOP macros and instead explicitly add (void *)
casts.
2) I don't understand the need make the code harder to read by adding
a SOLAR_ONLY((void*)) instead of just a plain (void*).
I'm looking forward to be able to use this feature on Linux!
thanks,
StefanK
Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp:
Error:Initializing const volatile oop requires ...
CheckUnhandledOops has limited usefulness now bug links at:
https://bugs.openjdk.java.net/browse/JDK-7180556
https://bugs.openjdk.java.net/browse/JDK-7195622
Summary of fix:
Update the C++ oop structure definition in oopsHierarchy.hpp to
solve several problems with the current definition when compiled
with various C++ compilers across supported platforms. These
changes initially address the problem reported in JDK-7180556 and
continue with additional improvements to allow CHECK_UNHANDLED_OOPS
to be defined in all fastdebug builds on all platforms as suggested
in JDK-7195622. Several notes concerning this fix:
1. A review should start at understanding the changes
made to oopsHierarchy.hpp
a. Addition of a non-volatile copy constructor
to address compile time errors
reported in JDK-7180556 and also currently
by g++ compilers on Linux.
b. Addition of member wise assignment operators
to handle many instances
of [non]volatile to [non]volatile oops
within the JVM.
Note: Solaris compilers would not allow for
the member wise assignment operators
of every flavor of non-volatile
to volatile and vice versa. However, unlike g++ compilers,
Solaris compilers had no issue
passing a volatile this pointer to a non-volatile
assignment operator. So the g++
compilers needed these different flavors
of the assignment operator and
Solaris did not.
d. For similar reasons as 1b, addition of a
volatile explicit conversion from oop - void *.
g++ specifically complained when trying to
pass a volatile this pointer.
e. Removal of the ambiguous behavior of having
overloaded copy constructor and
explicit user conversion member functions
defined of both integral and void *.
All C++ compilers, except Solaris, issue a
compile time error concerning this ambiguity.
2. Change #1e had the consequence of C++ compilers now
generating compile time
errors where the practice has been to cast an oop to
and from an integral type such
as intptr_t. To allow for this practice to
continue, when oop is a structure and not
a OopDesc *, as is the case when
CHECK_UNHANDLED_OOPS is defined, two new
macros were introduced within globalDefinitions.hpp,
CAST_TO_OOP and CAST_FROM_OOP.
3. Due to the change in #1a #1b, the oop structure in
no longer considered a POD (plain old data)
by the g++ compilers on Linux and MacOS. This caused
several changes to be needed
throughout the JVM to add an (void *) cast of an oop
when invoking print_cr().
4. Many instances of an assignment to a volatile oop
required a const_castoop to
cast away the volatileness of the lvalue. There is
already precedence for this
type of change within utilities/taskqueue.hpp. The
following comment was in place:
// g++ complains if the volatile result of the
assignment is
// unused, so we cast the volatile away. We cannot cast
directly
// to void, because gcc treats that as not using the result
of the
// assignment
RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications and was not concerned. 6. In utilities/hashtable.cpp, required CHECK_UNHANDLED_OOPS conditional for the instantiation of template class Hashtableoop, mtSymbol. With CHECK_UHANDLED_OOPS specified for a fastdebug build, an unresolved symbol occurred. philli:% nm --demangle build//linux_amd64_compiler2/fastdebug/libjvm.so | grep Hashtable | grep seed
RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications and was not concerned. 6. In utilities/hashtable.cpp, required CHECK_UNHANDLED_OOPS conditional for the instantiation of template class Hashtableoop, mtSymbol. With CHECK_UHANDLED_OOPS specified for a fastdebug build, an unresolved symbol occurred. philli:% nm --demangle build//linux_amd64_compiler2/fastdebug/libjvm.so | grep Hashtable | grep seed
RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723 in pop_local(). This was a result of #1b, Solaris' lack of allowing for all flavors of volatile/non-volatile member wise assignment operators. Per Liden of the GC group did pre-review this specific change with regards to performance implications and was not concerned. 6. In utilities/hashtable.cpp, required CHECK_UNHANDLED_OOPS conditional for the instantiation of template class Hashtableoop, mtSymbol. With CHECK_UHANDLED_OOPS specified for a fastdebug build, an unresolved symbol occurred. philli:% nm --demangle build//linux_amd64_compiler2/fastdebug/libjvm.so | grep Hashtable | grep seed
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
On 9/19/2013 6:09 PM, Christian Thalinger wrote: + #define CAST_TO_OOP(value) ((oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value))) + #define CAST_FROM_OOP(new_type, value) ((new_type)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value))) Could these two macros also be a method? Hi Christian, I assume by method you are implying methods within the oop class itself? That would work only in the case of fastdebug builds where an oop is defined as a class. In non-fastdebug builds, an oop is a (oopDesc *). The macros provided a way to preserve the existing cast to from an oop to a numerical type in all builds, even non-fastdebug ones. Thanks for the initial review, Lois On Sep 19, 2013, at 8:13 AM, Lois Foltan lois.fol...@oracle.com wrote: Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result of the // assignment. However, casting to E means that we trigger an // unused-value warning. So, we cast the E to void. 5. The addition of the volatile keyword to the GenericTaskQueue's pop_local() pop_global() member functions was to accommodate the Solaris C++ compiler's complaint about the assignment of the volatile elements of the private member data _elems when GenericTaskQueue is instantiated with a non-volatile oop. Line #723
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
On 9/19/2013 6:27 PM, Christian Thalinger wrote: On Sep 19, 2013, at 3:19 PM, Lois Foltan lois.fol...@oracle.com wrote: On 9/19/2013 6:09 PM, Christian Thalinger wrote: + #define CAST_TO_OOP(value) ((oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value))) + #define CAST_FROM_OOP(new_type, value) ((new_type)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value))) Could these two macros also be a method? Hi Christian, I assume by method you are implying methods within the oop class itself? Not necessarily. We already have a couple of inline methods is globalDefinitions.hpp. Would that work? That would work in the case of CAST_TO_OOP, where the type to cast value to is known, an oop. In the case of CAST_FROM_OOP, it wouldn't work as nicely without having to introduce many different inline methods based on the different types that an oop must be cast to. Lois That would work only in the case of fastdebug builds where an oop is defined as a class. In non-fastdebug builds, an oop is a (oopDesc *). The macros provided a way to preserve the existing cast to from an oop to a numerical type in all builds, even non-fastdebug ones. Thanks for the initial review, Lois On Sep 19, 2013, at 8:13 AM, Lois Foltan lois.fol...@oracle.com wrote: Please review the following fix: Webrev: http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop requires ... CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 Summary of fix: Update the C++ oop structure definition in oopsHierarchy.hpp to solve several problems with the current definition when compiled with various C++ compilers across supported platforms. These changes initially address the problem reported in JDK-7180556 and continue with additional improvements to allow CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as suggested in JDK-7195622. Several notes concerning this fix: 1. A review should start at understanding the changes made to oopsHierarchy.hpp a. Addition of a non-volatile copy constructor to address compile time errors reported in JDK-7180556 and also currently by g++ compilers on Linux. b. Addition of member wise assignment operators to handle many instances of [non]volatile to [non]volatile oops within the JVM. Note: Solaris compilers would not allow for the member wise assignment operators of every flavor of non-volatile to volatile and vice versa. However, unlike g++ compilers, Solaris compilers had no issue passing a volatile this pointer to a non-volatile assignment operator. So the g++ compilers needed these different flavors of the assignment operator and Solaris did not. d. For similar reasons as 1b, addition of a volatile explicit conversion from oop - void *. g++ specifically complained when trying to pass a volatile this pointer. e. Removal of the ambiguous behavior of having overloaded copy constructor and explicit user conversion member functions defined of both integral and void *. All C++ compilers, except Solaris, issue a compile time error concerning this ambiguity. 2. Change #1e had the consequence of C++ compilers now generating compile time errors where the practice has been to cast an oop to and from an integral type such as intptr_t. To allow for this practice to continue, when oop is a structure and not a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS is defined, two new macros were introduced within globalDefinitions.hpp, CAST_TO_OOP and CAST_FROM_OOP. 3. Due to the change in #1a #1b, the oop structure in no longer considered a POD (plain old data) by the g++ compilers on Linux and MacOS. This caused several changes to be needed throughout the JVM to add an (void *) cast of an oop when invoking print_cr(). 4. Many instances of an assignment to a volatile oop required a const_castoop to cast away the volatileness of the lvalue. There is already precedence for this type of change within utilities/taskqueue.hpp. The following comment was in place: // g++ complains if the volatile result of the assignment is // unused, so we cast the volatile away. We cannot cast directly // to void, because gcc treats that as not using the result
Re: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
On 9/19/2013 7:25 PM, Christian Thalinger wrote:
On Sep 19, 2013, at 4:22 PM, Lois Foltan lois.fol...@oracle.com wrote:
On 9/19/2013 6:27 PM, Christian Thalinger wrote:
On Sep 19, 2013, at 3:19 PM, Lois Foltan lois.fol...@oracle.com wrote:
On 9/19/2013 6:09 PM, Christian Thalinger wrote:
+ #define CAST_TO_OOP(value) ((oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value)))
+ #define CAST_FROM_OOP(new_type, value)
((new_type)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value)))
Could these two macros also be a method?
Hi Christian,
I assume by method you are implying methods within the oop class itself?
Not necessarily. We already have a couple of inline methods is
globalDefinitions.hpp. Would that work?
That would work in the case of CAST_TO_OOP, where the type to cast value to is
known, an oop. In the case of CAST_FROM_OOP, it wouldn't work as nicely
without having to introduce many different inline methods based on the
different types that an oop must be cast to.
How about a template method?
Hi Christian,
I had to prototype this idea, here's the implementation I came up with
template class T inline oop cast_to_oop(T value) {
return (oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))value);
}
template class T inline T cast_from_oop(oop o) {
return (T)(CHECK_UNHANDLED_OOPS_ONLY((void*))o);
}
The cast_from_oop() template method vs. the CAST_FROM_OOP() macro is a
wash, in that no extra copy construction was incurred. The
cast_to_oop() template method vs. the CAST_TO_OOP() macro was not. There
was one extra call to the (void *) conversion operator and an extra copy
construction. I believe this can be attributed to the return of the oop
since the temporary oop that was constructed could not be returned by
reference since it is a temporary, thus an extra copy construction
occurred to return it by value. Given the extra copy construction, it
is better to stick with the macros.
Thanks,
Lois
Lois
That would work only in the case of fastdebug builds where an oop is defined as a
class. In non-fastdebug builds, an oop is a (oopDesc *). The macros provided a
way to preserve the existing cast to from an oop to a numerical type in all
builds, even non-fastdebug ones.
Thanks for the initial review,
Lois
On Sep 19, 2013, at 8:13 AM, Lois Foltan lois.fol...@oracle.com wrote:
Please review the following fix:
Webrev:
http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/
Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile
oop requires ...
CheckUnhandledOops has limited usefulness now bug links at:
https://bugs.openjdk.java.net/browse/JDK-7180556
https://bugs.openjdk.java.net/browse/JDK-7195622
Summary of fix:
Update the C++ oop structure definition in oopsHierarchy.hpp to solve several
problems with the current definition when compiled with various C++ compilers
across supported platforms. These changes initially address the problem
reported in JDK-7180556 and continue with additional improvements to allow
CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds on all platforms as
suggested in JDK-7195622. Several notes concerning this fix:
1. A review should start at understanding the changes made to
oopsHierarchy.hpp
a. Addition of a non-volatile copy constructor to address
compile time errors
reported in JDK-7180556 and also currently by g++
compilers on Linux.
b. Addition of member wise assignment operators to handle
many instances
of [non]volatile to [non]volatile oops within the JVM.
Note: Solaris compilers would not allow for the member
wise assignment operators
of every flavor of non-volatile to volatile
and vice versa. However, unlike g++ compilers,
Solaris compilers had no issue passing a volatile
this pointer to a non-volatile
assignment operator. So the g++ compilers
needed these different flavors
of the assignment operator and Solaris did
not.
d. For similar reasons as 1b, addition of a volatile explicit
conversion from oop - void *.
g++ specifically complained when trying to pass a volatile
this pointer.
e. Removal of the ambiguous behavior of having overloaded
copy constructor and
explicit user conversion member functions defined of
both integral and void *.
All C++ compilers, except Solaris, issue a compile
time error concerning this ambiguity.
2. Change #1e had the consequence of C++ compilers now generating
compile time
errors where the practice has been to cast an oop to and from
an integral type such
as intptr_t. To allow for this practice to continue
Re: RFR JDK-8022407 (round 2) sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
On 8/29/2013 10:18 PM, David Holmes wrote: On 29/08/2013 10:01 PM, Lois Foltan wrote: On 8/29/2013 7:16 AM, David Holmes wrote: Hi Lois, Is this still needed: 142 PCH_FLAG/unsafe.o = $(PCH_FLAG/NO_PCH) given you no longer use -O0 ? Hi David, Yes, I believe so but this might be a cautionary change on my part. I read the comment to imply that Clang does not support a precompiled header compiled with an optimization level that is different than the one used to compile the actual C++ file, this is why I chose to add unsafe.o. I read the comment as only applying to a combination of -O0 with -O3. So now the comment is inaccurate as it claims all files are compiled with -O0. 133 # There are some files which don't like precompiled headers 134 # The following files are build with 'OPT_CFLAGS/NOOPT' (-O0) in the opt build. 135 # But Clang doesn't support a precompiled header which was compiled with -O3 136 # to be used in a compilation unit which uses '-O0'. We could also prepare an 137 # extra '-O0' PCH file for the opt build and use it here, but it's probably 138 # not worth the effort as long as only two files need this special handling. Either the comment or the entry at line 142 need to be changed. (the part about 'two files' is already out of date :( ) Hi David, Line #142 is necessary. If clang++ compiles unsafe.o with -O1 and tries to include the precompiled header that is compiled by default with -Os, (note that -Os is the current default, not -O3 as the comment indicates), the following compile time error is generated: error: __OPTIMIZE_SIZE__ predefined macro was enabled in PCH file but is currently disabled So, the the comment is incorrect in many ways. I have entered https://bugs.openjdk.java.net/browse/JDK-8024050, detailing the issues. Thanks, Lois David - Lois Thanks, David On 29/08/2013 3:56 AM, Lois Foltan wrote: Please review the updated webrev: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407.2/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O1. Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verifyprims/unsafe.cpp remained compiled at -Os Thank you, Lois
S RFR JDK-8024050: Incorrect optimization level and comment specified for unsafe.cpp
Please review the following fix: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8024050/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8024050 Summary of fix: The original sources used for the JDK-8022407 webrev sponsorship contained an incorrect optimization level specification for unsafe.cpp that was fixed on the MacOS machine prior to testing. Unfortunately, this incorrect specification of -01 instead of -O1 was committed. In addition, corrected the comment for the Clang optimization level skew issue between PCH Files and files of different optimization levels. Tests: MacOS: built fastdebug product images using clang++ and llvm-g++ Ran original JDK-8022407 test case. JTREG testing in progress. Thank you, Lois
Re: S RFR JDK-8024050: Incorrect optimization level and comment specified for unsafe.cpp
Thanks Ron for the review! Lois On 8/30/2013 1:28 PM, Ron Durbin wrote: Lois I am unofficial reviewer, but the change looks good. Ron -Original Message- From: Lois Foltan Sent: Friday, August 30, 2013 11:18 AM To: hotspot-runtime-...@openjdk.java.net; build-dev@openjdk.java.net Subject: S RFR JDK-8024050: Incorrect optimization level and comment specified for unsafe.cpp Please review the following fix: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8024050/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8024050 Summary of fix: The original sources used for the JDK-8022407 webrev sponsorship contained an incorrect optimization level specification for unsafe.cpp that was fixed on the MacOS machine prior to testing. Unfortunately, this incorrect specification of -01 instead of -O1 was committed. In addition, corrected the comment for the Clang optimization level skew issue between PCH Files and files of different optimization levels. Tests: MacOS: built fastdebug product images using clang++ and llvm-g++ Ran original JDK- 8022407 test case. JTREG testing in progress. Thank you, Lois
Re: RFR JDK-8022407 (round 2) sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
On 8/29/2013 7:16 AM, David Holmes wrote: Hi Lois, Is this still needed: 142 PCH_FLAG/unsafe.o = $(PCH_FLAG/NO_PCH) given you no longer use -O0 ? Hi David, Yes, I believe so but this might be a cautionary change on my part. I read the comment to imply that Clang does not support a precompiled header compiled with an optimization level that is different than the one used to compile the actual C++ file, this is why I chose to add unsafe.o. Lois Thanks, David On 29/08/2013 3:56 AM, Lois Foltan wrote: Please review the updated webrev: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407.2/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O1. Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verifyprims/unsafe.cpp remained compiled at -Os Thank you, Lois
RFR JDK-8022407 (round 2) sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
Please review the updated webrev: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407.2/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O1. Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verifyprims/unsafe.cpp remained compiled at -Os Thank you, Lois
Fwd: Re: RFR JDK-8022407 sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
Forwarding to include the openjdk distribution lists. Lois Original Message Subject: Re: RFR JDK-8022407 sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35 Date: Wed, 28 Aug 2013 14:10:08 -0400 From: Lois Foltan lois.fol...@oracle.com Organization: Oracle Corporation To: David Holmes david.hol...@oracle.com CC: Coleen Phillimore coleen.phillim...@oracle.com Hi David, Thanks for the review. In this situation for the file, -O1 is a valid work around and I have sent out a second round of review request. I also have one additional comment see interspersed below. Lois On 8/27/2013 8:50 PM, David Holmes wrote: offlist From past observation the normal way we handle this is to drop the optimization level one notch at a time until it works. That way we maintain maximum optimization and by seeing what optimizations are performed at each level we can generally narrow it down to a specific optimization that can be disabled. Clang does not support the same -fspecific_optimization command line options that gcc does. From the documentation I could find the difference between -O1 (passes) and -O2 (fails) for clang are * -O2 is based on -01 o /adds/: -inline -memdep -globaldce -constmerge o /removes/: -always-inline However, you can not pass these options directly to clang. From my take on this one must compile with -emit-llvm to generate llvm IR/bitcode and then pipe the resulting file into the opt phase. So not feasible for our build makefiles but should help me further narrow down which optimization is causing the issue. Thanks again for the tip on how this is usually handled. Lois David On 28/08/2013 6:05 AM, Coleen Phillimore wrote: On 08/27/2013 03:39 PM, Lois Foltan wrote: On 8/27/2013 1:29 PM, Christian Thalinger wrote: On Aug 27, 2013, at 9:18 AM, Lois Foltan lois.fol...@oracle.com wrote: Please review the following fix: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O0. Why are we lowering to -O0 when you state in the bug report that -O1 also works? What is the code that breaks? -- Chris Hi Chris, The convention within make/bsd/makefiles/gcc.make indicated that historically files that exhibited C++ compiler optimization issues were knocked down to /NOOPT or -O0. I did also check with Coleen to make sure that prims/unsafe.cpp was not a performance critical file. So my advice was that it wasn't performance critical - because it's mostly calls from Java. Except for maybe the anonymous class functions. If you disagree, let us know. Coleen -O1 does add some optimizations on top of -O0 but not inlining. I will recheck testing with -O1 to confirm and change unsafe.cpp's optimization level to -O1 if testing yields good results. I suspect the optimization problem is in unsafe.cpp's definition of Unsafe_GetNativeByte. I had left off tracking it at that level and certainly will not close the JDK bug until I can narrow in further and hopefully report to the clang compiler team. Thanks, Lois Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verify prims/unsafe.cpp remained compiled at -Os Thank you, Lois
Re: RFR JDK-8022407 (round 2) sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
Thanks Coleen for the review. I'm cc'ing openjdk distribution lists on this reply. Lois On 8/28/2013 2:11 PM, Coleen Phillimore wrote: Looks good. Coleen On 8/28/2013 1:56 PM, Lois Foltan wrote: Please review the updated webrev: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407.2/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O1. Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verifyprims/unsafe.cpp remained compiled at -Os Thank you, Lois
Re: RFR JDK-8022407 sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
On 8/27/2013 1:29 PM, Christian Thalinger wrote: On Aug 27, 2013, at 9:18 AM, Lois Foltan lois.fol...@oracle.com wrote: Please review the following fix: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O0. Why are we lowering to -O0 when you state in the bug report that -O1 also works? What is the code that breaks? -- Chris Hi Chris, The convention within make/bsd/makefiles/gcc.make indicated that historically files that exhibited C++ compiler optimization issues were knocked down to /NOOPT or -O0. I did also check with Coleen to make sure that prims/unsafe.cpp was not a performance critical file. -O1 does add some optimizations on top of -O0 but not inlining. I will recheck testing with -O1 to confirm and change unsafe.cpp's optimization level to -O1 if testing yields good results. I suspect the optimization problem is in unsafe.cpp's definition of Unsafe_GetNativeByte. I had left off tracking it at that level and certainly will not close the JDK bug until I can narrow in further and hopefully report to the clang compiler team. Thanks, Lois Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verify prims/unsafe.cpp remained compiled at -Os Thank you, Lois
RFR JDK-8022407 sun/misc/CopyMemory.java fails with SIGSEGV in Unsafe_SetByte+0x35
Please review the following fix: open webrev at http://cr.openjdk.java.net/~hseigel/bug_jdk8022407/ Bug: bug link at https://bugs.openjdk.java.net/browse/JDK-8022407 Summary of fix: The JDK 8 build on MacOS, when built with the Xcode 4.6.2 clang++ compiler, exhibited a compiler optimization issue when prims/unsafe.cpp was compiled at the default -Os level. As a work around fix, knock the optimization level down down to -O0. Tests: MacOS: built fastdebug product images using clang++ (ran JTREG vm.quick.testlist) built using llvm-g++ to verify prims/unsafe.cpp remained compiled at -Os Thank you, Lois
