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.

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

Add a proxy for ber scanf/printf varargs native methods. #50540

Closed
wants to merge 3 commits into from
Closed

Add a proxy for ber scanf/printf varargs native methods. #50540

wants to merge 3 commits into from

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Apr 1, 2021 •
edited
Loading

The PR adds wrappers for native varargs methods ber_scanf and ber_printf to fix failures on Apple arm64, closes #49105.

It was working on other platforms because vararg methods were expecting the arguments in the same registers as regular calls. It does not work for Apple OSX and for consistency I have decided to use these wrappers on all platforms. If these methods are performance-critical we could reconsider it.

This change could be done reverted if we implement #48796.

Until that we would need similar wrappers for all varargs for Apple Arm64, cc @sdmaclea , @janvorli , @JulieLeeMSFT, @mangod9.

@sandreenko
Copy link
Contributor Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@sandreenko
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop, runtime-libraries-coreclr outerloop-osx

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

jkotas
jkotas reviewed Apr 1, 2021
View reviewed changes
src/libraries/pretest.proj Outdated Show resolved Hide resolved
jkotas
jkotas reviewed Apr 1, 2021
View reviewed changes
src/libraries/Native/Unix/LdapProxy.Native/CMakeLists.txt Outdated
@@ -0,0 +1,61 @@
project(LdapProxy.Native C)
Copy link
Member

@jkotas jkotas Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nearly all shims under libraries/Native are proxies (trivial wrappers) like this one, but we do not call them proxies. Would System.DirectoryServices.Native be a better name for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to do that but it looked for me that Ldap related name shows what inside that library better than System.DirectoryServices.Native, but it is probably for me who did not work System.DirectoryServices. I will rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can't find a solution for it, System.DirectoryServices.Native is Linux only so it does not fail like this on Windows. I could delete windows's proxy and use __arglist in C# for Windows.
It means I need to deal with Linux failures first, there sometimes we can't find "ldap" library, example.
I am using find_library(LIBLDAP NAMES ldap) there, the name could be libldap-2.4.so.2 on Linux, libldap.dylib on OSX and it works in "default" configs but not in "SingleFile/AllSubsets_Mono" etc, am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @jkotas about naming this library System.DirectoryServicies.Native. It fits the model of the files we ship.

@sandreenko sandreenko closed this Apr 1, 2021
@sandreenko sandreenko reopened this Apr 1, 2021
jkotas
jkotas reviewed Apr 1, 2021
View reviewed changes
src/mono/wasm/Makefile Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Apr 1, 2021

cc @AaronRobinsonMSFT Maybe the complexity involved in implementing this shim will make us rather implement the vararg calling convention for Unix interop in some form.

@sandreenko sandreenko force-pushed the add_proxy_for_ber_scanf branch from 4e7f2d0 to 475aa96 Compare April 2, 2021 03:48
@danmoseley danmoseley requested review from joperezr and jkotas April 2, 2021 04:17
@sandreenko sandreenko closed this Apr 2, 2021
@sandreenko sandreenko reopened this Apr 2, 2021
@sandreenko
Copy link
Contributor Author

I have fixed "The following files are missing entries in the templated manifest" issues using #46342 as an example, no idea how System.IO.Ports.Native avoids these errors.

@sandreenko sandreenko requested a review from jkoritzinsky April 6, 2021 17:03
@jkoritzinsky
Copy link
Member

System.IO.Ports.Native doesn't ship with the shared framework, so the native assets don't need to be listed.

jkoritzinsky
jkoritzinsky reviewed Apr 6, 2021
View reviewed changes
src/libraries/Native/Unix/LdapProxy.Native/CMakeLists.txt Outdated
Comment on lines 20 to 27
find_library(LIBLDAP NAMES ldap)
if(LIBLDAP STREQUAL LIBLDAP-NOTFOUND)
find_library(LIBLDAP NAMES libldap.dylib)
endif()

if(LIBLDAP STREQUAL LIBLDAP-NOTFOUND)
find_library(LIBLDAP NAMES libldap-2.4.so.2)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
find_library(LIBLDAP NAMES ldap)
if(LIBLDAP STREQUAL LIBLDAP-NOTFOUND)
find_library(LIBLDAP NAMES libldap.dylib)
endif()
if(LIBLDAP STREQUAL LIBLDAP-NOTFOUND)
find_library(LIBLDAP NAMES libldap-2.4.so.2)
endif()
find_library(LIBLDAP NAMES ldap libldap.dylib libldap-2.4.so.2)

src/libraries/Native/Unix/LdapProxy.Native/CMakeLists.txt Outdated
@@ -0,0 +1,61 @@
project(LdapProxy.Native C)
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @jkotas about naming this library System.DirectoryServicies.Native. It fits the model of the files we ship.

@sandreenko
Copy link
Contributor Author

System.IO.Ports.Native doesn't ship with the shared framework, so the native assets don't need to be listed.

If I understand correctly we don't need LdapProxy.Native to be shipped with the shared framework, how is System.IO.Ports.Native excluded from it?

@sandreenko sandreenko marked this pull request as ready for review April 6, 2021 17:12
@jkoritzinsky
Copy link
Member

It's excluded here:

runtime/eng/liveBuilds.targets

Lines 157 to 158 in ee4d435

<ExcludeNativeLibrariesRuntimeFiles Condition="'$(IncludeOOBLibraries)' != 'true'"
Include="$(LibrariesNativeArtifactsPath)libSystem.IO.Ports.Native.*" />

@sandreenko sandreenko mentioned this pull request Apr 6, 2021
Merged
@sandreenko sandreenko force-pushed the add_proxy_for_ber_scanf branch 2 times, most recently from 9e38bf5 to d2a6890 Compare April 7, 2021 09:14
sandreenko
sandreenko commented Apr 7, 2021
View reviewed changes
Copy link
Contributor Author

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

squashed the commits for an easier review, rebased on top of the last infra changes.

JanV suggested that we would need Ldap library in our build docker images (now only in running images) so I opened dotnet/arcade#7197 to update them.

src/libraries/pkg/baseline/packageIndex.json
@@ -7861,7 +7861,8 @@
},
"ModulesToPackages": {
"libSystem.IO.Ports.Native": "runtime.native.System.IO.Ports",
"sni.dll": "runtime.native.System.Data.SqlClient.sni"
"sni.dll": "runtime.native.System.Data.SqlClient.sni",
"libSystem.DirectoryServices.Native": "runtime.native.System.DirectoryServices",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like it works for unix only, maybe we need:
"System.DirectoryServices.Nativ(.dll): "runtime.native.System.DirectoryServices",

src/libraries/pkg/runtime.native.System.DirectoryServices/netcoreapp.rids.props
@@ -0,0 +1,12 @@
<Project>
<ItemGroup Condition="'$(GeneratePackage)' == 'true'">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file was copied from System.IO.Ports.Native but probably should be updated for windows.

@runfoapp runfoapp bot mentioned this pull request Apr 7, 2021
Closed
@sandreenko sandreenko force-pushed the add_proxy_for_ber_scanf branch from d2a6890 to 13dc607 Compare April 7, 2021 18:43
@runfoapp runfoapp bot mentioned this pull request Apr 7, 2021
Closed
@sandreenko sandreenko force-pushed the add_proxy_for_ber_scanf branch from 13dc607 to 60750e3 Compare April 11, 2021 22:45
@runfoapp runfoapp bot mentioned this pull request Apr 12, 2021
Closed
@sandreenko sandreenko force-pushed the add_proxy_for_ber_scanf branch from d5b07c6 to be25066 Compare April 12, 2021 18:41
@sandreenko sandreenko mentioned this pull request Apr 14, 2021
Merged
@sandreenko
Copy link
Contributor Author

replaced by #51205

@sandreenko sandreenko closed this Apr 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky left review comments

@joperezr joperezr Awaiting requested review from joperezr

@jkotas jkotas Awaiting requested review from jkotas

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

[macOS-arm64] 55 System.DirectoryServices.Protocols.Tests fail on CoreCLR
5 participants
@sandreenko @jkotas @jkoritzinsky @jeffhandley @karelz