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.

Sign up for GitHub

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

Jump to bottom

Build support for s390x: PAL layer #53287

Merged

marek-safar merged 1 commit into dotnet:main from uweigand:s390x-clr-pal

Jun 8, 2021

Contributor

uweigand commented May 26, 2021

Add PAL implementation for Linux on s390x
Define BIGENDIAN on s390x when compiling coreclr
Do not run HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES cmake test when
cross-compiling

Note to reviewers:

The primary intent of this patch is to enable building ilasm/ildasm on s390x (even though we do not support coreclr VM/JIT)
However, the PAL support provided here is still complete and passes all paltests
As there is no Windows on s390x, there is no pre-existing CONTEXT structure, so I made one up. In particular, the CONTEXT_S390X define re-uses the same value as CONTEXT_AMD64. Let me know if I should instead choose another bit.
Similarly, there is no s390x PE file format, so IMAGE_FILE_MACHINE_NATIVE is defined to IMAGE_FILE_MACHINE_UNKNOWN

dotnet-issue-labeler bot added the area-PAL-coreclr label

Member

AaronRobinsonMSFT commented May 26, 2021

janvorli reviewed

View reviewed changes

src/coreclr/pal/src/configure.cmake Outdated

    
            @@ -1132,7 +1132,7 @@ int main()
          
              }" HAVE_FULLY_FEATURED_PTHREAD_MUTEXES)

              set(CMAKE_REQUIRED_LIBRARIES)

              if(NOT CLR_CMAKE_HOST_ARCH_ARM AND NOT CLR_CMAKE_HOST_ARCH_ARM64)

              if(NOT CMAKE_CROSSCOMPILING AND NOT CLR_CMAKE_HOST_ARCH_ARM AND NOT CLR_CMAKE_HOST_ARCH_ARM64)

Member

janvorli May 26, 2021

This is never executed while cross compiling, cmake skips all check_*_source_runs during cross compilation. The HAVE_xxx values for cross compilation are taken from the tryrun.cmake file. So this change should not be needed, but you'll need to update the eng/native/tryrun.cmake

Contributor Author

uweigand May 27, 2021

Ah, I missed that, sorry. Patch updated accordingly.

src/coreclr/pal/src/thread/context.cpp Outdated Show resolved Hide resolved

src/coreclr/inc/pedecoder.h Show resolved Hide resolved

runfoapp bot mentioned this pull request

FileSystemWatcher_DirectorySymbolicLink_TargetsFile_Fails #53366

Open

Contributor Author

uweigand commented Jun 7, 2021

Ping? Anything else that needs to be addressed?

janvorli reviewed

View reviewed changes

src/coreclr/pal/src/thread/context.cpp Show resolved Hide resolved

src/coreclr/pal/src/thread/context.cpp Outdated Show resolved Hide resolved

src/coreclr/pal/src/thread/context.cpp Outdated

    
            @@ -436,7 +462,7 @@ void CONTEXTToNativeContext(CONST CONTEXT *lpContext, native_context_t *native)
          
              #undef ASSIGN_REG

              #if !HAVE_FPREGS_WITH_CW

              #if HAVE_GREGSET_T || HAVE_GREGSET_T

              #if (HAVE_GREGSET_T || HAVE_GREGSET_T) && !defined(HOST_S390X)

Member

janvorli Jun 7, 2021

I have just noticed this #if has a wrong condition (event before your change). Since you are touching this code, could you please fix it? It should be

#if (HAVE_GREGSET_T || HAVE___GREGSET_T) && !defined(HOST_S390X)

Actually, the related #endif comment need to be updated to match your change (and this fix) to

#endif // HAVE_GREGSET_T || HAVE___GREGSET_T || !HOST_S390X

Contributor Author

uweigand Jun 7, 2021

Fixed.

src/coreclr/pal/src/thread/context.cpp

    
            @@ -562,7 +592,7 @@ void CONTEXTFromNativeContext(const native_context_t *native, LPCONTEXT lpContex
          
              #undef ASSIGN_REG

              #if !HAVE_FPREGS_WITH_CW

              #if HAVE_GREGSET_T || HAVE___GREGSET_T

              #if (HAVE_GREGSET_T || HAVE___GREGSET_T) && !defined(HOST_S390X)

Member

janvorli Jun 7, 2021

Can you please update the matching @endif comment?

Contributor Author

uweigand Jun 7, 2021

Done.

janvorli reviewed

View reviewed changes

src/coreclr/pal/src/thread/context.cpp Outdated

    
            @@ -448,7 +474,7 @@ void CONTEXTToNativeContext(CONST CONTEXT *lpContext, native_context_t *native)
          
                      // whether CONTEXT_FLOATING_POINT is set in the CONTEXT's flags.

                      return;

                  }

              #endif // HAVE_GREGSET_T || HAVE_GREGSET_T

              #endif // (HAVE_GREGSET_T || HAVE___GREGSET_T) && !defined(HOST_S390X)

Member

janvorli Jun 7, 2021

A nit - in the #endif comments, the convention we use is just !HOST_S390X instead of !defined(HOST_S390X)

Contributor Author

uweigand Jun 7, 2021

Ah, I see. Updated.


          Build support for s390x: PAL layer

* Add PAL implementation for Linux on s390x

* Define BIGENDIAN on s390x when compiling coreclr

* Provide a default HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES_EXITCODE
  value in eng/native/tryrun.cmake

janvorli approved these changes

View reviewed changes

Member

janvorli left a comment

LGTM, thank you!

janvorli added the auto-merge label

ghost commented Jun 7, 2021

Hello @janvorli!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Contributor Author

uweigand commented Jun 7, 2021

Thanks for the review! Would you mind having a look the two related PRs as well:
#53286
#53289
Both are required to get the PAL to actually build on s390x.

marek-safar merged commit 5d6278a into dotnet:main

ghost locked as resolved and limited conversation to collaborators

uweigand deleted the s390x-clr-pal branch

July 14, 2021 22:32

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-PAL-coreclr