jackaudio / jack2

jack2 codebase
GNU General Public License v2.0
2.16k stars 372 forks source link

Crash in Windows when calling 'jack_client_stop_thread ()' #720

Open johne53 opened 3 years ago

johne53 commented 3 years ago

Before I pass this to the Mixbus devs I'm just hoping someone here can help me make sense of it.... I'm building Mixbus with VS2019, having previously built it with much older VS compilers. Also, I'm now building as 64-bit - whereas my previous builds were always 32-bit. Part of Mixbus's code has a function looking like this:-

int
JACKAudioBackend::join_process_threads ()
{
    int ret = 0;

    for (std::vector<jack_native_thread_t>::const_iterator i = _jack_threads.begin ();
         i != _jack_threads.end(); i++) {

#if defined(USING_JACK2_EXPANSION_OF_JACK_API) || defined __jack_systemdeps_h__
        // jack_client is not used by JACK2's implementation
        // also jack_client_close() leaves threads active
        if (jack_client_stop_thread (NULL, *i) != 0) //  <--- THIS CALL CRASHES !!!
#else
        void* status;
        if (pthread_join (*i, &status) != 0)
#endif
        {
            error << "AudioEngine: cannot stop process thread" << endmsg;
            ret += -1;
        }
    }

    _jack_threads.clear();

    return ret;
}`

As you can see, the call to 'jack_client_stop_thread()' crashes in my 64-bit+VS2019 build. I found it defined like this:-

int jack_client_stop_thread(jack_client_t* client, jack_native_thread_t thread) JACK_OPTIONAL_WEAK_EXPORT;

and I found this code in 'weakjack.h':-

#ifndef JACK_OPTIONAL_WEAK_EXPORT
/* JACK_OPTIONAL_WEAK_EXPORT needs to be a macro which
   expands into a compiler directive. If non-null, the directive
   must tell the compiler to arrange for weak linkage of
   the symbol it used with. For this to work fully may
   require linker arguments for the client as well.
*/
#ifdef __GNUC__
#define JACK_OPTIONAL_WEAK_EXPORT __attribute__((WEAK_ATTRIBUTE))
#else
/* Add other things here for non-gcc platforms */
#endif
#endif

The above comment suggests that I'd need to #define JACK_OPTIONAL_WEAK_EXPORT for a VS2019 build. However, I'd never needed to do that with earlier MSVC compilers or for a 32-bit build -- but in any case -- Mixbus's own code suggests that 'jack_client_stop_thread()' shouldn't be getting called anyway when using Jack2.

So what's the problem likely to be? Is it that Mixbus shouldn't be calling 'jack_client_stop_thread()' - Or is it that I haven't #defined JACK_OPTIONAL_WEAK_EXPORT ??

falkTX commented 3 years ago

The issue is likely the change from msvc compiler to mingw, so the jack_native_thread_t changed from being a HANDLE to a pthread_t. That would explain the crash.

johne53 commented 3 years ago

Many thanks falkTX - that does feel kinda plausible. Presumably though, it would affect any function using jack_native_thread_t In my particular case, jack_client_stop_thread() is the only one that's crashing.

What about the comment (in Mixbus) that jack_client isn't used by Jack2's implementation. If that's true, should jack_client_stop_thread() even be getting called?

falkTX commented 3 years ago

sadly I dont have any experience with the callback-style of audio that ardour/mixbus uses to comment on this issue properly.

if you use the old jack installers, does it still crash? https://github.com/jackaudio/jackaudio.github.com/releases/tag/1.9.11

I can do some new jack2 builds with debug enabled, if you think that can help here.

johne53 commented 3 years ago

I tried an older installer earlier (1.9.12) and it still crashed but I'll give some thought to running a debug version. In the meantime....

I'm using a version of libpthread called pthread-win32. I'm running the 64-bit build where pthread_t is defined like this:-

typedef struct {
    void * p;                   /* Pointer to actual object */
    unsigned int x;             /* Extra information - reuse count etc */
} ptw32_handle_t;

typedef ptw32_handle_t pthread_t;

Is it the same in your Windows build?

falkTX commented 3 years ago

I have the native/internal mingw version of pthreads, not from pthread-win32 project. This has

typedef uintptr_t pthread_t;

But the JACK library does not expose any pthread symbols (it builds against it statically), so if you use and link to your own pthread and then call into JACK with pthread handles, I dont think that can work.

johne53 commented 3 years ago

I'll check tomorrow but I' think now that I'm also using pthreads (I don't build it myself). So it looks like I might be using the wrong header files (i.e. still the old ones from pthread-win32).

One more thing before I shut up for the night....

Mixbus calls 'jack_client_stop_thread()' with its first parameter set to NULL. Is that still accepted as valid these day?

falkTX commented 3 years ago

Mixbus calls 'jack_client_stop_thread()' with its first parameter set to NULL. Is that still accepted as valid these day?

According to https://github.com/jackaudio/jack2/blob/develop/common/JackAPI.cpp#L1759 and then https://github.com/jackaudio/jack2/blob/develop/windows/JackWinThread.cpp#L168 yes. The JackPosixThread has a similar check.

falkTX commented 3 years ago

btw, something I just noticed because of this. on windows builds, JackWinThread.cpp is always used, no matter if using mingw or msvc.

So this part might need updating https://github.com/jackaudio/jack2/blob/develop/common/jack/systemdeps.h#L91

johne53 commented 3 years ago

I can't find the original zip file but I checked my copy of pthread.h and it's got this message near the top:-

/*
 * See the README file for an explanation of the pthreads-win32 version
 * numbering scheme and how the DLL is named etc.
 */
#define PTW32_VERSION 2,9,1,0
#define PTW32_VERSION_STRING "2, 9, 1, 0\0"

So it looks like I'm still using pthread-win32. Do you know if your version is downloadable from somewhere - or is it only available as part of mingw?

falkTX commented 3 years ago

quick search online gives me main page and github mirror https://sourceware.org/pthreads-win32/

But I do not think that is relevant here. on Windows, the jack builds are using the win32 API to deal with threads, not pthread stuff. the stuff in the headers is likely there for those that self-compile jack2 on windows.

one thing you can try is to use pthread_getw32threadhandle_np on your pthread instance, so you get the win32 handle, and pass that one into jack.