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

Changes for ClrMD 2.1 #999

Merged
merged 11 commits into from
May 14, 2022
Merged

Changes for ClrMD 2.1 #999

merged 11 commits into from
May 14, 2022

Conversation

leculver
Copy link
Contributor

@leculver leculver commented May 12, 2022

ClrMD 2.1 initial changes

Please read docs/Migrating21.md.

Mark PEImage and Elf* as internal

ClrMD is getting out of the business of file format reading. :)

While it's useful to have a bunch of file reading abilities hanging around, there are simply better libraries for reading PE, Elf, and Mach-O images.

The intent here is still read images enough to perform ClrMD's operations, but we will:

  1. Drop our dependency on System.Reflection.Runtime (unfortunately it doesn't react well when there's missing chunks of PE files and it's causing problems with Single File executables).
  2. Mark all file readers as internal and slim them down to just what ClrMD needs.
  3. Provide a new way to open Stream objects for files loaded into memory. These Streams will be formatted as if they were files on disk (not mapped into memory), meaning you can pass these files off to libraries like System.Reflection.Metadata or Marklio.Metadata and get fully featured file format readers without relying on ClrMD's buggy implementation.

In effect, you may have to pick up a new dependency, but the end result will be better than where we are today.

Rework Symbol Server API implementation

ClrMD's symbol server code had several issues which are now fixed:

  1. We could not locate non-pe images correctly. (Meaning OS X and Linux diagnostics were busted unless you were on a machine with the installed runtime).
  2. We over-requested file locations that will never be filled. E.G. requesting file.ptr will not be filled on most symbol servers.
  3. We (should) now properly handle symsrv* symbol paths, as well as fully implementing the sympath syntax.

Rework ClrInfo creation

While this is mostly an internal change, greatly clean up how we locate CLR runtimes in the process.

Rework ModuleInfo

ModuleInfo is now an abstract base class with implementations for Elf, Mach-O and PE images implementing various functionality. This really cleaned up an ugly wart in creating ModuleInfo objects while eliminating some unneeded interfaces. Additionally we provide a way to get resources and exports from ModuleInfo now.

Rework DAC library info

We no longer provide just a single ClrInfo.DacLibrary but instead enumerate all known debugging libraries for the given runtime with ClrInfo.DebuggingLibraries. This will provide all dac and dbi libraries and index properties.

@leculver leculver marked this pull request as draft May 12, 2022 22:00
leculver added 11 commits May 12, 2022 15:24
ClrMD is getting out of the business of file format reading. :)

While it's useful to have a bunch of file reading abilities hanging around, there are simply better libraries for reading PE, Elf, and Mach-O images.

The intent here is still read images enough to perform ClrMD's operations, but we will:

Drop our dependency on System.Reflection.Runtime (unfortunately it doesn't react well when there's missing chunks of PE files and it's causing problems with Single File executables).
Mark all file readers as internal and slim them down to just what ClrMD needs.
Provide a new way to open Stream objects for files loaded into memory. These Streams will be formatted as if they were files on disk (not mapped into memory), meaning you can pass these files off to libraries like System.Reflection.Metadata or Marklio.Metadata and get fully featured file format readers without relying on ClrMD's buggy implementation.
In effect, you may have to pick up a new dependency, but the end result will be better than where we are today.

This is only a small piece of this overhaul.
- Make ModuleInfo a lot less strange.  Now it's an abstract base class defining a lot of properties, but defers most of its operations to specific implementations.
- Removed VersionInfo.
- Removed IExportReader, this is now defined on ModuleInfo.
- Handle single-file exe.
- Now consolidate all information about dacs and dbis into ClrInfo.cs and expose them via ClrInfo.DebuggingLibraries.
@leculver leculver marked this pull request as ready for review May 14, 2022 20:31
@leculver leculver changed the title Mark PEImage internal Changes for ClrMD 2.1 May 14, 2022
@leculver leculver merged commit bdbeeb7 into microsoft:master May 14, 2022
@leculver leculver deleted the peImageInternal branch May 14, 2022 20:40
@mikem8361
Copy link
Member

Marking ElfFile internal is really going to break SOS! I use it to lookup exports (ElfFile.TryGetExportSymbol). The symstore ELF reader doesn't have the symbol lookup support or otherwise, I'd use that.

@leculver
Copy link
Contributor Author

Don't pick up the change just yet. I was also going to send you an email first thing monday. =)

Let me dig through the SOS code on Monday and see what needs to be there. I didn't realize you took a direct dependency on it, so if this disrupts things too much I'm happy to either add it back or modify SOS to work better with the new system. I'm not trying to put more work on you here with these changes. =)

If you are only using this for export symbols, note that we actually now expose symbol reading directly through ModuleInfo.GetSymbolAddress in https://github.com/microsoft/clrmd/blob/master/src/Microsoft.Diagnostics.Runtime/src/Common/ModuleInfo.cs#L57. There's also a similar function for reading resources of modules to get things like the CLRDEBUGGINGINFO resource (though I'm not sure I'm happy with the design of that resource function).

@leculver
Copy link
Contributor Author

Also note I also plan to make creating ModuleInfo from a stream or base address possible through ClrMD, so even if you don't have the right ModuleInfo laying around it will be possible to construct one. This is not currently in the main codebase yet, mostly because the changeset was getting too big and I like to check in with smaller, more manageable PRs.

SOS is unique in that you are connected to my main branch builds, so I would expect the firehose of changes to be broken for SOS for a couple of days, but the end goal here is to get to a place where these commonly needed functions are exposed to you through more sensible APIs (like ModuleInfo) instead of having a direct line to the implementation detail (like ElfFile).

Fundamentally, I plan to rewrite a lot of the elf reading code in ClrMD next to slim it down and optimize it for when we are missing data in a dump. Also, PEImage and Elf* will go request their underlying files from the symbol store when it fails a read. These are very difficult to do with the current implementation. So even if we end up exposing Elf* as public again, I'll still need to make some SOS tweaks when I change the surface area of those implementations.

Let's sync up on Monday to make sure I'm headed in the right direction.

@mikem8361
Copy link
Member

The direct DARC dependency allows for more immediate fixes in the diagnostics repo from clrmd, but this is the downside. Technically there should be a 2.0 branch/build/DARC subscription that we could subscribe, but that is some work.

I did find GetSymbolAddress() The new ModuleInfo is better. Using System.Version is better. I haven't looked at the ClrInfo changes yet, but it shouldn't take me that long to do them. I'd rather be the one to make them.

@leculver
Copy link
Contributor Author

Sounds good, let me know if you run into trouble or have questions.

@leculver
Copy link
Contributor Author

Also, the new ClrInfo.DebuggingLibraries now contains the DBI archive info. Instead of relying on the properties of the old DacInfo or other module properties, you can simply find DBIs enumerated there for !clrstack's ICorDebug implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants