Threading unit tests and linux threading implementation

[bwrsandman]

Current Status on master

On Linux, the Xenia app and many of the test ui tests do not run correctly due to waits on WaitHandles flowing through and returning early.

Fix status of this PR

I have been working on this PR for about a year and it now has reached a point where I consider it done and ready for review.

This PR implements most of threading_posix.cc by TDD to make sure behaviour is the same as win32.
This is a follow-up to (#1108) in order to get xenia to actually run on Linux. Following this, a few GTK-Vulkan fixes need to be done as well as implementing POSIX file system stubs. With those, a GTK window actually appears and responds to events.

Fix description

Borrowed commit from #1076 for Linux debug symbols.

Fixed occurrences of micro/millisecond confusion on timers.

Posix threads do not support suspension in the same way as windows threads. Real-time interrupts used to give this functionality and to implement thread callback when in alertable state.
More real-time interrupts used to signal timers.
Since there are more than two signals used, SIGUSR1 and SIGUSR2 are not used. Signals are allocated between SIGRTMIN and SIGRTMAX which are dynamic and can change per machine or kernel version but should start from SIGUSR2 + 1.

Not all systems support real-time signals. bits/signum.h indicates
that they are supported by overriding __SIGRTMAX to a value greater
than __SIGRTMIN. These constants give the kernel-level hard limits,
but some real-time signals may be used internally by glibc. Do not
use these constants in application code; use SIGRTMIN and SIGRTMAX
(defined in signal.h) instead.

/* Return number of available real-time signal with highest priority.  */
extern int __libc_current_sigrtmin (void) __THROW;
/* Return number of available real-time signal with lowest priority.  */
extern int __libc_current_sigrtmax (void) __THROW;

#define SIGRTMIN        (__libc_current_sigrtmin ())
#define SIGRTMAX        (__libc_current_sigrtmax ())

Real-time interrupts handlers are installed in a lazy-load fashion. This could be moved to an initialize phase to avoid lazy-load, but no obvious location for this was found.

Waiting is implemented using static condition variables. To do this, each WaitHandle is also an instance of PosixConditionBase, the latter which has the same functionality of wait and wait multiple. This implementation was more successful than using the existing PosixFdHandle which was then removed in order to use a more generalizable super class which gave the added bonus of waiting on multiple types of primitives at the same time since they all use the same conditional variables.

Alertable state is set as a thread local static variable.

I ran these tests multiple times concurrently in Windows and Linux on my machine, my laptop and on travis. I will also test it on my LattePanda Alpha SBC to test a lower-end machine. Throughout the development, there have been fails which are not always reproducible and I brought this number down with fixes and hopefully there aren't any rare bugs left.

Things left to check later

Suspending an already suspended thread was not tested nor implemented and left for a later time.

Implementing affinity and priority was attempted but in the end abandoned due to Linux's technicalities.
I could be wrong but there doesn't seem to be any way to set a thread's priority which is higher than the main thread's without running as a super user so we could only set it to Normal or Lower.

Sleeping for times lower than 1ms works on my setups but causes deadlocks on Travis. This is no problem for the unit tests. The times were just increased by factors of 10 to 100. This, however, means that tests which run in about 5s could potentially run in < 1s if that deadlock issue is fixed.

There will always be fundamental differences between Windows and Linux which could be tested against. My hope is that the existence of these unit tests can make pin pointing these differences more easy.

[Razzile]

Hot damn this is amazing work!

[bwrsandman]

Note to reviewers:
Please let me know if there are any changes to be made regarding architecture, code style, naming, spelling or if there are any tests to add.

If you check out the branch and run into issues, please send me a coredump if one is produced. Let me know what distro you run, which kernel version, the compiler with version, machine specs (cpu, number of cores, memory, load) and reproduceability. Having all this information is incredibly useful to find and fix threading bugs.

Any comment is appreciated and I will try to make requested changes within a week.

[chris-hawley]

First of all. This is excellent work. This has been a long-standing missing component of the emulator.

I cloned your repository and then rebased it against the master for the latest fixes. I implemented the filesystem headers required and tried running it. I had some issues with it crashing and ran it under valgrind a few times and noticed this. To be clear, the crash wasn't caused by this output. The vulkan backend was crashing. Given my troubles with Vulkan, could you verify this behavior and fix if necessary? I have also added the three functions that were required to get the filesystem working.

==15421== Thread 2 :
==15421== Invalid read of size 1
==15421==    at 0x201FB4: xe::threading::PosixCondition<xe::threading::Thread>::ThreadStartRoutine(void*) (threading_posix.cc:906)
==15421==    by 0x55BEA9C: start_thread (in /usr/lib/libpthread-2.28.so)
==15421==    by 0x5D98B22: clone (in /usr/lib/libc-2.28.so)
==15421==  Address 0x87beab0 is 32 bytes inside a block of size 48 free'd
==15421==    at 0x48389AB: free (vg_replace_malloc.c:530)
==15421==    by 0x201F73: xe::threading::PosixCondition<xe::threading::Thread>::ThreadStartRoutine(void*) (threading_posix.cc:901)
==15421==    by 0x55BEA9C: start_thread (in /usr/lib/libpthread-2.28.so)
==15421==    by 0x5D98B22: clone (in /usr/lib/libc-2.28.so)
==15421==  Block was alloc'd at
==15421==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==15421==    by 0x569AE19: operator new(unsigned long) (in /usr/lib/libc++.so.1.0)
==15421==    by 0x203EFA: xe::threading::PosixThread::Initialize(xe::threading::Thread::CreationParameters, std::function<void ()>) (threading_posix.cc:836)
==15421==    by 0x20227A: xe::threading::Thread::Create(xe::threading::Thread::CreationParameters, std::function<void ()>) (threading_posix.cc:938)
==15421==    by 0x1ECA0D: xe::Logger::Logger(std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&) (logging.cc:71)
==15421==    by 0x1EBDF2: xe::InitializeLogging(std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&) (logging.cc:245)
==15421==    by 0x1C9AFE: main (main_posix.cc:37)

#include "xenia/base/assert.h"
#include "xenia/base/filesystem.h"
#include "xenia/base/logging.h"
#include "xenia/base/string.h"

#include <dirent.h>
#include <fcntl.h>
#include <ftw.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <libgen.h>
#include <pwd.h>
#include <iostream>

namespace xe {
namespace filesystem {

std::wstring GetExecutablePath() {
	char buff[FILENAME_MAX] = "";
	readlink("/proc/self/exe", buff, FILENAME_MAX);
  std::string s(buff);
  return to_wstring(s);
}

std::wstring GetExecutableFolder() {
  auto path = GetExecutablePath();
  return xe::find_base_path(path);
}

std::wstring GetUserFolder() {
  struct passwd *pw = getpwuid(getuid());
  char *homedir = pw->pw_dir;
  std::string home(homedir);
  return to_wstring(home+"/.local/share/");
}

[gibbed]

Consider this high on my priority list, I'll review it as soon as I am able. Great work!

[bwrsandman]

@chris-hawley I plan to tackle vulkan on linux next.
I have only spent a little time looking at the issues with vulkan (to get xenia-app running) as most of my time was spent at a lower level testing against the threading unit tests directly.

Other than the filesystem stubs which are easy to fix with hard-coded values, I identified two issues :

• The vulkan GTK window is not setup correctly
  • The xcb surface requirement isn't asked for (a9c2053)
  • The xcb connection/native_platform_handle isn't set (0e5af7d)
• The VkDebugReportCallbackCreateInfoEXT isn't properly set (32a0e25)

I am pretty familiar with Vulkan and graphics so I plan on taking a more in-depth look at the implementation and following up with fixes in another pull request.

As for:

==15421== Thread 2 :
==15421== Invalid read of size 1
...

I haven't seen this message before, do you have a branch I can check out?

[chris-hawley]

I'll create a branch called linux-testing in my xenia fork.

[bwrsandman]

Great. By the way, you can see my Vulkan work and changes on this branch. Subject to change.

[chris-hawley]

branch is all set to go

[bwrsandman]

@chris-hawley The is due to the xcb connection/native_platform_handle not being set (0e5af7d) and the crash happens within either gtk or the graphics driver (in my case nvidia).

If you cherry-pick a9c2053 and 0e5af7d from my branch, you won't get those issues.

Anyways, vulkan crashing is a separate issue which will be resolved with another PR.

[chris-hawley]

that fixed the vulkan crashing.

[chris-hawley]

Also, we have a discord if you would rather do this back and forth there. discord

[Triang3l]

GetUserFolder should have the following logic (@Prism019's comment on #1316 is related to this):

• If the environment variable XDG_DATA_HOME is set, the path is $XDG_DATA_HOME/Xenia.
• If it's not set:
  • If the environment variable HOME is set, the path is $HOME/.local/share/Xenia.
  • Optionally (I don't know how realistic this situation is, other apps don't always handle it), if HOME is not set, use getpwuid(getuid())->pw_dir as $HOME (this is not always the same as $HOME — the user may override the home directory, but $HOME takes precedence in this case).

[bwrsandman]

I fixed some memory leaks that were pointed out by valgrind.
They occurred due to threads not being joined after being cancels as in the case of Terminate().
There was a unit test not joining with threads.
There was a case of use after delete of thread start_data.

[DrChat]

Preliminary review with some cosmetic requests.
This is really cool, and using conditional variables is a very interesting solution! Good work!

[bwrsandman]

Thanks for the review @DrChat. I have made the requested changes and rebased on master.
Have you had a chance to run it yourself?
Please let me know if there's anything else to do.

[DrChat]

So, I didn't merge this at the time because of the complex nature of this pull request - it's modifying a delicate part of the emulator, which can introduce very subtle bugs.

The changes here look pretty well thought out though, and I'm leaning towards merging this. One thing though: What about that deadlock on Travis from the unit tests?

[bwrsandman]

// TODO(bwrsandman): due_times of under 1ms deadlock under travis

This was something which only happened on Travis. I wasn't able to get to the bottom of this since none of my machines could replicate. If I had to guess it's the nature of the container and the kernel used is somehow interpreting less than 1 ms as infinite.
I would love to have this fixed as it could speed up the tests by up to 1000x (switching ms to ns).
Unfortunately, since I can't replicate it outside of travis, it's difficult.

[bwrsandman]

Rebased to latest master