This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Move dlopen and dlsym to PAL #25134
Closed
Closed
Move dlopen and dlsym to PAL #25134
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3cbf2ff
Move dlopen and dlsym to PAL
qmfrederik 434e354
PR feedback
qmfrederik 06778ff
PR feedback
qmfrederik 306441a
P/Invoke dlopen directly on netcoreapp2.0 and use the PAL on netcorea…
qmfrederik 2aafcc4
Fix buildconfigurations to include netcoreapp for vertical build
safern File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
15 changes: 15 additions & 0 deletions
15
src/Common/src/Interop/Unix/System.Native/Interop.DlOpen.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class Sys | ||
| { | ||
| [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_DlOpen")] | ||
| internal static extern IntPtr DlOpen(string fileName, DlOpenFlags flag); | ||
| } | ||
| } |
15 changes: 15 additions & 0 deletions
15
src/Common/src/Interop/Unix/System.Native/Interop.DlOpen.netcoreapp20.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class Sys | ||
| { | ||
| [DllImport(Libraries.Libdl, EntryPoint = "dlopen")] | ||
| internal static extern IntPtr DlOpen(string fileName, DlOpenFlags flag); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
15 changes: 15 additions & 0 deletions
15
src/Common/src/Interop/Unix/System.Native/Interop.DlSym.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class Sys | ||
| { | ||
| [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_DlSym")] | ||
| internal static extern IntPtr DlSym(IntPtr handle, string symbol); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #include <dlfcn.h> | ||
| #include "pal_dl.h" | ||
|
|
||
| // Validate that our definitions match those used by the OS. | ||
| static_assert(PAL_RTLD_LAZY == RTLD_LAZY, ""); | ||
| static_assert(PAL_RTLD_NOW == RTLD_NOW, ""); | ||
|
|
||
| extern "C" void* SystemNative_DlOpen(const char *file, int mode) | ||
| { | ||
| return dlopen(file, mode); | ||
| } | ||
|
|
||
| extern "C" void* SystemNative_DlSym(void *handle, const char *name) | ||
| { | ||
| return dlsym(handle, name); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #pragma once | ||
|
|
||
| // Values for the mode flag in dlopen | ||
| enum | ||
| { | ||
| PAL_RTLD_LAZY = 1, | ||
| PAL_RTLD_NOW = 2 | ||
| }; | ||
|
|
||
| /** | ||
| * Loads a dynamic library file. Implemented as shim to dlopen(3). | ||
| * | ||
| * An opaque handle for the dynamic library, or null on failure. | ||
| */ | ||
| extern "C" void* SystemNative_DlOpen(const char *file, int mode); | ||
|
|
||
| /** | ||
| * Gets, for a given dynamic library, the address of a symbol in that library. Implemented as shim to dlopen(3). | ||
| * | ||
| * The address of the symbol, or null on failure. | ||
| */ | ||
| extern "C" void* SystemNative_DlSym(void *handle, const char *name); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <BuildConfigurations> | ||
| <PackageConfigurations> | ||
| netcoreapp2.0-Windows_NT; | ||
| netcoreapp2.0-Unix; | ||
| netfx; | ||
| netstandard; | ||
| </PackageConfigurations> | ||
| <BuildConfigurations> | ||
| $(PackageConfigurations); | ||
| netcoreapp-Windows_NT; | ||
| netcoreapp-Unix; | ||
| </BuildConfigurations> | ||
| </PropertyGroup> | ||
| </Project> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is turning System.Drawing.Common into yet another partial OOB, with unmanaged dependency. Partial OOBs like that have very high maintenance costs. If we want to do this, it should be approved by the .NET Core disro owners.
cc @Petermarcu @karelz
The right way to solve the problem with PInvoking to DlOpen is to add the imperative dll import APIs to .NET Core 2.1: https://github.com/dotnet/coreclr/issues/14968. It will avoid the need for the dependency on .NE T Core shims, and putting System.Drawing.Common inbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas, won't it be a partial OOB either way? A build for 2.0 on top of standard, and then a build for 2.1 using netcoreapp-specific APIs. How does a dependency on a System.Native.so/dylib function cause any more issues than a dependency on netcoreapp-specific method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is whether it is part of .NET Runtime download or whether the app carries it with itself.
Everything that depends on System.Native.so/dylib shim should be part of .NET Runtime download. The System.Native.so/dylib shim is not part of our public API surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think dotnet/coreclr#14968 will solve the problem this PR is trying to address.
Yes, there are a lot of cases where managed code wants to have a lot of control over which library is being loaded. Managed libraries which wrap around native libraries, for example, will know where their native library is located, what name(s) it may have, and will try to load that directly.
For example, I think it's normal System.Drawing.Common has a very strong opinion about where libgdiplus.so is located, because it has domain knowledge about libgdiplus. The same for, say, FFmpeg.AutoGen, which is a managed wrapper around ffmpeg.
On the other hand, System.Drawing.Common doesn't care at all about dlopen and has no domain knowledge about it. The way I understand it, dotnet/coreclr#14968 would allow System.Drawing.Common to say something along the lines of "dlopen may be in libdl.so on most Linux distro's, libdl.so.2 on CentOS and something else on FreeBSD".
I don't think it's the business of System.Drawing.Common to care about that.
Anyone writing a managed wrapper around a native library trying and using function pointers will need dlopen and dlsym. Everyone doing that will need to replicate the same search logic.
And most importantly, that logic will eventually fail: when corefx is ported to a new platform, when libdl versions,... .
Long story short, for this use case, I believe a better option would be to make dlopen/dlsym in one shape or another part of the CoreFX API, as suggested in #17135.
"The least that could possible work" may be to expose a public
LoadNativeLibraryandLoadNativeSymbolAPI in CoreFX on which System.Drawing.Common and others can depend.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I did not know that #17135 existed. The APIs proposed by #17135 are the kind of APIs that I thought we will add as part of dotnet/coreclr#14968.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nuget package declares dependency on netcore2.1, it is expected to work on netcoreapp2.2 or even netcoreapp3.0 as well. How do we make sure that it works there when it depends on netcoreapp2.1 shims?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By taking such a dependency we're signing up to keep those functions exposed, as part of the internal surface area of the product, little different than signing up to continue keeping public surface area exposed in subsequent versions.
Like I said earlier, I'm fine if we decide against that and want to impose the restrictions you highlight. I'm simply calling out that I don't think it's the only avenue available to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
The difference is that we have a lot of infrastructure to maintain compatibility of the public surface. Such infrastructure does not exist for the internal surface. It would need to be built.
We have tried to play similar internal surface games in .NET Framework in the past (e.g. with
System.SizedReference). It tends to turn into poorly designed, tested and documented part that everybody is afraid to touch. I would love to keep things simple and stick to public surface when it comes to dependencies between independent packages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I agree with @jkotas that we should avoid taking these type of dependencies in independent shipping packages. While the rule likely isn't written down anywhere we do try to follow the same strategy for anything the depends on System.Private.CoreLib directly as well.