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.

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

Fix Crossgen2 PDB generator #70407

Merged
merged 2 commits into from
Jun 9, 2022
Merged

Fix Crossgen2 PDB generator #70407

merged 2 commits into from
Jun 9, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Jun 7, 2022

During my perf investigation work I have found out that PDB emitter
in Crossgen2 is broken. I tracked this down to the change

fdf6485#diff-24e48862e3b82f52e7fa04f22700b1c976a012bfeb08a246406f4e5ec579699b

that caused two behavioral changes in the PDB emitter:

  1. The logic around QueryPDBNameEx got refactored to use a char[]
    instead of a StringBuilder and that silently caused _pdbFilePath
    to be set to the string "System.Char[]" instead of the actual path
    (cf PdbWriter.cs#221 in the quoted commit).

  2. For some reason I don't yet fully understand, the COM wrapper
    refactoring ends up bumping the refcount on the _ngenWriter by one
    so that it never gets actually closed and properly flushed.

In this PR I have worked around that by manually calling Release
twice but I suspect one of the Releases should be moved somewhere
else. In the debugger I see that, after returning from
CreateNGenPdbWriter the pdbWriterInst has the correct refcount = 1.
Then something happens in comWrapper.GetOrCreateObjectForComInstance
the debugger traces over and ends up in
ILCompilerComWrappers.CreateObject but by that time the refcount
has already been bumped up to 2. I'm not a COM expert so I defer
to Andy to comment on what is the right way to proceed here.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

P.S. An interesting corollary of this discovery is that our lab PDB validation
is next to none (the above commit is 5 months old).

P.P.S. If I'm right to assume that the changes in the quoted commit
basically constituted preparatory changes for building Crossgen2 using
NativeAOT, I wonder whether we need to test both IL and NativeAOT
version of Crossgen2 to make sure COM object tracking works fine
in both runtimes. As a side question, do we have any perf numbers
measuring the Crossgen2 switchover to NativeAOT? Do we still have the
Crossgen2 compilation perf lab that used to be maintained by
@DrewScoggins?

During my perf investigation work I have found out that PDB emitter
in Crossgen2 is broken. I tracked this down to the change

dotnet@fdf6485#diff-24e48862e3b82f52e7fa04f22700b1c976a012bfeb08a246406f4e5ec579699b

that caused two behavioral changes in the PDB emitter:

1) The logic around QueryPDBNameEx got refactored to use a char[]
instead of a StringBuilder and that silently caused _pdbFilePath
to be set to the string "System.Char[]" instead of the actual path
(cf PdbWriter.cs#221 in the quoted commit).

2) For some reason I don't yet fully understand, the COM wrapper
refactoring ends up bumping the refcount on the _ngenWriter by one
so that it never gets actually closed and properly flushed.

In this PR I have worked around that by manually calling Release
twice but I suspect one of the Releases should be moved somewhere
else. In the debugger I see that, after returning from
CreateNGenPdbWriter the pdbWriterInst has the correct refcount = 1.
Then something happens in comWrapper.GetOrCreateObjectForComInstance
the debugger traces over and ends up in
ILCompilerComWrappers.CreateObject but by that time the refcount
has already been bumped up to 2. I'm not the COM expert to I defer
to Andy to comment on what is the right way to proceed here.

Thanks

Tomas
@trylek
Copy link
Member Author

trylek commented Jun 7, 2022

/azp run runtime-coreclr crossgen2 outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

P.S. An interesting corollary of this discovery is that our lab PDB validation is next to none (the above commit is 5 months old).

Ran into that one in the past too: #13134 :)

I wonder whether we need to test both IL and NativeAOT version of Crossgen2 to make sure COM object tracking works fine in both runtimes

The COM object tracking should be the same in both - the preparatory change that @agocke did got rid of dependency on built-in COM, in favor of doing things "by hand". The same code runs on both JIT and AOT-hosted crossgen2. It's not COM from the runtime's perspective anymore, just a bunch of p/invokes and there's no complex runtime behaviors around it anymore.

That said, I still have #63571 in my backlog to compile crossgen2 with NativeAOT before we test is so that we run our crossgen testing with the same config that we ship it with.

As a side question, do we have any perf numbers measuring the Crossgen2 switchover to NativeAOT?

We have numbers to the tune of "crossgen2 compiled with ReadyToRun compiling CoreLib" and "crossgen2 compiled with NativeAOT compiling CoreLib" in the Preview 3 blog post: https://devblogs.microsoft.com/dotnet/announcing-dotnet-7-preview-3/.

@agocke
Copy link
Member

agocke commented Jun 8, 2022

@AaronRobinsonMSFT for the refcount question

@trylek
Copy link
Member Author

trylek commented Jun 8, 2022

/azp run runtime-coreclr crossgen2

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek trylek closed this Jun 8, 2022
@trylek trylek reopened this Jun 8, 2022
@trylek
Copy link
Member Author

trylek commented Jun 8, 2022

/azp run runtime-coreclr crossgen2

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek trylek merged commit a325e52 into dotnet:main Jun 9, 2022
@trylek trylek deleted the Crossgen2PDBFix branch June 9, 2022 18:06
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants