KirillOsenkov / MSBuildStructuredLog

A logger for MSBuild that records a structured representation of executed targets, tasks, property and item values.
MIT License
1.47k stars 198 forks source link

Unsafe accessors in Reflector don't work #834

Open KirillOsenkov opened 2 weeks ago

KirillOsenkov commented 2 weeks ago

People are reporting that on net8.0 Reflector doesn't work, fails to access fields.

KirillOsenkov commented 2 weeks ago

I temporarily disabled the net8.0 branch here: https://github.com/KirillOsenkov/MSBuildStructuredLog/commit/1a821951b21cc51035fe2bf6126ba1e9143f5f0f

Unfortunately I don't have time to investigate now because I need to unblock users.

@filipnavara if you perhaps have time to investigate this at some point. But it's not urgent, I mitigated it for now.

filipnavara commented 2 weeks ago

I'll put it on my backlog. Not sure when I will have time to have a look.

It worked in the local builds of the MSBuild Log Viewer, both in NativeAOT and CoreCLR, on macOS and Windows. I'd probably need more info or test case to see what is broken.

KirillOsenkov commented 2 weeks ago

there's absolutely no rush. I've asked for a repro.

brettfo commented 2 weeks ago

@KirillOsenkov @filipnavara I have a repro, but it's a bit involved. I tried to create a console app that shows the same behavior, but I couldn't, however this repro survives git clean -dxf on my box.

  1. Install .NET SDK 9.0rc1. I think I was able to repro it with rc2 as well, but definitely with rc1.
  2. Clone this repo at commit 92663fc995e8e0192a1f4ed5e911bf4e3c226556.
  3. Clone https://github.com/dependabot/dependabot-core at this branch.
    1. There are necessary submodules, so you'll need to run git submodule update --init --recursive
  4. From the dependabot-core repo, edit the file nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/NuGetUpdater.Core.csproj and remove the <PackageReference> for MSBuild.StructuredLogger and instead add a <ProjectReference> to src/StructuredLogger/StructuredLogger.csproj in this repo.
  5. From the dependabot-core repo open nuget/helpers/lib/NuGetUpdater/NuGetUpdater.sln in VS
    1. You may need to add StructuredLogger.csproj to the solution.
  6. In VS, open the file StructuredLogger -> Reflector.cs and set a breakpoint on line 55.
  7. Open the file NuGetUpdater.Core.Test -> Discover/DiscoveryWorkerTests.Project.cs and debug the first unit test.
  8. When you hit the breakpoint in Reflector.cs step and see the exception about the missing field message, even though the debugger shows it.
filipnavara commented 2 weeks ago

Thanks. The detailed instructions are much appreciated. I will report back soon.

KirillOsenkov commented 2 weeks ago

may need to checkout the binlog viewer repo to https://github.com/KirillOsenkov/MSBuildStructuredLog/commit/1fd03860348b4cc6084fc5cfde2ccbe6cff31a89 to revert my change where I disabled the 8.0 ifdef (https://github.com/KirillOsenkov/MSBuildStructuredLog/commit/1a821951b21cc51035fe2bf6126ba1e9143f5f0f)

filipnavara commented 2 weeks ago

I can reproduce it locally. The gist is that the UnsafeAccessor fails when the field is volatile:

using System;
using System.Runtime.CompilerServices;

var a = new A();
Console.WriteLine(GetSetFoo(a));

[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "foo")]
extern static ref string GetSetFoo(A args);

class A { private volatile string? foo = "Bar"; }

I'll check with the runtime team if this is expected behavior or bug.