However, while investigating I found the threading issue that this patch aims to fix. @lhames Might the unjoined thread have caused the assertion failure in SymbolStringPool?

After commit 2f9ba6aa8b6d the test failure https://lab.llvm.org/buildbot/#/builders/61/builds/10796 occurred on one of the few build bots that include the examples. The patch doesn't affect any code exercised in this test and the bot turned green with the subsequent build. Thus, the test might be considered flaky. So far I failed to reproduce the exact error:

Hey, just a few notes on proper error handling.

In D102080#2744882, @lhames wrote:

We're already building runtime libraries, but in the tests it looks like we're building a parallel archive for the runtime and writing it to the test directory for the tests to link against. Is there a reason the tests can't just link against the built runtimes?

LGTM. Thanks for fixing this!

Rebase patch

Don't apply lazy-MCJIT tests to the default (greedy) mode for Orc. They probably work well with OrcLazy, but that's a different story.

Apply MCJIT small code-model tests to ORC in preparation for default JIT engine switch

This can be used to fix test failures on Windows when re-applying D98931

In D98931#2644686, @sgraenitz wrote:

I think 5949bd91253672ea8599a7a234b18e78972d3aa1 will fix the PowerPC bots

In D98931#2644236, @nemanjai wrote:

Can we get it reverted and we can try to do what we can to help investigate the failures on PowerPC machines?

Fix error No callback manager available on powerpc64le-unknown-linux-gnu with eccd7ae2fdb3

For now keep eh-lg-pic.ll a MCJIT-only test

Alright fixed with 9cdbdbea29ce

@thakis Thanks for the note. I guess it's D98579. Fix will be quick.

Thanks for the confirmation. I will land this as soon as D98579 is done.

Patch 5 other tests that slipped through

Make returnError() static (oops). Rename RTraits -> AltRetTraits. Turn some autos into concrete typenames to aid readability.

I actually didn't manage to reproduce the warning with GCC 10 on Debian, so I couldn't really test this. @thakis Do you want to run a pre-test? Otherwise, we could land it right away and see if it is effective.

Add special-case for calling accept(2) on AIX. I cannot test it here, @rzurob does this work for you?

Patch tests for MCJIT-specific behavior

Run tests for common JIT tasks against both, MCJIT and Orc

Thanks for elaborating.

Pending resources are irrelevant in both, notifyTransferringResources() and notifyRemovingResources(). They are guaranteed to be either finalizd in notifyEmitted() or discarded in notifyFailed().

This is hard to reproduce with lli, because single-threaded execution with LLLazyJIT will implicitly sequence materialization. LLJIT instead materializes module dependencies recursively, which makes this issue easy to reproduce. Unfortunately, we have no LLJIT tool that I could use for a test. It's not the first time this limitation comes up for me and I wonder if it's worth adding an orc-greedy kind to lli. What do you think?

Print 'Connection established' only when connecting via a TCP socket

Some sample output for llvm-jitlink with various broken addresses:
Failed to connect TCP socket 'localhost:1': Connection refused (inactive)
Failed to connect TCP socket '8.8.8.8:1': Operation timed out (non-responding)
Failed to connect TCP socket '127.0.0.1:0': Can't assign requested address
Failed to connect TCP socket '127.0.0.1:-1': Address resolution failed (nodename nor servname provided, or not known)
Failed to connect TCP socket '127.0.0.1': Port for -oop-executor-connect can not be empty

Follow-up patch that submits learnings from fixing D98579. The error it fixed would now print as:
Failed to connect TCP socket 'localhost:13921': Address resolution failed (Bad hints)

Unfortunately, we seem to have no test coverage for this code. My last take was that LIT doesn't exactly make it easy to run client and server concurrently. Is that still the case or could we do it somehow differently?
I had to take a detailed look into this code now anyway, so I could just as well add a test case.

The patch proposed here is a minimal fix. I am eager to streamline the code and add better error messages in a follow-up commit.

Thanks. From my side this looks good now.

Thanks for elaborating. LGTM

I think you're right, but rather than approach this through the plugin API I think the solution will come from rethinking "finalization" on the memory manager API

I think this makes good progress. I found two details in the test code that need attention. The stdout issue might be done now or in a follow-up patch some time soon. Otherwise, this seems to get ready to land.

Factoring out architecture specific code sounds like a reasonable approach to reduce code duplication across the JITLink backends. As far as I can see, the patch only ports the MachOJITLinker_x86_64 to use the generic functionality. The ELFJITLinker_x86_64 mostly remains unchanged. Is there a conceptual reason for it or would that be a rather straightforward next step?

This means that materialization is blocked until finalization of the debug object allocation is done, i.e.:

• memory was copied over to the target
• an entry was added to the JIT descriptor (locks a global mutex)
• the executing process ran into the rendezvous breakpoint (1st process switch)
• the debugger processed the debug object, read the symbol table and returned control back to the executor (2nd process switch)

In D97335#2614807, @ASDenysPetrov wrote:

Right. Probably the action was choosed previously in the drop-down list and I missed it. Sorry.

@ASDenysPetrov I guess you set the Needs Revision status accidentally?

Attempting to close the review after it was re-opened.

For the entry point, prefer lli over llvm-jitlink. D97339 added debug support for JITLink in lli.

In D97335#2603542, @ASDenysPetrov wrote:

My build fails after hack to this patch https://reviews.llvm.org/rG900f076113302e26e1939541b546b0075e3e9721. Could you revise your solution?

@klausler Thanks for reporting this. lli was missing JITLink as a link component in CMake. I fixed it with https://reviews.llvm.org/rG295ea050ad594b8868b6dd944824ba9a16273f91. Sorry for the inconvenience.

This fixes the symbol lookup issue on Windows that was discussed post-review in D97339. Sorry for the inconvenience.