Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Mon, 22 Jan 2024 08:59:30 GMT, Kim Barrett wrote: > I don't think `#pragma GCC poison` works for us. It would complain about a > system or library header that uses NULL and is included after the pragma. I see, that's a shame in that case > However, there are still a lot of NULL's left. All of the per-cpu .ad files, > and the jvmtiXXX.xsl files contain NULL's that will appear in the associated > generated code. Also, NULL usage in gtests doesn't seem to have been > addressed yet. > > But it does look like there's been a bit of backsliding: > https://bugs.openjdk.org/browse/JDK-8324286 That's a little worrying, maybe the Style Guide's NULL section could be reworded since now most usages of NULL are nullptr? The .ad files and .xsl files are a bit of a problem though > MSVC's deprecation pragma might work for this, at least for shared and > Windows-specific code. I couldn't find a way to use it for FORBID_C_FUNCTION, > but the problems I encountered for that don't seem applicable in this case. The deprecation pragma only works for macros and identifiers, it doesn't accept method signatures and would warn for every time a identifier is used, even in the method's declaration itself! Probably can't be used in FORBID_C_FUNCTION as mentioned above, but sounds good for a macro like NULL I've also been trying to implement FORBID_C_FUNCTION and ALLOW_C_FUNCTION portably, speaking of it, but it hasn't been going great so far :/ https://github.com/openjdk/jdk/pull/17387 - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1903579473
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions I don't think `#pragma GCC poison` works for us. It would complain about a system or library header that uses NULL and is included after the pragma. MSVC's deprecation pragma might work for this, at least for shared and Windows-specific code. I couldn't find a way to use it for FORBID_C_FUNCTION, but the problems I encountered for that don't seem applicable in this case. However, there are still a lot of NULL's left. All of the per-cpu .ad files, and the jvmtiXXX.xsl files contain NULL's that will appear in the associated generated code. Also, NULL usage in gtests doesn't seem to have been addressed yet. But it does look like there's been a bit of backsliding: https://bugs.openjdk.org/browse/JDK-8324286 - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1903531104
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote:
>> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes
>> I'd appreciate if this was considered trivial.
>
> Johan Sjölen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Align
> - Suggestions
For Visual C++, that would be #pragma deprecated("NULL")
To quote Microsoft:
"You can deprecate macro names. Place the macro name in quotes or else macro
expansion will occur."
I have no idea how to achieve this with the xlc compiler
-
PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1902611263
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions I'm an entire year late, but if poisoning NULL is desired, what about #pragma GCC poison? - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1902610611
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Looks good. Thanks, Serguei - PR Review: https://git.openjdk.org/jdk/pull/14198#pullrequestreview-1456781527
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions I think if we just rely on reviews, NULLs will slip through again and we would need to have regular cleanup PRs. Doug's idea seems simple enough to implement in Skara/jcheck. An alternative to whitelisting would be a warning in the offending PR or a requirement for "special approvement" of such changes (for example, via a Skara command). - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1573169963
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote:
>> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes
>> I'd appreciate if this was considered trivial.
>
> Johan Sjölen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Align
> - Suggestions
It may be simpler to use simple grepping + an allow list. For example using
[`ag`](https://github.com/ggreer/the_silver_searcher) and `grep` seems to catch
the few remaining offenders:
> ag NULL src/hotspot/ --cpp | grep -v _NULL | grep -v NULL_ | grep -v -E
> '[A-Z]NULL' | grep -v -E '//.*NULL' | grep -v '"NULL"'
src/hotspot/cpu/ppc/macroAssembler_ppc.hpp:735: void
load_klass_check_null(Register dst, Register src, Label* is_null = NULL);
src/hotspot/cpu/ppc/stubGenerator_ppc.cpp:4700:if (UnsafeCopyMemory::_table
== NULL) {
src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp:191:if (nop == NULL) {
src/hotspot/cpu/riscv/codeBuffer_riscv.cpp:74: if
(cb->stubs()->maybe_expand_to_ensure_remaining(total_requested_size) &&
cb->blob() == NULL) {
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp:4019:if
(UnsafeCopyMemory::_table == NULL) {
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp:4077:if (bs_nm != NULL) {
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp:125: NativeCall* call =
NULL;
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp:158:if (nop == NULL)
{
src/hotspot/share/jfr/dcmd/jfrDcmds.hpp:162:JavaPermission p =
{"java.lang.management.ManagementPermission", "monitor", NULL};
src/hotspot/share/jfr/dcmd/jfrDcmds.hpp:187:JavaPermission p =
{"java.lang.management.ManagementPermission", "monitor", NULL};
src/hotspot/share/include/jvm.h:423: * Find a class from a boot class loader.
Returns NULL if class not found.
src/hotspot/share/prims/jvmtiAgent.cpp:375: const jint err =
(*on_load_entry)(&main_vm, const_cast(agent->options()), NULL);
src/hotspot/share/prims/whitebox.cpp:1885: if (cp->cache() == NULL) {
src/hotspot/share/prims/whitebox.cpp:1894: if (cp->cache() == NULL) {
src/hotspot/share/classfile/stringTable.hpp:150: static oop
init_shared_table(const DumpedInternedStrings* dumped_interned_strings)
NOT_CDS_JAVA_HEAP_RETURN_(NULL);
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:95: #undef NULL
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:96: #define NULL 0L
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:98: #ifndef NULL
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:99:#define NULL 0
src/hotspot/share/cds/filemap.cpp:363: assert(ent != NULL, "sanity");
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:65:#undef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:69:#define NULL 0LL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:71:#ifndef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:72:#define NULL 0
src/hotspot/share/utilities/globalDefinitions.cpp:162:
static_assert(sizeof(NULL) == sizeof(char*), "NULL must be same size as
pointer");
src/hotspot/share/adlc/output_c.cpp:279: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:305: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:368: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:393: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:1009: for (_pipeline->_reslist.reset();
(resource = _pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/gc/x/xBarrierSet.inline.hpp:187:return
Raw::oop_arraycopy_in_heap(nullptr, 0, src, NULL, 0, dst, length);
src/hotspot/share/gc/x/xPageTable.inline.hpp:43:if (entry != NULL && entry
!= _prev) {
src/hotspot/share/gc/x/xBarrier.cpp:242: return NULL;
src/hotspot/share/oops/cpCache.cpp:888: LogStream* log_stream = NULL;
src/hotspot/share/oops/cpCache.cpp:906:
assert(resolved_references->obj_at(appendix_index) == NULL, "init just once");
src/hotspot/share/oops/cpCache.cpp:914: if (log_stream != NULL) {
src/hotspot/share/opto/runtime.cpp:491: fields[TypeFunc::Parms+0] = NULL; //
void
src/hotspot/share/jvmci/jvmciEnv.cpp:366:if (ex != NULL) {
-
PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571756690
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Thu, 1 Jun 2023 05:23:25 GMT, Tobias Hartmann wrote: > What's the plan now to prevent re-introducing `NULL`? Hi Tobias. The only plan in place is social, the reviewers have to look out for it. I am however researching how to do this through machine. I'm currently researching ways of preventing any re-introductions by machine. These include poisoning the NULL macro by re-defining it and finding a tool which is capable of parsing C++ code which is yet to go through the pre-processor. - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571722147
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 00:57:40 GMT, David Holmes wrote: > Can we now poison NULL so it can't get reintroduced? Or would that > potentially break standard headers? I'm sure it would. Maybe some changes to Skara? - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571706648
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions What's the plan now to prevent re-introducing `NULL`? - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571360076
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Right, seems like the Windows CI fails at fetching JTReg. I'm merging this, thank you all for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1569815714
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Thanks for addressing the comment. Looks Good :-) - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/14198#pullrequestreview-1452183165
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14198#pullrequestreview-1451670515
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes > I'd appreciate if this was considered trivial. Johan Sjölen has updated the pull request incrementally with two additional commits since the last revision: - Align - Suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/14198/files - new: https://git.openjdk.org/jdk/pull/14198/files/c7e28571..c9d9c30c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14198&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14198&range=00-01 Stats: 9 lines in 8 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/14198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14198/head:pull/14198 PR: https://git.openjdk.org/jdk/pull/14198
