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

Fix zlib-ng compilation on Raspberry Pi ARM32 #106586

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Aug 18, 2024

Fixes #106584

Note that we don't lose any optimized code paths since NEON is enabled:

  -- The following features have been enabled:
  
   * CMAKE_BUILD_TYPE, Build type: DEBUG (selected)
   * ACLE_CRC, Support ACLE optimized CRC hash generation, using "-march=armv8-a+crc"
   * NEON_ADLER32, Support NEON instructions in adler32, using "-mfpu=neon"
   * NEON_SLIDEHASH, Support NEON instructions in slide_hash, using "-mfpu=neon"
   * WITH_GZFILEOP, Compile with support for gzFile related functions
   * ZLIB_COMPAT, Compile with zlib compatible API
   * WITH_SANITIZER, Enable sanitizer support
   * WITH_GTEST, Build gtest_zlib
   * WITH_OPTIM, Build with optimisation
   * WITH_NEW_STRATEGIES, Use new strategies
   * WITH_ACLE, Build with ACLE
   * WITH_NEON, Build with NEON intrinsics

On the unsupported ARMv6 Mono builds the system Zlib is used instead.

@filipnavara
Copy link
Member Author

The windows-arm64 build failure is unrelated. It was fixed yesterday by #106578.

@am11
Copy link
Member

am11 commented Aug 19, 2024

On the unsupported ARMv6 Mono builds the system Zlib is used instead.

nit: it's a community-supported platform, not unsupported. 😉

@carlossanlop
Copy link
Member

We were already blocking armv6 here, so I am wondering why it is not working: https://github.com/dotnet/runtime/blob/main/eng/native/configureplatform.cmake#L511-L516

@filipnavara
Copy link
Member Author

filipnavara commented Aug 19, 2024

We were already blocking armv6 here, so I am wondering why it is not working: https://github.com/dotnet/runtime/blob/main/eng/native/configureplatform.cmake#L511-L516

This is ARMv7 system though, so it is (correctly) not blocked.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations. The comment seems to be descriptive enough about the mismatch.

If this is not an upstream issue in zlib-ng as you mentioned, but rather an issue in runtime, could you please then open an issue for us to continue the discussion around this mismatch and make sure we get it fixed when the time comes?

@carlossanlop
Copy link
Member

/ba-g The CI failure in crossaot is unrelated to your PR and has already been fixed in main: #106578

@filipnavara
Copy link
Member Author

could you please then open an issue for us to continue the discussion around this mismatch and make sure we get it fixed when the time comes?

I can open an issue but I don't think there's anything else to fix.

ARMv6 is community supported platform that uses system Zlib. There's no degradation from disabling this code path on the supported ARMv7 platforms since they have NEON available and zlib-ng thus gets the fast paths. We may want to double check what happens when cross-compiling but the PR should certainly not make it worse.

Using thumb instruction set in CoreCLR is conscious choice that is unlikely to be changed.

@carlossanlop
Copy link
Member

I can open an issue but I don't think there's anything else to fix.

Alright, no problem, I trust your assessment. Do you have permission to hit the merge button or would you like me to do it?

@filipnavara
Copy link
Member Author

Do you have permission to hit the merge button or would you like me to do it?

Please merge it, I don’t have the permissions. Thanks!

@carlossanlop
Copy link
Member

I also forgot to ask: do you know if the runtime-community azp run tests the affected architecture? Asking in case we should execute a run just for peace of mind.

@filipnavara
Copy link
Member Author

filipnavara commented Aug 19, 2024

It should be covered by the regular runtime tests for linux-arm. If you want to be extra cautious then /azp run runtime-nativeaot-outerloop has coverage for the build and runtime paths. There are no legs to cover full build on linux-arm that’s not cross-build (AFAIK).

@carlossanlop
Copy link
Member

All the linux-arm legs passed. I think that should be enough. Thanks.

@carlossanlop carlossanlop merged commit 29f3687 into dotnet:main Aug 19, 2024
146 of 149 checks passed
@filipnavara filipnavara deleted the patch-17 branch August 19, 2024 20:44
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zlib-ng compilation fails on Raspbian ARM32 system
3 participants