dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.66k stars 1.06k forks source link

Any CPU is treated as x86 even when it'll really be x64 #1545

Closed davidmatson closed 3 years ago

davidmatson commented 7 years ago

In the following project:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net47</TargetFramework>
  </PropertyGroup>
</Project>

And also I believe in this one:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net47</TargetFramework>
    <PlatformTarget>AnyCPU</PlatformTarget>
  </PropertyGroup>
</Project>

The implicit RuntimeIdentifier is currently, rather surprisingly win7-x86 rather than just win7. That is likely fundamentally broken for an AnyCPU platform target. The purpose of Any CPU is that it will run on any CPU, including as 64-bit on a 64-bit CPU (unless Prefer32Bit is set to true). If we treat it as x86, we'll be handling the other half of Any CPU cases incorrectly.

I can work up a repro if needed - I suspect when including a package with native references, we'd incorrectly pull in win7-x86 instead of win7 in this case, which will cause runtime breaks on an x64 OS.

Some additional details here: https://github.com/dotnet/project-system/issues/2744#issuecomment-325483773

livarcocc commented 6 years ago

@nguerrera I remember this discussion a while ago on why this was the best we could do in inference. Can you comment on this issue?

nguerrera commented 6 years ago

The first example will be emitted AnyCPU as long as there are no native dependencies pulled in from NuGet.

The second example will be emitted AnyCPU always.

nguerrera commented 6 years ago

Duplicate of #1560 (which is newer but has more discussion)

davidmatson commented 6 years ago

The problem here is that the project is compiled one way but package restored the other way. I understand there's a shared implementation characteristic manifesting in both of these ways, but I think this impact is the much more serious part.

Note that this bug does not occur if the project explicitly sets PlatformTarget=AnyCPU, as mentioned in #1560, only if the PlatformTarget is implicitly set to the default of AnyCPU.

For example, we have NuGet packages for native dependencies with targets files that operate differently for AnyCPU binaries vs. x86-only or x64-only binaries. When we package restore on AnyCPU, we binplace files like this: bin\x64\mynativemodule.dll bin\x86\mynativemodule.dll

For an x86-only consumer, we instead binplace like this: bin\mynativemodule.dll (x86 version)

And for x64-only consumers, we binplace like this: bin\mynativemodule.dll (x64 version)

At runtime, we check to see if the entry assembly is AnyCPU, and then look in the architecture-specific folder based on whether we're actually running as x64 or x86 this time. But if the entry assembly is not AnyCPU, we look for the file directly in bin, since there's only one runtime architecture possible.

The current way package restore is handled for native dependencies does not handle (implicit) Any CPU correctly - the assembly is still actually built as AnyCPU, but package restore assumes it will always run as x86 at runtime, and it won't.

nguerrera commented 6 years ago

Reopening to track the separate but related concerns between the two bugs. Whoever works on either bug should consider the other.

dianaqu commented 4 years ago

@nguerrera any update on this bug. I have been hitting this and haven't found a way to fix it. I have a package that include both dotnet and native dlls. the native dlls got pulled in for anyCPU is always x86 bits

nguerrera commented 4 years ago

@dianaqu I don't work on this project anymore, but I'm not aware of anything recent. cc @dsplaisted @marcpopMSFT

marcpopMSFT commented 4 years ago

@dianaqu if you have a package repro that you can share, @dsplaisted was interested in taking a look. The original issue listed here is below our current priority list.

Achilles1515 commented 3 years ago

@marcpopMSFT @dsplaisted Reproduction repo (.NET Framework, SDK-style): https://github.com/Achilles1515/RuntimeIdentifierInferenceBug

Clone, then build/run --> unable to load native dll.

This simple project is referencing the RE2.Managed library (v1.0.0 for the example) which contains native libraries in the following package paths:

runtimes\win-x64\native\RE2.Native.x64.dll runtimes\win-x86\native\RE2.Native.x86.dll

This .NET Framework, SDK-style project does not define PlatformTarget or RuntimeIdentifier properties so they are implicitly set according to: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

A build target in the csproj is also printing these properties to see their resulting values.

Build the project for AnyCPU and you will see:

PlatformTarget = x86
RuntimeIdentifier = win7-x86
Prefer32Bit = false (also not set in the csproj)

If these are to be the defaults for .NET Framework, then that's fine. But the problem then lies with the output executable, which is AnyCPU (64-bit preferred).

Following the win7-x86 RID, NuGet/package restore will put the 32-bit native binary, RE2.Native.x86.dll, in the build output. But when the AnyCPU executable is ran on a 64-bit OS, it will run as a 64-bit process and will be looking for the 64-bit native binary, RE2.Native.x64.dll (which it obviously won't find and will throw an exception).

The "easy" solution for this problem is to also set Prefer32Bit appropriately according to the implicit PlatformTarget being set. So in this above scenario with PlatformTarget = x86, Prefer32Bit would be set to true and the executable would be AnyCPU (32-bit preferred), which would run as a 32-bit process and successfully find RE2.Native.x86.dll.

dsplaisted commented 3 years ago

@Achilles1515 Thanks for the repro. I dug into this, and it turns out that one of the following needs to be true for the SDK to pick up the fact that there are native assets coming from a NuGet package:

This behavior was unintentional in the .NET Core 1.x and 2.x SDKs. We accidentally fixed it in the 3.0 SDK, but that ended up being a breaking change, so we added a workaround to match the previous behavior, even though it's not optimal: https://github.com/dotnet/sdk/pull/3543

To work around this, I'd recommend setting the RuntimeIdentifier or PlatformTarget explicitly. You can also add a reference to the Microsoft.NETCore.Platforms package:

<PackageReference Include="Microsoft.NETCore.Platforms" Version="5.0.0" />
Achilles1515 commented 3 years ago

I dug into this, and it turns out that one of the following needs to be true for the SDK to pick up the fact that there are native assets coming from a NuGet package:

  • The NuGet package uses the EXACT RuntimeIdentifier win7-x86 for its native assets (in the repro, the NuGet package uses win-x86 instead).

Interesting - yeah, I was wondering why it apparently was not understanding that x86 native assets were being pulled in. So what's the verdict here? It sounds like this is a bug that needs to be fixed. It's all the same whether it matches win7-x86 or win-x86 --> 32-bit native dependencies are being consumed and the output app needs to be compatible.

To work around this, I'd recommend setting the RuntimeIdentifier or PlatformTarget explicitly. You can also add a reference to the Microsoft.NETCore.Platforms package:

I understand that these workarounds exist, but they are not ideal because they all require intervention from the consumer. Should every NuGet package consumable by .NET Framework with runtimes\win-x86\native assets also place these assets in a runtimes\win7-x86\native package path to workaround this bug without requiring consumer intervention???

The package I was working with was using a .targets file and I added the following to fix the problem:

    <PropertyGroup Condition="'$(UsingMicrosoftNETSdk)' == 'true' And
                              '$(_UsingDefaultPlatformTarget)' == 'true' And
                              ($(RuntimeIdentifier.EndsWith('-x86')) or $(RuntimeIdentifier.Contains('-x86-'))) And
                              '$(Prefer32Bit)' == 'false'">
      <Prefer32Bit>true</Prefer32Bit>
      <Prefer32BitToFixDotnetBug>true</Prefer32BitToFixDotnetBug>
    </PropertyGroup>
    <Message Condition="'$(Prefer32BitToFixDotnetBug)' == 'true'" Importance="high" Text="Ariel7.Interop.targets: Build property `Prefer32Bit` set to `true` to mitigate https://github.com/dotnet/sdk/issues/1545"/>

...which is essentially doing the same thing as my PR to fix this problem. https://github.com/dotnet/sdk/pull/15497

dsplaisted commented 3 years ago

Unfortunately, there are projects that rely on the bug, so fixing the bug would mean breaking those apps. In cases like that we have to weigh the trade-off between fixing the bug or breaking apps.

Packages with native assets which will be consumed by .NET Framework projects could add a package dependency on the Microsoft.NETCore.Platforms package (for the .NET Framework TargetFramework) to work around this issue.

appel1 commented 3 years ago

Unfortunately, there are projects that rely on the bug, so fixing the bug would mean breaking those apps. In cases like that we have to weigh the trade-off between fixing the bug or breaking apps.

Packages with native assets which will be consumed by .NET Framework projects could add a package dependency on the Microsoft.NETCore.Platforms package (for the .NET Framework TargetFramework) to work around this issue.

Is there a similar workaround when you consume a package? We would like to use SDK style csproj to support targetting both net452 and net 5.0.

When targetting .NET Framework I end up with only the x86 native binaries from NuGet packages making it really hard to debug on a x64 machine, you need to manually replace the native binary after each build. Also makes deployment more difficult because we need to reach into the nuget package to find both x86 and x64 binaries to include in the final msi.

Even worse for AnyCPU .dll that are used as add-ins in programs that are available as both x86 and x64.

davidmatson commented 2 years ago

I just spend a bit of time diagnosing a failure, which was ultimately figure out why our DLL couldn't find the right file in the bin directory from our NuGet package. After debugging awhile, I realized it was this bug coming back yet again.

@dsplaisted - can you elaborate on how to add the workaround to ensure our package works correctly? Specifically, how does one add a dependency on the Microsoft.NETCore.Platforms package for the .NET Framework TargetFramework? We're targeting .NET 4.8, but I only see a list of .NET Core versions in the Versions tab here: https://www.nuget.org/packages/Microsoft.NETCore.Platforms/

Thanks!

dsplaisted commented 2 years ago

@davidmatson The package version does roughly correspond to the version of .NET (Core) it was released with, but the package can still be referenced from a .NET Framework project. So you can use version 5.0.0 as in the original workaround, or a later version should also work:

<PackageReference Include="Microsoft.NETCore.Platforms" Version="5.0.0" />
davidmatson commented 2 years ago

@dsplaisted - I tried adding this to my nuspec:

      <dependency id="Microsoft.NETCore.Platforms" version="5.0.0"/>

But it didn't help. To clarify, I'm a package author, and the package includes native contents for both x86 and x64. For x86 -only or x64-only projects, I only want the relevant binaries to be placed in the bin directory, but for AnyCPU projects, it needs to binplace both copies (one under an "x86" subdirectory and the other under an "x64" subdirectory), and at runtime it'll detect whether it's an AnyCPU assembly or not and look for the binary in the corresponding location (directly in the exe's directory for x86-only or x64-only; under the correct subdirectory name when AnyCPU).

Is there a workaround I can use as a package author to ensure my package works correctly for consumers that are x86-only, x64-only and AnyCPU? Thanks!

dsplaisted commented 2 years ago

@davidmatson In that case, I don't think the built-in functionality will help you. I think you'll probably need to include a .targets file in your NuGet package that deploys the right files.

@nkolev92 @zivkan Is there any guidance on how to author a NuGet package that supports .NET Framework and deploys an x86 or x64 native binary for projects with a specific CPU platform target, but deploys both binaries for AnyCPU projects?

zivkan commented 2 years ago

TL;DR version is no, there isn't any NuGet feature to automate this.

Long version, starting with package consumption with packages.config. NuGet doesn't know anything about runtime or native files. Therefore, it's entirely up to the package author to ship their own props/targets to do whatever they want, if they want to support their package consumers using packages.config. Many package authors instead just drop their native files under their package's content/ folder, which NuGet them copies into the project, regardless of the project's build architecture.

For projects consuming packages with PackageReference, NuGet has a feature, the runtimes/ folder. Note that https://github.com/dotnet/sdk/issues/9643 might be an issue 🤷‍♂️

In theory at least, I believe that putting both the x64 and x86 native files under runtimes/win/native/*.dll, while putting the x64 only assets under runtimes/win-x64/native/*.dll, and and the x86 native files under runtimes/win-x86/native/*.dll might work, as long as the project has access to the RID graph. This does mean that the native files need to be duplicated, once in the architecture specific RID and once again in the /win/ ("AnyCPU"?) RID. However, this entirely depends on how both the SDK style .NET SDK (this repo) and the non-SDK style SDK (.NET Framework Targeting Pack) define MSBuild properties before NuGet runs the restore. If AnyCPU builds define the MSBuild property that tells NuGet there's a specific RID because other parts of the build always want a default and can't handle it being empty, then NuGet can't tell the difference between an AnyCPU and RID specific project. I'm confident this is not a problem for SDK style projects (even if they target the .NET Framework), so I'd say only the non-SDK style projects have a chance of not playing nice with NuGet in this regard.

On the topic of the RID graph, if a package brings in the Microsoft.NETcore.Platforms package, with the .NET SDK override the package's RID graph, or will the package's RID graph win over the SDK's? Or will both be used? My concern has always been that the package's RID graph will win and therefore package authors using this package as a dependency will cause their package consumers problems when the package consumer is using a newer version of the .NET SDK than the package version they declared their dependency on.

Anyway, this is a complex issue that crosses many team boundaries of responsibility. Not that that's an excuse to giving customers (like package authors) a bad experience. What I'm trying to say is while I'm familiar with NuGet's codebase and features, the customer experience depends on how other parts of the build interact with NuGet, which I'm not an expert on. So I simply don't have enough knowledge to give a definitive or confident answer.

I have this tracking issue where I add links to all the issues I'm aware of related to packages that are managed wrappers of native code. Maybe looking through the comments in that issue, or reading through all the linked issues will help. Unfortunately this list of related questions and the upvotes on the issue have not yet been enough for management to prioritize an effort in documenting or improving the scenario: https://github.com/NuGet/docs.microsoft.com-nuget/issues/2070

davidmatson commented 2 years ago

@dsplaisted - could we reopen this issue to tracking providing package authors some way to write packages in this scenario? I understand we can't make the code at the top of this issue just work, unfortunately, due to back-compat concerns, but could we still track providing some kind of solution?

Thanks!

dsplaisted commented 2 years ago

@davidmatson It looks like https://github.com/NuGet/docs.microsoft.com-nuget/issues/2070 tracks this. Does that match what you were looking for?

zivkan commented 2 years ago

I think what the customer is reporting is that the .NET SDK is setting PlatformTarget to x86.

I'm having a chicken-egg problem trying to figure it out myself, but here the SDK is setting PlatformTarget, based on RuntimeIdentifier: https://github.com/dotnet/sdk/blob/458b5f7f628fcb1003806eeed3fd472543c20305/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L72-L105

but here it's setting RuntimeIdentifier based on PlatformTarget: https://github.com/dotnet/sdk/blob/458b5f7f628fcb1003806eeed3fd472543c20305/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L55-L62

All I know is that when I have an SDK style project targeting net48 and I do not specify either TargetPlatform or RuntimeIdentifier myself, when I take a binlog, I see that RuntimeIdentifier = win-x86 and TargetPlatform = x86.

How are customers writing their own targets (or NuGet itself) supposed to detect the "AnyCPU" scenario?

edit: actually, I see that GetDefaultPlatormTargetForNetFramework sets PlatformTarget to AnyCPU, but since this happens in a target, it means that when writing our own targets files, we have to ensure that the AdjustDefaultPlatformTargetForNetFrameworkExeWithNoNativeCopyLocalItems target runs first.

edit2: searching the binlog for AnyCPU, the following properties are evaluated with the value: Platform, PlatformName, Platforms. Therefore, I assume that customers can use this in their targets. Therefore I guess this is an education issue. Customers don't know about binlogs and the binlog viewer to do basic MSBuild debugging themselves. Maybe MSBuild also needs docs to explain to check the Platform property for AnyCPU, if it isn't already documented?

dsplaisted commented 2 years ago

@zivkan The logic you're looking at is complicated, and got more complicated over time. But basically if neither a RuntimeIdentifier nor a PlatformTarget is set for a .NET Framework project, then the SDK will guess that the RuntimeIdentifier is win7-x86 and see if it gets any native assets from NuGet packages. If it does, then it will set the PlatformTarget to x86. Otherwise, it will go back to AnyCPU.

I don't think that's the main thing @davidmatson is hitting though. It sounds like he wants guidance for how to create a NuGet package that correctly includes native assets, and in his case includes both x86 and x64 assets for AnyCPU, but otherwise includes only the matching assets.

It sounds like that only intersects with the original problem in this issue in that the guidance may need to include a recommendation to have packages with native assets in them depend on the Microsoft.NETCore.Platforms package.

davidmatson commented 2 years ago

@dsplaisted - it looks like the issue you linked is docs only. I'm looking for built-in functionality that allows binplacing native files in a way that honors AnyCPU (opting out of the behavior that can't be changed for back-compat). Let me know if that helps clarify.

Thanks!

zivkan commented 2 years ago

My previous response was long and rambly, sorry about that, but I think I covered it already. Your package should contain the following files:

+ ref
  + net48
    Managed.dll
+ runtimes
  + win
    + lib
      + net48
        Managed.dll
    + native
      native-x86.dll
      native-x64.dll
  + win-x86
    + lib
      + net48
        Managed.dll
    + native
      native-x86.dll
  + win-x64
    + lib
      + net48
        Managed.dll
    + native
      native-x64.dll

Managed.dll needs to do a runtime check if it's x86 or x64, and then PInvoke into the right dll.

I'm not 100% sure this will work, if it does, feedback would be great as maybe we can finally document it. But I also need someone to give feedback if this does not work, because I feel like what you're asking for is already possible (for package consumers using PackageReference. packages.config is not going to get any new major features).

davidmatson commented 2 years ago

@zivkan - I'm hoping there's a simpler solution with far less package size/number of files involved. What we have today is something like the following. .nuspec:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">
  <!-- ... -->
  <files>
    <file src="bin\Release\net48\OurDotNetNetComponent.dll" target="lib\net48\"/>
    <file src="OurPackageName.targets" target="build"/>
    <file src="bin\Release\some\native\path\x64\somenativebinary.exe" target="build\x64"/>
    <file src="bin\Release\some\native\path\x86\somenativebinary.exe" target="build\x86"/>
  </files>
</package>

.targets:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <None Include="$(MSBuildThisFileDirectory)\x64\**" Condition="'$(PlatformTarget)' == 'AnyCPU' or '$(PlatformTarget)' == ''">
      <Link>x64\%(RecursiveDir)%(Filename)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <None Include="$(MSBuildThisFileDirectory)\x86\**" Condition="'$(PlatformTarget)' == 'AnyCPU' or '$(PlatformTarget)' == ''">
      <Link>x86\%(RecursiveDir)%(Filename)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <None Include="$(MSBuildThisFileDirectory)\x64\**" Condition="'$(PlatformTarget)' == 'x64'">
      <Link>%(RecursiveDir)%(Filename)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <None Include="$(MSBuildThisFileDirectory)\x86\**" Condition="'$(PlatformTarget)' == 'x86'">
      <Link>%(RecursiveDir)%(Filename)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
  </ItemGroup>
  <Target Name="OurPackageNameValidatePlatformTarget">
    <Error Condition="!( '$(PlatformTarget)' == 'AnyCPU' or '$(PlatformTarget)' == '' or '$(PlatformTarget)' == 'x64' or '$(PlatformTarget)' == 'x86' )"
           Text="The OurPackageName package does not yet support $(PlatformTarget). Please contact the package author to request this support."/>
  </Target>
  <PropertyGroup>
    <BuildDependsOn>OurPackageNameValidatePlatformTarget;$(BuildDependsOn)</BuildDependsOn>
  </PropertyGroup>
</Project>

OurDotNetNetComponent:

        static void RunDependency()
        {
            ProcessStartInfo startInfo = new ProcessStartInfo
            {
                UseShellExecute = false,
                FileName = Path.Combine(GetExePath(), "somenativebinary.exe"),
            };

            Process process = Process.Start(startInfo);
            // ...
        }

        static string GetExePath()
        {
            string relativeExePath;
            bool? isCurrentProcessAnyCpu = IsCurrentProcessAnyCpu();

            if (isCurrentProcessAnyCpu == null)
            {
                throw new InvalidOperationException("No entry assembly is available.");

            }
            else if (!isCurrentProcessAnyCpu.Value)
            {
                relativeExePath = string.Empty;
            }
            else
            {
                relativeExePath = Environment.Is64BitProcess ? @"x64" : @"x86";
            }

            return Path.Combine(Path.GetDirectoryName(Assembly.GetEntryAssembly().Location), relativeExePath);
        }

        static bool? IsCurrentProcessAnyCpu()
        {
            Assembly entryAssembly = Assembly.GetEntryAssembly();

            if (entryAssembly == null)
            {
                return null;
            }

            entryAssembly.ManifestModule.GetPEKind(out PortableExecutableKinds peKind, out _);

            if ((peKind & PortableExecutableKinds.PE32Plus) != 0)
            {
                // x64-only assemblies are not Any CPU.
                return false;
            }
            else if ((peKind & PortableExecutableKinds.Required32Bit) != 0)
            {
                // x86-only assemblies are not Any CPU.
                return false;
            }
            else if ((peKind & PortableExecutableKinds.Preferred32Bit) != 0)
            {
                // If Prefer 32 bit is true, it must also be Any CPU.
                return true;
            }
            else
            {
                // Any CPU without Prefer 32-bit does not have any of the above flags set.
                return true;
            }
        }

This all works fine and ensures we only put each native binary once in the package, but the $(PlatformTarget)' == 'AnyCPU' doesn't work with PackageReference consumers using .NET Framework with the new-style csproj. Let me know if that all makes sense.