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

Support emitting PortablePDB #486

Closed
wants to merge 7 commits into from
Closed

Conversation

Martin1994
Copy link

Summary

Support debugging symbol with .NET Core (PortablePDB)

Reference

https://github.com/dotnet/runtime/blob/main/docs/design/specs/PortablePdb-Metadata.md

High level design

  • Moved common logic with PdbWriter to an abstract class.
  • Added a new PortablePdbWriter
  • Added a new field to ikvm's MethodBase to track the MethodDef table row in the emitted PE image.

Demo

ikvm-ppdb preview

Note: variable name and source path requires -g compile flag sent to javac

Next steps

  • Need help to build IKVM.Java with debugging symbol, and preferably with SymbolLink

@CLAassistant
Copy link

CLAassistant commented Feb 6, 2024

CLA assistant check
All committers have signed the CLA.

@wasabii
Copy link
Contributor

wasabii commented Feb 6, 2024

Hmmmmm.

#478

@Martin1994
Copy link
Author

Hmmmmm.

#478

hmmmmmmm.........

@wasabii
Copy link
Contributor

wasabii commented Feb 6, 2024

Alright, I had some time this morning to look this over. As I sent last night, we already have a PR moving forward on PortablePdbs, that takes a little bit of a different path.

So, first, I love this work. I can't stress this enough. Thank you so much. But there's going to be some problems with accepting this.

First, it's against the main branch. We do active development on the next major version against the develop branch and minor releases (hotfixes, etc) against the main branch. The develop branch has deviated quite a bit at this point. For instance, the develop branch IKVM.Reflection has already been changed to use SRME to build PEs. Which means elimination of a number of things you changed in this PR: there's no more TextSection, ModuleWriter is largely rewritten, etc. Had this work been some quick additions to the develop branch, I probably could have accepted it ahead of the other changes in PR 478.

That said, PR 478 moves forward on a bit of a different track: we are deprecating the native PDB writer, and the MDB writer. Just removing them completely. Mono supports Portable PDBs. Framework (>4.7.2) supports Portable PDBs. So there's not really any reason to keep the existing support. So, there's not much of a need for a new abstraction model over symbol writers.

So, while I appreciate this effort, I just don't see how I can accept it.

We do most of the discussions and coordination about work and planning and stuff in our Discord channel. I think you would benefit quite a bit by joining and participating. We would love to have you.


// header:
// LocalSignature
writer.WriteCompressedInteger(0); // Omit for now?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed? My implementation actually included a StandAloneSig.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I am not sure, but i don't see problems with vsdbg or netcoredbg.

@wasabii
Copy link
Contributor

wasabii commented Feb 22, 2024

Closing in favor of #478

@wasabii wasabii closed this Feb 22, 2024
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.

3 participants