Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAL compatibility issues #6404

Open
rhuanjl opened this issue Apr 3, 2020 · 10 comments
Open

PAL compatibility issues #6404

rhuanjl opened this issue Apr 3, 2020 · 10 comments

Comments

@rhuanjl
Copy link
Collaborator

rhuanjl commented Apr 3, 2020

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
Copy link
Member

ppenzin commented Apr 8, 2020

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
Copy link
Collaborator Author

rhuanjl commented Apr 8, 2020

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
Copy link
Member

ppenzin commented Apr 9, 2020

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
Copy link
Member

ppenzin commented Apr 19, 2020

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

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 4, 2020

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
Copy link
Member

ppenzin commented Jan 6, 2021

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
Copy link
Member

ppenzin commented Jan 9, 2021

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
Copy link
Collaborator Author

rhuanjl commented Jan 9, 2021

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

@ppenzin
Copy link
Member

ppenzin commented Feb 12, 2021

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 added a commit to ppenzin/ChakraCore that referenced this issue Feb 12, 2021
Remove externally visible redifinitions of math and memory management
functions. Delete corresponding math function implementations, as they
pretty much call their system counterparts. Hide memory management
functions, since they are still used from inside of PAL.

CC chakra-core#6404
ppenzin added a commit to ppenzin/ChakraCore that referenced this issue Feb 12, 2021
Drive by cleanup - build all wabt files in the build directory.

Disambiguate template parameter names. Overloading a template parameter
by a declaration (including in parameters to nested templates) is
treated as an error by Clang 10.

PAL math and memory API clean up (also for chakra-core#6404). Remove PAL
redefintions of system functions with incompatible exception
declarations. Remove externally visible redifinitions of math and memory
management functions. Delete corresponding math function
implementations, as they pretty much call their system counterparts.
Hide memory management functions, since they are still used from inside
of PAL.

[CI] Use Ubuntu 20.04 image for default Linux builds. Change default
Linux image to Ubuntu 20.04. Add two extra build jobs for Ubuntu 18.04.

Closes chakra-core#6600
rhuanjl pushed a commit that referenced this issue Feb 12, 2021
Drive by cleanup - build all wabt files in the build directory.

Disambiguate template parameter names. Overloading a template parameter
by a declaration (including in parameters to nested templates) is
treated as an error by Clang 10.

PAL math and memory API clean up (also for #6404). Remove PAL
redefintions of system functions with incompatible exception
declarations. Remove externally visible redifinitions of math and memory
management functions. Delete corresponding math function
implementations, as they pretty much call their system counterparts.
Hide memory management functions, since they are still used from inside
of PAL.

[CI] Use Ubuntu 20.04 image for default Linux builds. Change default
Linux image to Ubuntu 20.04. Add two extra build jobs for Ubuntu 18.04.

Closes #6600
@ppenzin
Copy link
Member

ppenzin commented Apr 25, 2024

Recap of ways we depend on PAL from our discussion:

  • Thread API
  • String API (mentioned above)
  • Potentially some parts of calling conventions and stack frame layout - PAL comes up for JIT and exception handling
  • File handling, probably minimal
  • Maybe additional Win32 types

The parts are likely at least somewhat interdependent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants