StackExchange.Redis icon indicating copy to clipboard operation
StackExchange.Redis copied to clipboard

UWP Native compiler error

Open soroshsabz opened this issue 3 years ago • 2 comments

I have a UWP project

When I want build this solution in Release Configuration I got a below error

1>C:\Users\ssoroosh\.nuget\packages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets(809,5): error : Internal compiler error: Object reference not set to an instance of an object.

if I remove StackExchange.Redis package dependency from my UWP project, this project build correctly

StackExchange.Redis version is 2.6.48

soroshsabz avatar Aug 12 '22 11:08 soroshsabz

related to #568

soroshsabz avatar Aug 12 '22 11:08 soroshsabz

After I some investigating problem, I found the problem is from version 2.2.3,

I suspect to Microsoft.Bcl.AsyncInterfaces, In 2.1.58 the dependencies list of the Redis is like below

.NETStandard 2.0 Microsoft.Bcl.AsyncInterfaces (>= 1.1.1) Pipelines.Sockets.Unofficial (>= 2.1.16) System.Diagnostics.PerformanceCounter (>= 4.7.0) System.Threading.Channels (>= 4.7.1)

and in 2.2.3 dependencies list of the Redis is like below

.NETStandard 2.0 Microsoft.Bcl.AsyncInterfaces (>= 5.0.0) Pipelines.Sockets.Unofficial (>= 2.2.0) System.Diagnostics.PerformanceCounter (>= 5.0.0) System.Threading.Channels (>= 5.0.0)

soroshsabz avatar Aug 12 '22 11:08 soroshsabz

It sounds like this is an internal compiler error the team may want to take a look at - @richlander any chance you know where Microsoft.Net.Native.Compiler lives - should this be a dotnet/runtime issue?

NickCraver avatar Aug 15 '22 22:08 NickCraver

Will ask for help for this to be routed.

richlander avatar Aug 16 '22 10:08 richlander

Hi @soroshsabz we are sorry to hear about the .NET native compilation error. Please follow the instructions on https://github.com/dotnet/core/blob/main/Documentation/ilcRepro.md to send us an ILC repro. Feel free to close this issue as we will follow-up with you over email. Hope to hear from you soon!

tommcdon avatar Aug 16 '22 14:08 tommcdon

@tommcdon as you says in ilcRepro.md I create ilRepro.zip and share it via link and send it to your email [email protected]

Did you think any security consideration to share this link here?

soroshsabz avatar Aug 17 '22 20:08 soroshsabz

@tommcdon as you says in ilcRepro.md I create ilRepro.zip and share it via link and send it to your email [email protected]

Thank you! I haven't received it yet -has it been sent?

Did you think any security consideration to share this link here?

My suggestion is to not share the link publicly as it will contain the IL (intermediate language) representation of the program as well as dependencies it uses to build. It may also contain some machine-specific environmental information such as the user profile name (e.g. C:\Users<name>) and nuget cache location.

tommcdon avatar Aug 18 '22 17:08 tommcdon

@soroshsabz I actually did receive the email. Taking a look at it now!

tommcdon avatar Aug 18 '22 17:08 tommcdon

@soroshsabz I've investigated the ilcrepro - the ILC compiler is crashing due to the following static variable assignment.

https://github.com/StackExchange/StackExchange.Redis/blob/7293213169a07fafe85716d1a91e4d0d34efb6d3/src/StackExchange.Redis/PhysicalBridge.cs#L298

ILC is in the "StaticInitData" phase of compilation and is parsing the IL in the static initializer StackExchange.Redis.PhysicalBridge.BridgeStatus. We are crashing when parsing the "ldloca.s" IL instruction

.method private hidebysig specialname rtspecialname static 
        void  .cctor() cil managed
{
  // Code size       27 (0x1b)
  .maxstack  2
  .locals (valuetype StackExchange.Redis.PhysicalBridge/BridgeStatus V_0)
  IL_0000:  ldloca.s   V_0
  IL_0002:  initobj    StackExchange.Redis.PhysicalBridge/BridgeStatus
  IL_0008:  ldloca.s   V_0
  IL_000a:  call       valuetype StackExchange.Redis.PhysicalConnection/ConnectionStatus StackExchange.Redis.PhysicalConnection/ConnectionStatus::get_Zero()
  IL_000f:  call       instance void modreq(System.Runtime.CompilerServices.IsExternalInit) StackExchange.Redis.PhysicalBridge/BridgeStatus::set_Connection(valuetype StackExchange.Redis.PhysicalConnection/ConnectionStatus)
  IL_0014:  ldloc.0
  IL_0015:  stsfld     valuetype StackExchange.Redis.PhysicalBridge/BridgeStatus StackExchange.Redis.PhysicalBridge/BridgeStatus::'<Zero>k__BackingField'
  IL_001a:  ret
} // end of method BridgeStatus::.cctor

Notice that BridgeStatus is a struct that contains an init field:

https://github.com/StackExchange/StackExchange.Redis/blob/75da236160c244f1ea0246f20d849519a6875acd/src/StackExchange.Redis/PhysicalBridge.cs#L293

I believe init fields are a C# 9 feature, while .NET native only supports C# 7.3. This syntax is actually not allowed for UWP C# or netstandard 2.0 projects and is blocked by VS 2022.

Would it be possible to try changing the initialization so that it uses C# 7.3 compliant syntax?

tommcdon avatar Aug 19 '22 03:08 tommcdon

@tommcdon Some context would help here - what are the plans for .NET native? I don't think it's realistic to constraint the library to language features < 4 years ago...we'll just constantly break UWP or be hindered by it. If there are plans to upgrade the support there over time, I'd say this is best solved in the README pointing people to the highest compatible version they're able to use and this issue for details on why.

Is the 7.3 limitation temporary and will go away as a limitation, or is that a hard stop? In either case, I'm not a fan or limiting the library to older versions of tooling/language as a solution - overall IMO it's akin to when we drop say .NET 4.0 support and the response is "if you're using .NET 4.0, this was the last version of the library that works".

NickCraver avatar Aug 19 '22 12:08 NickCraver

Hi @NickCraver! We are actively fixing bugs in .NET native, though since it is built on top of .NET Core 2.1 era corefx we are likely to stay with C# 7.3-level syntax.

tommcdon avatar Aug 19 '22 15:08 tommcdon

@tommcdon So I'm clear: the compiler will never support > C# 7.3? If that's the case, then I'd say:

  1. We should throw a more informative error than object not found if possible on the compiler side
  2. We can add a note to the README I guess that says the maximum version of SE.Redis you're able to use in this scenario

Overall, I'm not willing to restrict us to an old version of C# forever to support a single (and rare) use case - it's not a reasonable ask, IMO. It would be catering to the least common denominator and necessitate me rolling back a lot of newer language feature usage we already have (because the only way to ensure this in the future is to actually downgrade our language version).

I love what's happening with the native compilers, but I'd say at a more meta level: if the language version is capped indefinitely, the amount of current/supported libraries you can actually compile will continue to dwindle by nature of others upgrading and support being lost. I think to remain as or more useful going forward, it'll have to support newer language versions.

NickCraver avatar Aug 21 '22 02:08 NickCraver

We should throw a more informative error than object not found if possible on the compiler side

Great suggestion, I'll open a work item to track that

I love what's happening with the native compilers, but I'd say at a more meta level: if the language version is capped indefinitely,

I suggest checking out Native AOT - it is a fully modern open-source rewrite of .NET native and is cross-platform, and is shipping with .NET 7. It currently only supports console apps though this is just a "point in time" problem and I expect it to evolve to support other workloads. It's my understanding that it supports the latest C# version as well.

tommcdon avatar Aug 23 '22 15:08 tommcdon

@NickCraver I think this issue is bug not question, because latest version of this library is not working with UWP.

thanks :)

soroshsabz avatar Aug 25 '22 10:08 soroshsabz

@soroshsabz In this case there isn't a bug in our library, the question is "would we drop > C# 7.3 to support this use case?" - but the answer there is going to be no, it's not reasonable to constrain the library to tooling of 4+ years ago. We use the newer bits constantly and pervasively, so it's just not feasible. As the Native AOT story progresses, it sounds like you'll be able to use newer versions of this (and other) libraries, but there's nothing for us to do inside this repo.

If you open an issue anywhere on the compiler side asking for support on this, I'm happy to chime in and add a voice, but the issue does need to be improved there :)

NickCraver avatar Aug 25 '22 13:08 NickCraver

Closing this out to tidy up since any chances here would be on the compiler side :)

NickCraver avatar Sep 04 '22 01:09 NickCraver