8290059: Do not use std::thread in panama tests

[JornVernee]

This patch removes the use of std::thread from the java.lang.foreign tests, and switches to the OS specific thread APIs, in order to change things such as the stack size on some platforms where this is required in the future (see the JBS issue).

This is done by adding a small header-only library that exposes a function to run code in freshly spawned threads (run_async).

Testing: Running the affected tests on platforms that implement the linker.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8290059: Do not use std::thread in panama tests

Reviewers

• Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 49e60f2d
• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 49e60f2d
• Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 49e60f2d

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9599/head:pull/9599
$ git checkout pull/9599

Update a local copy of the PR:
$ git checkout pull/9599
$ git pull https://git.openjdk.org/jdk pull/9599/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9599

View PR using the GUI difftool:
$ git pr show -t 9599

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9599.diff

[bridgekeeper]

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@JornVernee The following labels will be automatically applied to this pull request:

• build
• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (be6c484b)
• 03: Full - Incremental (721f70bc)
• 02: Full - Incremental (cf415f5d)
• 01: Full - Incremental (49e60f2d)
• 00: Full (8e538b47)

[mcimadamore]

Looks good!

[openjdk]

@JornVernee This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290059: Do not use std::thread in panama tests

Reviewed-by: mcimadamore, stuefe, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 47 new commits pushed to the master branch:

• 9234624: Merge
• 36c00fd: 8291006: java/foreign/TestUnsupportedPlatform fails after JDK-8290455
• 8c9d5ad: 8290455: jck test api/java_lang/foreign/VaList/Empty.html fails on some platforms
• c29242e: 8290460: Alpine: disable some panama tests that rely on std::thread
• 2f3e494: 8290074: Remove implicit arguments for RegisterMap constructor
• e804236: 8291289: gc/TestPLABAdaptToMinTLABSize fails after JDK-8289137
• adaf3b9: 8291056: Remove unused Generation::par_promote*()
• 48b77a6: 8285792: Posix signal handler modification checking issues.
• 8ec3197: 8291002: Rename Method::build_interpreter_method_data to Method::build_profiling_method_data
• 61e072d: 8290705: StringConcat::validate_mem_flow asserts with "unexpected user: StoreI"
• ... and 37 more: https://git.openjdk.org/jdk/compare/7ec0132ad3129b805664c85351fe6d55041066fa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[RealCLanger]

@JornVernee thanks for doing this so quickly. I suggest undoing d7f0de2 in this change, too. If you update this PR I can run it through our Alpine test pipeline.

[JornVernee]

@RealCLanger Note that I'm not setting the stack size of the thread in this patch. I'm just using the defaults. From the discussion on the bug, I don't think this sufficient to make the tests pass on Apline/MuslC.

I avoided getting into that since I don't have ready access to an Alpine/MuslC testing environment atm. So, I've left setting the stack size on MuslC, and re-enabling the tests for someone that does. Hopefully this patch is enough to get that going easily.

[JornVernee]

@mcimadamore Thanks for the comments. I've cleaned up the comments on the compiler switches (just removed them).

I also realized there was an unused typedef for THREAD_ID, which I've removed. Finally, I realized that after printing an error, we should probably also exit the test (I'd forgotten to add this before making the PR public). I've added a helper function called fatal that prints the error message and then calls exit. The tests still pass with that.

[RealCLanger]

@RealCLanger Note that I'm not setting the stack size of the thread in this patch. I'm just using the defaults. From the discussion on the bug, I don't think this sufficient to make the tests pass on Apline/MuslC.

I avoided getting into that since I don't have ready access to an Alpine/MuslC testing environment atm. So, I've left setting the stack size on MuslC, and re-enabling the tests for someone that does. Hopefully this patch is enough to get that going easily.

OK, thanks for clarifying that. @tstuefe, maybe you want to have a look after this fix is in?

[tstuefe]

Hi @JornVernee,

good job and thanks for doing it so quickly. Looks good, module the GetLastError comment.

About the stack size and Alpine, I am fine with doing the Aĺpine-specific fix in a follow-up, or you can do it as part of this change. Strictly speaking its not part of this bug report to adjust the stack size.

If you fix the GetLastError issue, I don't need another look.
Thanks, Thomas

[tstuefe]

Won't work as intended for Windows APIs. I would print the result of GetLastError() instead.

Alternatively I am fine fine with just omitting the error code, because I think the old tests did not handle errors either. Or did we catch std::thread exceptions somewhere?

[JornVernee]

The intent was to exit the test with a non-zero exit code, in order to avoid any accidental false positives.

I could return the error code from GetLastError and from the respective pthread apis as an exit code instead. Is that what you mean?

[JornVernee]

FWIW, perror just prints to stderr: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/perror-wperror?view=msvc-170

[JornVernee]

Ah, I see what you mean now. The C standard library function perror defines this to also print a textual description of errno. https://en.cppreference.com/w/c/io/perror so that won't work for the Windows APIs.

I think the alternative would be to use FormatMessage on Windows. I didn't really think that much into this.

Maybe it's clearer to use fputs with stderr here.

[tstuefe]

I'm curious, does this only exist because of the DWORD vs void* return value size of the native thread functions? I wondered why you don't just pass the test procedure to the thread start API directly.

About stdcall, does Oracle still build Windows 32bit?

[JornVernee]

Yes, this exists as an adapter from PROCEDURE to whatever callback type the OS API expects. I tried passing the procedure to the start API as well, but the compiler complains that the types don't match. Even if I make PROCEDURE return int, it seems to want DWORD explicitly.

This was taken from the old library code which uses _beginthreadex. That one explicitly wants to use __stdcall, so I just matched that. Looking again, it doesn't look like the same is required for CreateThread [1]. I'll remove it for clarity.

[1] : https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/ms686736(v=vs.85)

[dholmes-ora]

run_async is an odd name for this, especially as the fact you create then join mean it doesn't run asynchronously at all - it runs synchronously in another thread. The API would be more generally useful if you split the start and the join, but that would require also exposing an opaque thread "handle" (though splitting also means you don't have to try and think of a good name for run_in_a_new_thread-and_join_it() - you just have threadStart and threadJoin).

[erikj79]

Build changes look good.

[JornVernee]

run_async is an odd name for this, especially as the fact you create then join mean it doesn't run asynchronously at all - it runs synchronously in another thread.

I took the name from the CompleteableFuture API [1], although in that case a future is returned on which a user can call get. I can rename the function. Does run_in_new_thread seem good enough?

The API would be more generally useful if you split the start and the join, but that would require also exposing an opaque thread "handle" (though splitting also means you don't have to try and think of a good name for run_in_a_new_thread-and_join_it() - you just have threadStart and threadJoin).

I can see this being useful in order to start many threads up front, and then join then all afterwards (to get them running concurrently). Though, I'd like to hold off on getting into that since the current tests don't need that functionality (it could be added, but stay unused). The library header can be freely amended later as well.

[1] : https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html#runAsync(java.lang.Runnable)

[JornVernee]

I made some tentative changes based on the review comments:

• Added some code comments.
• Switch perror to fputs with stderr for clarity.
• Use the OS API error code as exit code.
• Switch the windows type of procedure to exactly what the CreateThread expects: https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/ms686736(v=vs.85)
• Renamed run_async to run_in_new_thread.

Please take another look.

[dholmes-ora]

Does run_in_new_thread seem good enough?

No, sorry, the fact it both runs and joins is a critical aspect. run_async in CompleteableFuture just does the "run in new thread" part, whereas the get() on the returned FutureTask provides the "join". So a function that does both needs to make that clear in the name - and there is no nice succinct name for that, so we get stuck with something like run_in_new_thread_and_join. Or we just have startThread and joinThread :)

[JornVernee]

Does run_in_new_thread seem good enough?

No, sorry, the fact it both runs and joins is a critical aspect. run_async in CompleteableFuture just does the "run in new thread" part, whereas the get() on the returned FutureTask provides the "join". So a function that does both needs to make that clear in the name - and there is no nice succinct name for that, so we get stuck with something like run_in_new_thread_and_join. Or we just have startThread and joinThread :)

startThread and joinThread is not the functionality that the tests need. The functionality that's needed is one that combines these two, so that's what I went with. An API without real users is harder to get right.

I've looked into adding startThread and joinThread. But, the story is not so simple. Since now the context passed to CreateThread and pthread_create has to outlive the function, which means heap allocating, or asking the caller to stack allocate. I think a typical C API might have a new_thread and delete_thread to do the memory management. These functions could be folded into startThread and joinThread, and we'd rename those to create_and_start_thread and join_and_destroy_thread (I think it's important to signal to callers that allocation happens, so that the memory will be cleaned up by calling join as well), but at that point it feels like we're back where we started. The alternative being stack allocation. i.e. the caller declares a THREAD variable and passes a pointer to it to start_thread, which then uses that memory. This avoids the heap allocation, but adds more noise in the caller.

Adding that just feels like too much unneeded complexity at this moment, since there are no users that need it. So, run_in_new_thread_and_join it is.

[dholmes-ora]

startThread and joinThread is not the functionality that the tests need.

But it is the functionality the tests are already using.

now the context passed to CreateThread and pthread_create has to outlive the function

That's not an issue for the API to solve, that is an issue for the user of the API to solve. pthread_create doesn't involve any memory management.

So, run_in_new_thread_and_join it is.

Okay, that is fine.

[JornVernee]

@tstuefe Do the changes I made look good? If so I'll go ahead and integrate.

[tstuefe]

Looks still good.

[JornVernee]

/integrate

[openjdk]

Going to push as commit 54a2c5a.
Since your change was applied there have been 62 commits pushed to the master branch:

• 5214a17: 8291479: ProblemList compiler/rangechecks/TestRangeCheckHoistingScaledIV.java on ppc64le
• 471a427: 8287794: Reverse*VNode::Identity problem
• 5d1ad39: 8290839: jdk/jfr/event/compiler/TestJitRestart.java failed with "RuntimeException: No JIT restart event found: expected true, was false"
• 97fc8de: 8291106: ZPlatformGranuleSizeShift is redundant
• dd69a68: 8291000: C2: Purge LoadPLocked and Store*Conditional nodes
• 07f0612: 8290532: Adjust PKCS11Exception and handle more PKCS11 error codes
• 93f96d8: 8290399: [macos] Aqua LAF does not fire an action event if combo box menu is displayed
• 5d82d67: 8290034: Auto vectorize reverse bit operations.
• 348a052: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
• bc6a3c7: 8290739: Simplify storage of dump-time class information
• ... and 52 more: https://git.openjdk.org/jdk/compare/7ec0132ad3129b805664c85351fe6d55041066fa...master

Your commit was automatically rebased without conflicts.

[openjdk]

@JornVernee Pushed as commit 54a2c5a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.