chakra-core / ChakraCore

ChakraCore is an open source Javascript engine with a C API.
MIT License
9.06k stars 1.19k forks source link

PAL compatibility issues #6404

Open rhuanjl opened 4 years ago

rhuanjl commented 4 years ago

EDIT: ideal goal is to remove all dependence on PAL, but this will be a large project

  1. PAL was added to ChakraCore in ~2016 to make it build on Linux and MacOS.

  2. Since then it has been updated a little - occasionally new content from dotnet has been brought over but not in a full or systematic way.

  3. Additionally various parts of PAL have been edited for ChakraCore either in some cases by just removing bits that weren't really needed to slim the build down (e.g. most of SEH was taken out to remove the libunwind dependency, and the stop the synchmanager from starting a thread as it wasn't being used) or other cases by tweaking/enhancing.

  4. The result of 1, 2 and 3 is a very different version of PAL; which is fine as long as chakracore is using it on its own BUT if you're using something else that interacts with it; e.g. if you're using dotnet, then there are problems.

6403 fixes a known presenting issue - but it is a bit of a hack fix and its unclear if it could introduce an alternate problem. It may be worth attempting to update the whole of PAL to the latest from dotnet and then re-applying some of the historic chakracore changes - this could be a reasonably large piece of work, but could present future problems.

ppenzin commented 4 years ago

Ideally we would just depend on PAL in some form or the other (for example via CMake or git submodules), rather than have out of date partial copy checked into the sources, though dotnet does not seem to provide a way to grab just PAL sources (without the rest of dotnet) on unix-like OSes.

Side note is that PAL is added to provide Win32-like APIs to get to system functionality, to which there are alternatives, but those won't be very simple to implement immediately.

rhuanjl commented 4 years ago

Ideally we would just depend on PAL in some form or the other (for example via CMake or git submodules), rather than have out of date partial copy checked into the sources, though dotnet does not seem to provide a way to grab just PAL sources (without the rest of dotnet) on unix-like OSes.

The downside of taking it as a dependency - apart from any difficulty setting that up - would be the inclusion of parts of it that CC does not need including for instance bringing back the libunwind dependency.

Side note is that PAL is added to provide Win32-like APIs to get to system functionality, to which there are alternatives, but those won't be very simple to implement immediately.

Agreed - in an ideal world direct system calls would be used instead of PAL but that could be much harder work and would involve writing quite a bit of Linux and MacOs specific code. I'd have to do more digging to see what all the features that are used from PAL are, the obvious ones that are used heavily are thread management and string handling.

ppenzin commented 4 years ago

Agreed - in an ideal world direct system calls would be used instead of PAL but that could be much harder work and would involve writing quite a bit of Linux and MacOs specific code. I'd have to do more digging to see what all the features that are used from PAL are, the obvious ones that are used heavily are thread management and string handling.

There is a good amount of changes between dotnet and CC - mostly the latter is out of date, but there are changes in other direction as well, I think driven by libunwind removal.

Letting PAL diverge further we will face some maintenance challenges and end up fixing bugs in PAL dotnet already fixed. I'll try to ask around how we can reduce the effort needed for PAL maintenance.

This is relatively hard, solution might not come right away :)

ppenzin commented 4 years ago

5876 is a also worth mentioning, looks like thread tracking in PAL is having some issues.

rhuanjl commented 4 years ago

I'm going to work on updating PAL prior to our release - though it will be painful. In the long run I'd like us to remove the dependency all together but that will be more work and may want someone who's actually written multi-threaded code before (as the most interesting parts of the PAL code are the thread initialisation tracking and messaging logic), I know the theory but have never written any.

ppenzin commented 3 years ago

I've started doing research into removing PAL. First thing coming to mind is BSTR and friends - those types are only used for system calls, I think we can come up with a wrapper to invoke those natively on Windows, while not using BSTR on other OSes (BSTR is a string with size prepended to it). A good starting point may be converting WCHAR to wchar_t, I don't think we are likely to build using a toolchain without native wchar_t support.

ppenzin commented 3 years ago

I started to work on WCHAR vs wchar_t vs char16_t and it opens a bit of a can of worms. We can keep UTF-16, this would require carefully replacing WCHAR with char16_t, which would require adding includes of relevant char headers on Windows.

Between flavors of Unicode, UTF-8 is the most cross-platform. We can use it (#6570), and convert to UTF-16 when needed. This should be only the case for pre-Win10 OSes.

rhuanjl commented 3 years ago

Could we use our own #defines to avoid needing extra headers?

ppenzin commented 3 years ago

Another PAL gripe: since PAL functions as a system runtime, meaning that fixing anything its headers triggers a full rebuild. If build fails at 50% with something PAL-related and you make a header change you have to get to the same point again to see if it even succeeded. There are probably ways to work around this, but from the developer experience point of view this is not ideal.

ppenzin commented 2 months ago

Recap of ways we depend on PAL from our discussion:

The parts are likely at least somewhat interdependent.