Re: RFR: 8346990: Remove INTX_FORMAT and UINTX_FORMAT macros
On Fri, 3 Jan 2025 14:32:39 GMT, Coleen Phillimore wrote:
> There are a lot of format modifiers that are noisy and unnecessary in the
> code. This change removes the INTX variants. It's not that disruptive even
> for backporting because %z modifier has been available for a long time so
> should backport fine. This was mostly done with a sed script plus some hand
> fixups.
>
> Testing mach5 and other platform cross compilations in progress. Opening
> this for GHA testing.
Speaking of %z, there is a non Standard %Ix in os_windows.cpp
tty->print_cr("reserve_memory of %Ix bytes took " JLONG_FORMAT " ms ("
JLONG_FORMAT " ticks)", bytes,
reserveTimer.milliseconds(), reserveTimer.ticks());
Could changing that to %zu be trivial enough to fit into this change?
-
PR Comment: https://git.openjdk.org/jdk/pull/22916#issuecomment-2569435948
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v7]
On Mon, 11 Nov 2024 09:51:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge branch 'openjdk:master' into unused > - Neater warning silencer in proc_md.h > - Revert _WIN32 workaround in log_messages.c > - Copyright in VersionInfo.cpp > - Remove neutralLangId in VersionInfo.cpp > - Remove global memHandle, would've liked to keep it as a comment :( > - Merge branch 'master' into unused > - 8342682 Oh, haha I was referring to the other Pull Requests that clean up the unused code in other parts of the JDK - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2543470498
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v7]
On Mon, 11 Nov 2024 09:51:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge branch 'openjdk:master' into unused > - Neater warning silencer in proc_md.h > - Revert _WIN32 workaround in log_messages.c > - Copyright in VersionInfo.cpp > - Remove neutralLangId in VersionInfo.cpp > - Remove global memHandle, would've liked to keep it as a comment :( > - Merge branch 'master' into unused > - 8342682 Keep it open, I'm integrating this all at once when all the corresponding changesets are approved - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2533689602
Re: RFR: 8344262: Win32AttachOperationRequest objects are created by using global new
On Fri, 15 Nov 2024 21:05:21 GMT, Alex Menkov wrote: > The issue was introduced by > [JDK-8339289](https://bugs.openjdk.org/browse/JDK-8339289). > The fix makes `Win32AttachOperationRequest` successor of > `CHeapObj`, so `Win32AttachOperationRequest` inherits its > `new` operator > > testing: tier1..tier4 Ah, you beat me to it. Thanks for the fix! - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/22166#pullrequestreview-2440305957
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 31 Oct 2024 19:07:42 GMT, Chris Plummer wrote: >>> I do wonder if mutex support can be implemented for Windows with >>> Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >>> would be nice to have. Shame threads.h is not available with some Visual >>> Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >>> nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > >> > I do wonder if mutex support can be implemented for Windows with >> > Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >> > would be nice to have. Shame threads.h is not available with some Visual >> > Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >> > nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > > Feel free to if you'd like to. Personally I don't consider it to be that > important. Bumping, @plummercj I found that casting to void in MUTEX_LOCK and MUTEX_UNLOCK also works, and it seems neater to me. Is this ok with you? - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2467690492
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v7]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'openjdk:master' into unused - Neater warning silencer in proc_md.h - Revert _WIN32 workaround in log_messages.c - Copyright in VersionInfo.cpp - Remove neutralLangId in VersionInfo.cpp - Remove global memHandle, would've liked to keep it as a comment :( - Merge branch 'master' into unused - 8342682 - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/5e9039fb..53036a65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=05-06 Stats: 200965 lines in 1998 files changed: 125072 ins; 52904 del; 22989 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v6]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Copyright in VersionInfo.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/68767cbe..5e9039fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 31 Oct 2024 19:07:42 GMT, Chris Plummer wrote: >>> I do wonder if mutex support can be implemented for Windows with >>> Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >>> would be nice to have. Shame threads.h is not available with some Visual >>> Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >>> nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > >> > I do wonder if mutex support can be implemented for Windows with >> > Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >> > would be nice to have. Shame threads.h is not available with some Visual >> > Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >> > nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > > Feel free to if you'd like to. Personally I don't consider it to be that > important. @plummercj @alexeysemenyukoracle @sashamatveev reawaiting review - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2466039622
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
On Fri, 8 Nov 2024 13:10:48 GMT, Alexey Semenyuk wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove global memHandle, would've liked to keep it as a comment :( > > src/jdk.jpackage/windows/native/libjpackage/VersionInfo.cpp line 183: > >> 181: add32bitPadding(buf); // Padding2 >> 182: >> 183: // const DWORD neutralLangId = (0x04b0 | MAKELANGID(LANG_NEUTRAL, >> SUBLANG_NEUTRAL) << 16); > > As ` neutralLangId` is unused, remove it instead of commenting it out. Will do - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1834413246
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v5]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Remove neutralLangId in VersionInfo.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/4d15fe78..68767cbe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342860: Fix more NULL usage backsliding [v3]
On Thu, 7 Nov 2024 08:32:16 GMT, theoweidmannoracle wrote: >> - Changed several "NULL" in comments to "null" >> - Changed several `NULL` in code to `nullptr` > > theoweidmannoracle has updated the pull request incrementally with one > additional commit since the last revision: > > Fix backslides in test Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/21826#pullrequestreview-2423332121
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
On Thu, 31 Oct 2024 07:18:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove global memHandle, would've liked to keep it as a comment :( Bumping, awaiting jpackage review - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2464249727
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v31]
On Fri, 8 Nov 2024 02:32:42 GMT, Alex Menkov wrote: > Can someone confirm that use of `__stdcall` has no affect on name > decorations, as there is no mention here about anything being ignored: > > https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170 > > I would have expected that if argument passing needs to use the stack then > the decorated name would still need to encode that somehow. https://godbolt.org/z/xve7cbG1e - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2463794511
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Thu, 7 Nov 2024 12:08:50 GMT, Aleksey Shipilev wrote:
>> src/hotspot/os/windows/os_windows.cpp line 510:
>>
>>> 508: // Thread start routine for all newly created threads.
>>> 509: // Called with the associated Thread* as the argument.
>>> 510: static unsigned thread_native_entry(void* t) {
>>
>> Whoa! Hold on there. The `_stdcall` is required here and nothing to do with
>> 32-bit. We use `begindthreadex` to start threads and the entry function is
>> required to be `_stdcall`.
>> https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170
>
> Not sure why this comment was marked as "Resolved". I have the same question
> here.
@shipilev See addressing comments below:
> https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170
> On ARM and x64 processors, __stdcall is accepted and ignored by the compiler;
> on ARM and x64 architectures, by convention, arguments are passed in
> registers when possible, and subsequent arguments are passed on the stack.
> To my knowledge the only thing __cdecl and __stdcall do is affect the
> argument passing on the stack since 32 bit uses the stack to pass arguments.
> Since 64 bit passes arguments inside registers and then only later uses the
> stack if there are too many parameters to fit in the parameter registers
> (Basically permanent __fastcall), these specifiers are probably ignored in
> all 64 bit platforms
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1832581212
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Wed, 6 Nov 2024 01:44:48 GMT, Alex Menkov wrote: > I think you may be throwing the baby out with the bath water when it comes to > `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see > anything that states `__stdcall` is ONLY for 32-bit! To my knowledge the only thing __cdecl and __stdcall do is affect the argument passing on the stack since 32 bit uses the stack to pass arguments. Since 64 bit passes arguments inside registers and then only later uses the stack if there are too many parameters to fit in the parameter registers (Basically permanent __fastcall), these specifiers are probably ignored in all 64 bit platforms - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458712195
Re: RFR: 8342860: Fix more NULL usage backsliding [v2]
On Tue, 5 Nov 2024 15:05:51 GMT, theoweidmannoracle wrote: >> - Changed several "NULL" in comments to "null" >> - Changed several `NULL` in code to `nullptr` > > theoweidmannoracle has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'openjdk:master' into JDK-8342860 > - Fix copyright year > - 8342860: Fix more NULL usage backsliding Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/21826#pullrequestreview-2416030956
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
On Thu, 31 Oct 2024 07:18:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove global memHandle, would've liked to keep it as a comment :( @alexeysemenyukoracle Sorry for the page, needed a reviewer for jpackage. Is jpackage good to go? - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2453881416
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
On Thu, 31 Oct 2024 07:18:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove global memHandle, would've liked to keep it as a comment :( I guess Weijun's comment means security is good to go, even though it's not an approval. jpackage, any comments? - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2451316177
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Remove global memHandle, would've liked to keep it as a comment :( - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/cd57fc6c..4d15fe78 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Sat, 26 Oct 2024 04:35:09 GMT, Julian Waters wrote: > I do wonder if mutex support can be implemented for Windows with > Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it > would be nice to have. Shame threads.h is not available with some Visual > Studio versions we support, or at all with gcc. mtx_lock/unlock would be a > nice solution to this problem I'll also start looking into this in the meantime, unless you explicitly want me not to do so - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2449178499
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v3]
On Sun, 27 Oct 2024 06:25:02 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into unused > - 8342682 Also, core-libs, any comment on jpackage? - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2449178848
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v3]
On Wed, 30 Oct 2024 06:27:22 GMT, Chris Plummer wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'master' into unused >> - 8342682 > > src/jdk.jdi/windows/native/libdt_shmem/shmem_md.c line 40: > >> 38: */ >> 39: >> 40: // static HANDLE memHandle = NULL; > > I still think this should be removed instead of commented out. I was more fond of keeping it as a comment, but oh well. Give me a moment... - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1823903506
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v15]
On Wed, 30 Oct 2024 11:19:03 GMT, Magnus Ihse Bursie wrote: >> src/hotspot/share/adlc/main.cpp line 494: >> >>> 492: } >>> 493: >>> 494: #if !defined(_WIN32) || defined(_WIN64) >> >> Removing the conditionalization is fine for this change. But see also >> https://bugs.openjdk.org/browse/JDK-8342639 >> I've added a note there that this change removed the conditionalization. > > I'm glad you're giving some TLC to adlc. It is in desperate need of it. TLC? - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1822524730
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v3]
On Sun, 27 Oct 2024 06:25:02 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into unused > - 8342682 Bumping - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2445894205
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v11]
On Tue, 29 Oct 2024 16:01:49 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > IS_WIN64 is never used and can be completely removed Oh, I meant don't merge current master into the branch for this PR haha, you'll very likely get red all over your GHA from the broken assembler_x86.cpp failing to compile (From what I can tell, all x86 builds are affected) - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2444736299
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v11]
On Tue, 29 Oct 2024 16:01:49 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > IS_WIN64 is never used and can be completely removed Just a heads up, don't merge master for the time being. I think 8341527 just broke the x86 assembler - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2444710777
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port [v4]
On Tue, 29 Oct 2024 14:37:43 GMT, Aleksey Shipilev wrote: >> @shipilev There _is_ a way to implement SpinPause on Windows/x64 though, if >> support is really as simple as a single pause instruction. Should I help >> implement this separately (After this PR is integrated, to avoid conflicts)? >> Although, the way SpinPause can be implemented is honestly so simple and >> trivial that @magicus could simply replace the entire body of this SpinPause >> with it in this PR > > Submit a separate PR and implement this :) Pretty sure you'll get into some > dark territories in Windows/AArch64, see how Linux/AArch64 does this. But > honestly, this whole `extern "C"` mess should probably be cleaned up in favor > of arch-specific stubs or something like that... Oh, I was thinking about Windows/x64, but I guess I can consider Windows/ARM64 too. I had a look at Linux/ARM64 actually, and it seems like it doesn't actually properly support SpinPause? It seems like it uses the overhead of a method call to "implement" SpinPause. I had a look at some example assembly that could potentially be used to implement it for Windows/ARM64, but I don't know if it's correct. If you want, we could continue this discussion elsewhere - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820959571
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port [v4]
On Tue, 29 Oct 2024 14:44:03 GMT, Magnus Ihse Bursie wrote: >> I stand corrected: I forgot about Windows/ARM64. To correct myself: Due to a >> bug, this file, which is meant for Windows/x64, is used by Windows/ARM64 >> instead. The consequences of this are unknown > > What bug are you referring to that makes this file unused? https://mail.openjdk.org/pipermail/hotspot-dev/2024-October/095864.html This file isn't unused, I misspoke. Instead, it is meant as the implementation of frem and drem for Windows x64, but due to a bug, it's potentially being wrongly used as the implementation of frem and drem for Windows/ARM64 instead - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820964150
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port
On Mon, 28 Oct 2024 19:25:09 GMT, Aleksey Shipilev wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86
>> Port_](https://openjdk.org/jeps/479).
>>
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port.
>>> This port was [deprecated for removal in JDK
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a
>>> future release.
>
> src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp line 523:
>
>> 521:
>> 522: extern "C" int SpinPause () {
>> 523: #ifdef AMD64
>
> Weird that SpinPause is not implemented on Win64, but oh well. This whole
> SpinPause mess should be arch-specific, not OS/Arch specific, probably.
@shipilev There _is_ a way to implement SpinPause on Windows/x64 though, if
support is really as simple as a single pause instruction. Should I help
implement this separately (After this PR is integrated, to avoid conflicts)?
Although, the way SpinPause can be implemented is honestly so simple and
trivial that @magicus could simply replace the entire body of this SpinPause
with it in this PR
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820797875
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port
On Tue, 29 Oct 2024 06:59:22 GMT, Julian Waters wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > src/hotspot/os/windows/sharedRuntimeRem.cpp line 28: > >> 26: #include "runtime/sharedRuntime.hpp" >> 27: >> 28: #ifdef _WIN64 > > Just a heads up: Due to a bug, this entire file is never used at all I stand corrected: I forgot about Windows/ARM64. To correct myself: Due to a bug, this file, which is meant for Windows/x64, is used by Windows/ARM64 instead. The consequences of this are unknown - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820776554
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port
On Tue, 29 Oct 2024 09:32:21 GMT, David Holmes wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86
>> Port_](https://openjdk.org/jeps/479).
>>
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port.
>>> This port was [deprecated for removal in JDK
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a
>>> future release.
>
> src/hotspot/share/prims/jvm.cpp line 381:
>
>> 379: {
>> 380: #undef CSIZE
>> 381: #if defined(_LP64)
>
> Windows is actually LLP64 programming model not LP64. Does Windows x64 define
> _LP64 or is that something we do in our build?
It's something we do in our build. For us, _LP64 really means 64 bit
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820464097
Re: RFR: 8339783: Implementation of JEP 479: Remove the Windows 32-bit x86 Port
On Mon, 28 Oct 2024 18:09:41 GMT, Magnus Ihse Bursie wrote: > This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 > Port_](https://openjdk.org/jeps/479). > > This is the summary of JEP 479: >> Remove the source code and build support for the Windows 32-bit x86 port. >> This port was [deprecated for removal in JDK >> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >> future release. src/hotspot/os/windows/sharedRuntimeRem.cpp line 28: > 26: #include "runtime/sharedRuntime.hpp" > 27: > 28: #ifdef _WIN64 Just a heads up: Due to a bug, this entire file is never used at all - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1820188590
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v3]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into unused - 8342682 - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/2294b27f..cd57fc6c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=01-02 Stats: 30419 lines in 377 files changed: 9472 ins; 19545 del; 1402 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 24 Oct 2024 14:22:28 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8342682 I do wonder if mutex support can be implemented for Windows with Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it would be nice to have. Shame threads.h is not available with some Visual Studio versions we support, or at all with gcc. mtx_lock/unlock would be a nice solution to this problem - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2439331318
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 24 Oct 2024 14:22:28 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8342682 Turns out jpackage is part of core libs - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2439318255
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 24 Oct 2024 14:22:28 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8342682 I don't know what label jpackage falls under - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2437133404
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Thu, 24 Oct 2024 07:11:16 GMT, David Holmes wrote: > > the way I did it I'd have to force push > > That should not be the case. You can just anti-delta changes. I did it in a really inefficient way, by checking out all files except for the files that I wanted to stay. I could not figure out how to undo it. In any case, I'm sorry for the sin I just committed... - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2435429996
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8342682 - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/e149e654..2294b27f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=00-01 Stats: 68 lines in 22 files changed: 1 ins; 21 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Wed, 23 Oct 2024 16:51:23 GMT, Chris Plummer wrote: >> I wasn't sure whether the global memHandle not being used was a bug, so I >> commented out the local one. I missed the line 88 one because it wasn't >> flagged. If it really isn't needed I'll remove that one instead > > I'm not sure what you mean by "that one". It's the static one that should be > removed. The local variables always hide the static, and there seems to be no > reason for the value of memHandle to survive outside of the local scope it is > used in. Oh, I was referring to the static global one. In that case I'll remove it (Or rather, comment it so it isn't lost to time and someone can bring it back if need be) - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1814187989
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Thanks Weijun for the ok on security (Though it still makes me uneasy because it smells like a bug, but I guess it's alright). I haven't pushed yet to remove the changes to the other files in this Pull Request because the way I did it I'd have to force push and that would destroy all the review comments, I'll do that once the comments have been fully addressed - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2434176106
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Tue, 22 Oct 2024 18:03:12 GMT, Chris Plummer wrote:
>> After 8339120, gcc began catching many different instances of unused code in
>> the Windows specific codebase. Some of these seem to be bugs. I've taken the
>> effort to mark out all the relevant globals and locals that trigger the
>> unused warnings and addressed all of them by commenting out the code as
>> appropriate. I am confident that in many cases this simplistic approach of
>> commenting out code does not fix the underlying issue, and the warning
>> actually found a bug that should be fixed. In these instances, I will be
>> aiming to fix these bugs with help from reviewers, so I recommend anyone
>> reviewing who knows more about the code than I do to see whether there is
>> indeed a bug that needs fixing in a different way than what I did
>
> src/jdk.jdi/windows/native/libdt_shmem/shmem_md.c line 47:
>
>> 45: {
>> 46: void *mappedMemory;
>> 47: // HANDLE memHandle;
>
> Why comment out this one but not the one at line 88? It seems they are both
> equally problematic and are hiding the static memHandle. I'm not sure why the
> 2nd one isn't flagged. I'd actually suggest getting rid of the static
> memHandle. It does not seem to be needed.
I wasn't sure whether the global memHandle not being used was a bug, so I
commented out the local one. I missed the line 88 one because it wasn't
flagged. If it really isn't needed I'll remove that one instead
> src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c line 53:
>
>> 51: #ifndef _WIN32
>> 52: static MUTEX_T my_mutex = MUTEX_INIT;
>> 53: #endif
>
> The reason for no reference on windows is because of the following on windows:
>
>
> #define MUTEX_LOCK(x) /* FIXUP? */
> #define MUTEX_UNLOCK(x) /* FIXUP? */
>
>
> So looks like this mutex support is something we never got around to. I think
> your current workaround is fine.
I'm curious now, how to implement mutex support on Windows? I think I prefer
that to just making it completely unavailable on Windows
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1811884490
PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1811885815
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Tue, 22 Oct 2024 09:40:35 GMT, David Holmes wrote: > > Aren't the dt_shmem and jdwp changes related to HotSpot? > > Nope. That's core-svc - the non-hotspot side of serviceability. :) Oh, well I guess you learn something new everyday :) - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2429033577
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Tue, 22 Oct 2024 01:40:11 GMT, David Holmes wrote: > > and whatever team is responsible for HotSpot debugging. > > I don't see anything hotspot related here. > > I think you would be better off splitting this up into distinct issues and > PRs for different areas. I expect the client team in particular would prefer > that. Aren't the dt_shmem and jdwp changes related to HotSpot? But I guess I'll split it up in that case, I thought it might have been quicker to group it all together in one shot. This particular Pull Request I'll probably reserve for jdwp and the like, core-libs and client (And everything else) I'll move to their own PRs - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2428041172
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did. This will > require reviews from core-libs, awt, security, accessibility, jpackage, and > whatever team is responsible for HotSpot debugging. > > This fix requires C++17, so it will have to wait for the switch to C++17. I > will be working towards that in the meantime and addressing the issues that > come with changing the language standard elsewhere. This will only go in once > the JDK actually starts using C++17 src/java.desktop/share/native/common/awt/debug/debug_trace.h line 66: > 64: /* each file includes this flag indicating module trace status */ > 65: #ifdef __cplusplus > 66: [[maybe_unused]] I don't particularly like this one, but I haven't managed to find a cleaner solution. This is because a static global in a header is not great to begin with. It's practically begging for an unused-variable error to happen - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1808952439
RFR: 8342682: Errors related to unused code on Windows after 8339120
After 8339120, gcc began catching many different instances of unused code in the Windows specific codebase. Some of these seem to be bugs. I've taken the effort to mark out all the relevant globals and locals that trigger the unused warnings and addressed all of them by commenting out the code as appropriate. I am confident that in many cases this simplistic approach of commenting out code does not fix the underlying issue, and the warning actually found a bug that should be fixed. In these instances, I will be aiming to fix these bugs with help from reviewers, so I recommend anyone reviewing who knows more about the code than I do to see whether there is indeed a bug that needs fixing in a different way than what I did. This will require reviews from core-libs, awt, security, accessibility, jpackage, and whatever team is responsible for HotSpot debugging. This fix requires C++17, so it will have to wait for the switch to C++17. I will be working towards that in the meantime and addressing the issues that come with changing the language standard elsewhere. This will only go in once the JDK actually starts using C++17 - Commit messages: - Compile as C++17 - 8342682 Changes: https://git.openjdk.org/jdk/pull/21616/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342682 Stats: 72 lines in 25 files changed: 25 ins; 1 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8341722: Fix some warnings as errors when building on Linux with toolchain clang
On Wed, 9 Oct 2024 06:25:07 GMT, Matthias Baesken wrote: >> src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c line 393: >> >>> 391: >>> 392: hcreate_r(htab_sz, symtab->hash_table); >>> 393: // guarantee(rslt, "unexpected failure: hcreate_r"); >> >> The commented out guarantee line references rslt. I'm not so sure why it was >> commented out, but it goes back to the initial load of the file 17 years >> ago. It looks like the correct thing to do if rslt is null is to "goto bad;" >> but that change is probably beyond the scope of this PR. Maybe file a new CR. > > Hi Chris , I created https://bugs.openjdk.org/browse/JDK-8341820 for the > return value checking. Might be a good idea to have rslt commented out rather than removed outright if we don't want to forget about it. guarantee on a variable that doesn't exist will be rather confusing to anyone reading through the code - PR Review Comment: https://git.openjdk.org/jdk/pull/21407#discussion_r1793006705
Re: RFR: 8340524: Remove NarrowPtrStruct
On Sat, 21 Sep 2024 04:19:13 GMT, Kim Barrett wrote:
> Please review this change which removes the class NarrowPtrStruct. The only
> place it was still being used was as the type of CompressedOops::_narrow_oops.
> Instead, we move the members from NarrowPtrStruct directly into
> CompressedOops, flattening its structure.
>
> Testing: mach5 tier1-3
> Tiers 2&3 run serviceability tests that hit the changes to that component.
Took me a while to work through the confusing diff in one of the files, but
this looks ok
src/hotspot/share/oops/compressedOops.hpp line 37:
> 35: class ReservedHeapSpace;
> 36:
> 37: struct NarrowPtrStruct {
Not an objection, just a complaint on how the GitHub diff view makes this
confusing to review
-
Marked as reviewed by jwaters (Committer).
PR Review: https://git.openjdk.org/jdk/pull/21115#pullrequestreview-2320823566
PR Review Comment: https://git.openjdk.org/jdk/pull/21115#discussion_r1770523694
Re: RFR: 8338851: Hoist os::Posix::realpath() to os::realpath() and implement on Windows [v5]
On Thu, 12 Sep 2024 13:56:42 GMT, Simon Tooke wrote: >> This PR changes the status of realpath() from a Posix-specific API to a >> globally available API, i.e. adding it to the "Hotspot Porting API". Code >> would refer to os::realpath() instead of os::Posix::realpath(). >> >> This requires the addition of a stub routine in os_posix.cpp and a Windows >> implementation of realpath(), using Windows _fullpath(). >> >> This PR depends on #20597 in that it removes the need for one #ifdef in that >> PR. Because of that, this PR will be modified when and if #20597 is >> integrated (or vice-versa) >> >> Please note that guidelines for doing this appear in >> src/hotspot/share/runtime/os.hpp > > Simon Tooke has updated the pull request incrementally with three additional > commits since the last revision: > > - remove empty line > - fix indentation > - fix missing return statement Good work! - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/20683#pullrequestreview-2306707222
Re: RFR: 8338851: Hoist os::Posix::realpath() to os::realpath() and implement on Windows [v3]
On Wed, 11 Sep 2024 14:31:43 GMT, Simon Tooke wrote:
>> This PR changes the status of realpath() from a Posix-specific API to a
>> globally available API, i.e. adding it to the "Hotspot Porting API". Code
>> would refer to os::realpath() instead of os::Posix::realpath().
>>
>> This requires the addition of a stub routine in os_posix.cpp and a Windows
>> implementation of realpath(), using Windows _fullpath().
>>
>> This PR depends on #20597 in that it removes the need for one #ifdef in that
>> PR. Because of that, this PR will be modified when and if #20597 is
>> integrated.
>>
>> Please note that guidelines for doing this appear in
>> src/hotspot/share/runtime/os.hpp
>
> Simon Tooke has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - move os::realpath() to previous location
> - move os::realpath() to previous location
src/hotspot/os/posix/os_posix.cpp line 1031:
> 1029:
> 1030: // Sleep forever; naked call to OS-specific sleep; use with CAUTION
> 1031: void os::infinite_sleep() {
Ouch, looks like something broke in one of the commits, the new diff it's
showing isn't pretty (infinite_sleep and friends have been displaced in the
file, at least on my end)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1754761196
Re: RFR: 8338851: Hoist os::Posix::realpath() to os::realpath() and implement on Windows [v2]
On Wed, 4 Sep 2024 20:10:54 GMT, Simon Tooke wrote:
>> This PR changes the status of realpath() from a Posix-specific API to a
>> globally available API, i.e. adding it to the "Hotspot Porting API". Code
>> would refer to os::realpath() instead of os::Posix::realpath().
>>
>> This requires the addition of a stub routine in os_posix.cpp and a Windows
>> implementation of realpath(), using Windows _fullpath().
>>
>> This PR depends on #20597 in that it removes the need for one #ifdef in that
>> PR. Because of that, this PR will be modified when and if #20597 is
>> integrated.
>>
>> Please note that guidelines for doing this appear in
>> src/hotspot/share/runtime/os.hpp
>
> Simon Tooke has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - simplify windwos realpath() implementation
> - get rid of os::posix::realpath() and os::win32::realpath()
Looks good, just 1 comment
src/hotspot/os/posix/os_posix.cpp line 1027:
> 1025: }
> 1026:
> 1027: char* os::Posix::realpath(const char* filename, char* outbuf, size_t
> outbuflen) {
I'm looking at this from the GitHub UI so I might be missing something, but why
was this moved up?
-
Marked as reviewed by jwaters (Committer).
PR Review: https://git.openjdk.org/jdk/pull/20683#pullrequestreview-2282462140
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1745181260
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Mon, 2 Sep 2024 04:09:19 GMT, David Holmes wrote: > > This does make me wonder: What if the new method for checking if the VM was > > statically linked was inlined? Then the problem comes back yet again as the > > object files need to be recompiled once more. This is possible if Link Time > > Optimization is switched on, and I don't like the implication that LTO > > might be removed as a result just to make this work > > Wouldn't such link-time "inlining" only appear in the object code stored > within the library, not in the original object file? Ah, you're right, good catch. I got mixed up with the implementation details of different compilers there, sorry - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2323778397
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly This does make me wonder: What if the new method for checking if the VM was statically linked was inlined? Then the problem comes back yet again as the object files need to be recompiled once more. This is possible if Link Time Optimization is switched on, and I don't like the implication that LTO might be removed as a result just to make this work - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2323416932
Re: RFR: 8339120: Use more fine-granular gcc unused warnings [v6]
On Wed, 28 Aug 2024 15:17:52 GMT, Magnus Ihse Bursie wrote: >> Currently, we issue -Wno-unused for all files in gcc, which is a rather big >> sledgehammer to get rid of some warnings that proliferate in a few areas of >> the build. >> >> We should instead leave -Wunused turned on (as done by -Wall) and use a much >> more fine-grained approach to disabling specific warnings in specific files >> or libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Roll back indentation fix Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2266632564
Re: RFR: 8339120: Use more fine-granular gcc unused warnings
On Tue, 27 Aug 2024 20:04:21 GMT, Magnus Ihse Bursie wrote: > Currently, we issue -Wno-unused for all files in gcc, which is a rather big > sledgehammer to get rid of some warnings that proliferate in a few areas of > the build. > > We should instead leave -Wunused turned on (as done by -Wall) and use a much > more fine-grained approach to disabling specific warnings in specific files > or libraries. That's a lot of new warnings, but I like the idea of being more fine grained in what is disabled and enabled - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2265064013
Integrated: 8336289: Obliterate most references to _snprintf in the Windows JDK
On Fri, 12 Jul 2024 06:29:34 GMT, Julian Waters wrote: > snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) This pull request has now been integrated. Changeset: cd9e241f Author:Julian Waters URL: https://git.openjdk.org/jdk/commit/cd9e241f0ec10c7b31d36cbfb994bc20d81a0517 Stats: 70 lines in 12 files changed: 0 ins; 16 del; 54 mod 8336289: Obliterate most references to _snprintf in the Windows JDK Reviewed-by: kbarrett, dholmes, jpai, mullan, djelinski, prr - PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Sat, 24 Aug 2024 05:12:42 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into snprintf > - Change copyright years and formatting > - Revert Standard Integer type rewrite > - USe os::snprintf in HotSpot > - Obliterate most references to _snprintf in the Windows JDK Thanks everyone for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2311533460
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Sat, 24 Aug 2024 05:12:42 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into snprintf > - Change copyright years and formatting > - Revert Standard Integer type rewrite > - USe os::snprintf in HotSpot > - Obliterate most references to _snprintf in the Windows JDK Awesome! Will wait a while before integrating so objections, if any, can be raised. If no objections are raised by tomorrow morning I'll proceed with integration - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2310083308
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v4]
On Sat, 24 Aug 2024 05:06:18 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Change copyright years and formatting Alright, never mind, I've gone ahead with the copyright anyway, and in the process also fought my OCD and given the snprintf calls proper formatting too. Now awaiting re-review from all, as well as the missing review from security - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2308107303
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
> snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into snprintf - Change copyright years and formatting - Revert Standard Integer type rewrite - USe os::snprintf in HotSpot - Obliterate most references to _snprintf in the Windows JDK - Changes: - all: https://git.openjdk.org/jdk/pull/20153/files - new: https://git.openjdk.org/jdk/pull/20153/files/e14ef6b6..d8f7b849 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=03-04 Stats: 56099 lines in 1629 files changed: 32020 ins; 16189 del; 7890 mod Patch: https://git.openjdk.org/jdk/pull/20153.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20153/head:pull/20153 PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v4]
> snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Change copyright years and formatting - Changes: - all: https://git.openjdk.org/jdk/pull/20153/files - new: https://git.openjdk.org/jdk/pull/20153/files/a0477b8b..e14ef6b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=02-03 Stats: 46 lines in 10 files changed: 0 ins; 0 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/20153.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20153/head:pull/20153 PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite I was going to ask for a security review, but I decided to only do that after I change the copyright years in the files that need it. Well, when I have the time that is... - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2308076996
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Thu, 22 Aug 2024 02:46:34 GMT, David Holmes wrote: > Sorry but I don't understand the point of changing build-time constructs > using `ifdef STATIC_BUILD` into what appear to be runtime checks, but the > result of which is already determined at build time. These are not really > runtime checks. I believe the new methods are for checking whether the JDK was statically linked or not, but I half agree with your point. The change to using the new methods instead of preprocessor checks don't seem to really do anything. Maybe the new methods can be kept alongside the preprocessor checks - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2303813593
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Alright! Just need approval for java.base and security, then we should be good to go - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2268188595
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Yeah, I do unfortunately need reviewers for the sections that haven't been approved yet. Taking this reminder as an opportunity to bump this Pull Request :) - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2263315060
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 06:54:10 GMT, Kim Barrett wrote:
>> src/hotspot/share/prims/methodHandles.cpp line 439:
>>
>>> 437: default:
>>> 438: fatal("unexpected intrinsic id: %d %s", vmIntrinsics::as_int(iid),
>>> vmIntrinsics::name_at(iid));
>>> 439: return 0;
>>
>> Do we no longer need these returns after `fatal` to keep compilers happy?
>
> Now that we have, and are using, `[[noreturn]]` on all platforms, we no
> longer need that dead code.
I'll admit, I do prefer having a return to end all possible control flows in a
non void method, but oh well
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20385#discussion_r1696829614
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 04:12:33 GMT, Kim Barrett wrote: > Please review this change that removes some uses of literal 0 as a null > pointer constant in prims code. > > Testing: mach5 tier1 Looks Good! - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/20385#pullrequestreview-2207470079
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
> snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Revert Standard Integer type rewrite - Changes: - all: https://git.openjdk.org/jdk/pull/20153/files - new: https://git.openjdk.org/jdk/pull/20153/files/1bd6bc09..a0477b8b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20153.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20153/head:pull/20153 PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v2]
On Mon, 15 Jul 2024 16:30:02 GMT, Kim Barrett wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> USe os::snprintf in HotSpot > > src/jdk.jdwp.agent/windows/native/libjdwp/util_md.h line 32: > >> 30: #include /* for _MAx_PATH */ >> 31: >> 32: typedef unsigned long long UNSIGNED_JLONG; > > This change has nothing to do with _sprintf. Not sure why it's being made > here. It was a small change, so I thought I could make it out of convenience. I'll switch it out to a separate changeset - PR Review Comment: https://git.openjdk.org/jdk/pull/20153#discussion_r1679012119
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK
On Fri, 12 Jul 2024 06:29:34 GMT, Julian Waters wrote: > snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) I would add a FORBID_C_FUNCTION for _snprintf for Windows, but unfortunately that would be completely useless since there's no way to restrict methods on Visual Studio - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2226782288
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v2]
> snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) Julian Waters has updated the pull request incrementally with one additional commit since the last revision: USe os::snprintf in HotSpot - Changes: - all: https://git.openjdk.org/jdk/pull/20153/files - new: https://git.openjdk.org/jdk/pull/20153/files/20a8e2a0..1bd6bc09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=00-01 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/20153.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20153/head:pull/20153 PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK
On Fri, 12 Jul 2024 19:18:09 GMT, Kim Barrett wrote: > This should be using `os::snprintf` rather than `snprintf`. Rationale is in > the comment here: > > https://github.com/openjdk/jdk/blob/1f6e106b45e5109224e13d70f1a40c9e666ec2ab/src/hotspot/share/runtime/os.cpp#L118-L126 > > And yes, I know there are lots of bare uses of snprintf (about 125?), > including in shared code. That's why it isn't currently in the forbidden > list. There's some cleanup to do there. Ah, I'd assumed that the places where bare _snprintf was used had a reason for using it directly. I'll change all the usages in this Pull Request to use os::snprintf - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2226776993
RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK
snprintf has been available for all officially and unofficially supported compilers for Windows, Visual Studio since version 2015 and gcc since, well, forever. snprintf is conforming to C99 since the start when compiling using gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides better semantics than _snprintf, and is not compiler specific, we should use it in most places where we currently use _snprintf. This makes JDK code where this is used more robust due to the null terminating guarantee of snprintf. Since most of the changes are extremely simple, I've included all of them hoping to get this all done in one shot. The only places _snprintf is not replaced is places where I'm not sure whether the code is ours or not (As in, imported source code for external libraries). Note that the existing checks in place for the size of the characters written still work, as the return value of snprintf works mostly the same as _snprintf. The only difference is the null terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for jdk.management(?) - Commit messages: - Obliterate most references to _snprintf in the Windows JDK Changes: https://git.openjdk.org/jdk/pull/20153/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20153&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336289 Stats: 35 lines in 12 files changed: 0 ins; 16 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/20153.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20153/head:pull/20153 PR: https://git.openjdk.org/jdk/pull/20153
Re: RFR: 8335688: Fix -Wzero-as-null-pointer-constant warnings from fflush calls in jvmti tests
On Thu, 4 Jul 2024 12:15:09 GMT, Kim Barrett wrote: > Please review this change to some jvmti tests, which were calling fflush with > an argument of 0. Most of these are in C++ code, where we change them to use > nullptr as the argument. A couple are in C, where we change them to use NULL. > This removes a bunch of -Wzero-as-null-pointer-constant when building with > that enabled. > > Testing: mach5 tier1 Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/20032#pullrequestreview-2160159717
Re: RFR: 8335370: Fix -Wzero-as-null-pointer-constant warning in jvmti_common.hpp
On Sun, 30 Jun 2024 22:15:34 GMT, Kim Barrett wrote: > Please review this trivial change to the print_method function in > test/lib/jdk/test/lib/jvmti/jvmti_common.hpp. It was passing 0 as the FILE* > argument to fflush, triggering -Wzero-as-null-pointer-constant if that warning > is enabled. The change is to replace that 0 with nullptr. > > This removes about 10% of the -Wzero-as-null-pointer-constant warnings when > building with that option. > > Testing: mach5 tier1 Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19962#pullrequestreview-2150361649
Re: RFR: 8332400: isspace argument should be a valid unsigned char [v2]
On Tue, 11 Jun 2024 18:07:10 GMT, Robert Toyonaga wrote: >> ### Summary >> This change ensures we don't get undefined behavior when >> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). >> `isspace` accepts an `int` argument that "the application shall ensure is >> a character representable as an unsigned char or equal to the value of the >> macro EOF.". >> >> Previously, there was no checking of the values passed to `isspace`. I've >> replaced direct calls with a new wrapper `os::is_space` that performs a >> range check and prevents the possibility of undefined behavior from >> happening. For instances outside of Hotspot, I've added casts to `unsigned >> char`. >> >> **Testing** >> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check >> `os::is_space` is working correctly. >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Replace wrapper with casts. Had I seen this earlier I would have suggested more C++ styled casts to make which conversion exactly is happening clearer, but oh well. I agree that it's unfortunate that this has to be done - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2112608501
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v4]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Capstone Support - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=03 Stats: 395 lines in 19 files changed: 201 ins; 41 del; 153 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v3]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Capstone Support - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: - all: https://git.openjdk.org/jdk/pull/18915/files - new: https://git.openjdk.org/jdk/pull/18915/files/09fb3d65..b0b088e9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=01-02 Stats: 121745 lines in 2137 files changed: 85534 ins; 24227 del; 11984 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 13:01:15 GMT, SendaoYan wrote: >> As confusing as they are, unfortunately GitHub UI does not render extra >> trailing newlines. This is the only one I could find with grepWin. > > I find the extra trailing newlines through below shell command: > > for i in `find . -iname "Makefile*" | sed "/./build/d"` ; do tail -n 2 $i | > grep -c "^$" | grep -q "^1$" ; if [[ 0 -eq $? ]] ; then echo $i ; fi ; done > > > There are only two files has been found: > > ./test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile > ./test/jdk/java/rmi/reliability/benchmark/bench/Makefile Ah, I had not realized that there was more than 1 newline. GitHub's UI confused me here, so we're good to go - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631213656
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:26:39 GMT, SendaoYan wrote: >> test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile line 1: >> >>> 1: # >> >> This file change is dubious: >> 1. It does not have any trailing whitespace that can fail the skara checks. >> 2. If the duplicate blank lines in the end of this Makefile is indeed >> problematic (as fixed here), please fix the only other occasion in the JDK, >> which is the Makefile in the parent directory. (Checked with `\n$^\n$\Z` >> pattern in all Makefiles) >> >> Recommended actions: Either >> 1. Revert changes in this file; >> 2. Also update `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` to >> remove the trailing blank line. > > Thanks for the suggestion, the trailing blank line of > `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` has been removed. Hmm, I'm inclined to keep the newlines at the EOF for both, what do the rest of you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631140457
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:29:39 GMT, SendaoYan wrote: >> Hi all, >> This PR several extra empty spaces and extra empty lines in several >> Makefiles. It's trivial fix, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > delete extra empty trailing blank line in > test/jdk/java/rmi/reliability/benchmark/bench/Makefile Marked as reviewed by jwaters (Committer). test/jdk/java/rmi/reliability/benchmark/bench/Makefile line 50: > 48: clean: > 49: rm -f *.class .classes > 50: Hmm, shouldn't this newline at EOF be kept? Asking @ all the people who've reviewed this so far, no need to change it just yet - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2104439647 PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631140418
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie wrote:
> Hi Julian, sorry for not getting back to you.
>
> The problem from my PoV is that this is a very large and intrusive patch in
> the build of the actual product, for a claimed problem in the tangential
> hsdis library. I have not understood a pressing business need to get a
> Windows/gcc port for hsdis, which means I can't really prioritize trying to
> understand this patch.
>
> As you know, the build system does not currently really separate between "the
> OS is Windows" and "the toolchain is Microsoft". I understand that you want
> to fix that, and in theory I support it. However, you must also realize that
> making a complete fix of this requires a lot of work, for something that
> there is no clearly defined need. (After all, cl.exe works fine to create the
> build, has no apparent issues, and is as far as an "official" compiler for
> Windows as you can get.) That makes it fall squarely in the WIBNIs bucked
> ("wouldn't it be nice if...").
>
> If you can fix just the hsdis build without changing anything outside the
> hsdis Makefiles, that would be a different story. Such a change would be
> limited in scope, easy to say it will not affect the product proper, and be
> easier to review.
Oh, no worries. Sorry for sounding a little impatient.
The problem is that there are areas in the build system that will need changing
to support hsdis compilation, not just the hsdis Makefile and autoconf file
itself. I can try to work on minimizing the amount of changes to non-hsdis
files (I was hoping the current changes were small enough, but it seems they
are not), but I believe it's impossible to achieve this by only touching the
hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no
matter how minimal, to non-hsdis files.
As for why do this at all, I was mainly driven by seeing the hack in place for
the binutils backend on Windows. It only supports Cygwin, of which the gcc
installation is subpar compared to the newer gcc provided by some MSYS2
subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it
doesn't get as much attention and misses out further fixes and enhancements
from the MSYS2 team, for instance it even links to the obsolete msvcrt
library), and the hack itself has several flaws that don't seem apparent at
first (For instance, -lpthread will link to libwinpthread.dll and result in an
invisible dependency on that dll depending on the Windows platform, which will
cause loading hsdis to silently fail depending on how it's loaded for seemingly
random reasons!). I wanted to replace that (or I guess provide a better
alternative) by adding semi proper support to the hsdis build for the more
modern and better battle tested gcc that some MSYS2 subsystems provide, to
streamline comp
iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on
Windows. hsdis-capstone I also decided to support because it's small and
relatively easy to pile on top of the existing work, and MSYS2 also provides
Capstone as well. The same unfortunately could not be said for hsdis-llvm,
which was significantly more complex than Capstone is
I'm not sure where to go from here. I could support gcc for hsdis binutils by
extending the hack that exists for Cygwin, but I _really_ don't want to do
that. I could enhance the build system to semi properly support gcc for hsdis
only, as I've done, but the changes will necessarily touch non hsdis files as
well, no matter how minimal they are. I'll return this to draft for the time
being I suppose, I'd be interested in hearing which way forward you prefer
though
But while I work on making this change even smaller and easier to review, could
I ask the above questions again? (Well, except for the FIXPATH one, you can
ignore that)
> @magicus I think I might need some help here. Currently all the Cygwin stuff
> is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be
> behind isBuildOsEnv cygwin checks instead? I don't know how to add support
> for Cygwin gcc for most of hsdis, since it is quite different from the more
> modern gcc distributions that are found in places like MSYS2 and MINGW64. But
> most of the existing logic seems to accomodate more for the Microsoft
> compiler than it is concerned about the OS environment being used, and for
> this reason I can't tell which of the 2 checks to use for the existing hack
> that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but
> Microsoft does, but I don't want to check for microsoft inside
> TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get
> FIXPATH while Microsoft does?
> Also @magicus, what is the typical path passed to --with-binutils like on
> Windows? Something like
> --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work
> correctly, since the include path to dis-asm.h wou
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Thu, 9 May 2024 07:50:00 GMT, Julian Waters wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Add check for compiler in configure > - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis Magnus? Erik? You guys there? :( - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2112465392
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Thu, 9 May 2024 07:50:00 GMT, Julian Waters wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Add check for compiler in configure > - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis Hmm, it seems Magnus isn't available at the moment. @erikj79 might you be able to answer my questions regarding hsdis? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2109049333
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2106619396
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2103667760
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. Also @magicus, what is the typical path passed to --with-binutils like on Windows? Something like --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work correctly, since the include path to dis-asm.h would then become `#include "/c/Users/vertig0/Downloads/binutils-2.42/include/dis-asm.h"` Which causes a configure check to fail on the compile stage since gcc cannot recognise the MINGW-esque /c/ as a drive, and then causes configure to erroneously report binutils as using the Old API when it's in fact using the New API. --with-binutils=C:/Users/vertig0/Downloads/binutils-2.42 on the other hand works as expected. Should there be a fixup for the path there so gcc can recognise it properly? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2102146604
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=01 Stats: 334 lines in 19 files changed: 154 ins; 25 del; 155 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus I think I might need some help here. Currently all the Cygwin stuff is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be behind isBuildOsEnv cygwin checks instead? I don't know how to add support for Cygwin gcc for most of hsdis, since it is quite different from the more modern gcc distributions that are found in places like MSYS2 and MINGW64. But most of the existing logic seems to accomodate more for the Microsoft compiler than it is concerned about the OS environment being used, and for this reason I can't tell which of the 2 checks to use for the existing hack that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but Microsoft does, but I don't want to check for microsoft inside TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get FIXPATH while Microsoft does? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2097196121
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 14:25:22 GMT, Magnus Ihse Bursie wrote: > There's a huge amount of changes for just hsdis... You might have to separate > out the infrastructure changes that seem to amount to most of the changes > here. > > This is going to take me a while to get through. Sorry, it's still a WIP, and I believe something broke and scattered changes from one of my other local branches into this current changeset - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072483937
RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
WIP This changeset contains hsdis for Windows/gcc Port. It supports both the binutils and capstone backends, though the LLVM backend is left out due to compatibility issues encountered during the build. Currently, which gcc distributions are supported is still to be clarified, as several, ranging from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, the build system hack in place at the moment to compile the binutils backend on Windows is still left in place, for now. - Commit messages: - Implementation of hsdis for 8288293: Windows/gcc Port Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330988 Stats: 347 lines in 28 files changed: 161 ins; 22 del; 164 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 12:58:28 GMT, Claudio Nieder wrote: > > Could I trouble you to mention what exactly was different? > > No trouble at all. > > `CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is > where I checked out OpenJDK) > > `CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working > parent commit but just `pch_defines` with this commit. I.e. > `CCACHE_SLOPPINESS=pch_defines,time_macros` vs > `CCACHE_SLOPPINESS=pch_defines`. Thanks! Seems curious to me, off the top of my head I can't seem to discern why these would change, perhaps it's time for a little investigating - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976534197
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 11:28:02 GMT, Magnus Ihse Bursie wrote: > > I wonder if it's the SetupToolchain changes that has caused this. ccache is > > collapsed into CC and CXX to my knowledge > > Yeah, it must have been. Would you like to take a look at it? Otherwise I'll > file a bug and fix it. Breaking ccache was an unintended regression, so it > needs to be fixed. If we want to drop ccache support it needs to be done > explicitly and separately. No problem! I do hope that dropping ccache support isn't something that's planned though :( > Additionally some environment variables are missing/different. Could I trouble you to mention what exactly was different? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976398726
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I'd find it quite sad if we stopped supporting ccache just because it broke after this commit, I know for a fact that quite a few JDK developers do use it (myself included). Regarding the issue itself, I wonder if it's the SetupToolchain changes that has caused this. ccache is collapsed into CC and CXX to my knowledge, which would explain why the ccache cc or ccache c++ command is missing from the command line (substitute cc and c++ for the respective compiler drivers) - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1975838054
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 Was a blast looking through all 729 files, but have a +1 - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1905352841
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I can't really say I agree, but I guess we'll see where this goes from here first - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966471047
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 11:09:26 GMT, Magnus Ihse Bursie wrote: > > can we get rid of LDCXX? > > Yeah, that is something I plan to look into. Linking C++ object files with > gcc will fail; and it might be that linking pure C with g++ might be > problematic. If this is the case, I hope we can at least determine this > automatically which linker frontend to use, i.e. selecting g++ as linker > frontend if there is at least one .cpp file in the set of sources. > > This PR is actually a kind of prerequisite for that kind of work. I'd certainly opt for selecting which linker driver to use automatically than using one for both; According to some discussions with several gcc maintainers (https://web.libera.chat/) they really aren't meant to be intermingled Also was fine with LANG; There wasn't a need to change it to LINK_TYPE, but oh well - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966341897
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE Just a small question by the way: All those new parameters to SetupNativeCompilation, were they always there and the comments about them were just missing from the documentation about the function? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966025499
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:07:38 GMT, Daniel Jeliński wrote: > can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is > `gcc`; I searched for the differences, and the only thing I could find is > that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could > explicitly add these options, and use gcc everywhere. > > See: https://stackoverflow.com/a/172592 > https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html I don't think that's a good idea, the differences between the gcc and g++ drivers are supposed to be (opaque) implementation details. Doing so would make us dependent on internal mechanisms of gcc subject to change between versions and would make bugfixing hard to do if such a change really happened and the linker command line got messed up as a result (This likely would impact clang too, unless I am mistaken?) - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966002485
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE Thanks for the run down on the history of the build system! I'm sorry it took me a day to respond, I must have missed this in my inbox while going over the GitHub activity of the day. Given that the function was meant for the older build system, it now seems reasonable to replace it with this newer solution. In the worst case scenario, a backout is possible, as was suggested. I would have said that LANG is an ok name and that there was no need to rename it to LINK_TYPE after the context given and the knowledge that future work was planned for it, had I read this earlier though :( This is also the first time I've ever had an objection to a Pull Request. Feels weird, really - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965949353
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. !!! Not to be a party pooper, but this seems like it removes pretty some useful functionality from the build system. What if this is needed down the line? On the motivation of this change, TOOLCHAIN_LINK_CXX is pretty clear that the linker to be used is g++ instead of gcc for instance, while the new LANG parameter makes it look like SetupNativeCompilation only accepts and compiles C++ files if C++ is passed, and only C files if the new default of C is passed (For anyone looking in on this Pull Request who isn't familiar with the build system, it can compile a mix of both without issue). I'm not particularly sure this is a good idea, since a lot of flexibility seems to be lost with this change. I don't seem to see any simplification to SetupNativeCompilation either, maybe I'm missing something? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1962269640
