Skip to content

Conversation

@DanielT
Copy link
Author

DanielT commented Nov 18, 2024

There are also two pull requests for getsentry/pdb which I intentionally did not include in this bundle:

@vvuk
Copy link

vvuk commented Nov 18, 2024

For getsentry#154 -- the PDB files in question come from dotnet built with crossgen2, which is a dotnet tool that pre-JITs assemblies for faster startup. These PDB files contain debug info for that pre-JITted code, and are needed for (in the particular use case) samply to identify any useful info about stacks that contain that code.

The PDB files seem to be very minimal -- they're generated by crossgen2 directly, and as such don't contain the same info that pdb files generated by cl or similar contain. The solution in that PR was the simplest that got the desired results when working with these files. I don't think it would ever really be hit in any other use case, because typically the section headers will be present otherwise.

Copy link
Owner

@afranchuk afranchuk left a comment

Choose a reason for hiding this comment

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

There are a few minor issues/questions I'd like addressed, otherwise it looks good!

@DanielT
Copy link
Author

DanielT commented Nov 21, 2024

Thanks for the review!

As noted in the description, this is a bundle of changes by other people from the pdb issue tracker.
I have some changes of my own to support various section types used by LLVM for Rust code that I will create pull request for later, but I started on the basis of all of these changes. Once we've reached a consensus about this, I'll rebase my code and send you that too.

As a result I can't really say anything about the original intention of each author.
From my perspective your review comments look like good improvements.

How would you prefer that I address them - should I amend the original commits, or add a "fixes" commit to the pull request?

@afranchuk
Copy link
Owner

I would normally say to amend the commits, but as they are pulled from elsewhere a fixes commit would be more appropriate. Most of these are minor, the important ones are probably #2 (comment) and #2 (comment).

@CouleeApps
Copy link

If you're taking my one PR you may want the various other changes I made since then. See https://github.com/Vector35/pdb-rs/commits/master/

Is there a consensus on which version of this repo is maintained? The v35 one is not super active and the getsentry one has been effectively untouched for 2+ years. Would love to have somewhere to contribute changes back to, but I don't think I have the time to maintain it myself through my work.

@afranchuk
Copy link
Owner

afranchuk commented Nov 21, 2024

If you're taking my one PR you may want the various other changes I made since then. See https://github.com/Vector35/pdb-rs/commits/master/

Is there a consensus on which version of this repo is maintained? The v35 one is not super active and the getsentry one has been effectively untouched for 2+ years. Would love to have somewhere to contribute changes back to, but I don't think I have the time to maintain it myself through my work.

This one is published to crates.io as pdb2 since I could not get the pdb owners to respond to me. For the time being, I can maintain it. However, if they choose to in the future, they may take the changes here back in.

@DanielT
Copy link
Author

DanielT commented Nov 21, 2024

@michal-kapala and @CouleeApps - should I remove your changes from this PR?
If you want to submit your changes yourself, then that would be ideal from my point of view, but I'll proceed if you don't have time.

@CouleeApps
Copy link

If you want to submit your changes yourself, then that would be ideal from my point of view, but I'll proceed if you don't have time.

You're welcome to include any changes I've made (they are under the MIT/Apache2 license after all) but I don't forsee getting much time at work to maintain this in the near future.

@michal-kapala
Copy link

michal-kapala commented Nov 21, 2024

If you want to submit your changes yourself, then that would be ideal from my point of view, but I'll proceed if you don't have time.

I'd appreciate a merge with the typo fixes, my PR was self-contained and stable, just an occasional one from me

@DanielT
Copy link
Author

DanielT commented Nov 21, 2024

If you want to submit your changes yourself, then that would be ideal from my point of view, but I'll proceed if you don't have time.

You're welcome to include any changes I've made (they are under the MIT/Apache2 license after all) but I don't forsee getting much time at work to maintain this in the near future.

That's not necessarily an argument against merging your changes. Otherwise, by that logic, we should discard most of the pdb code, since the original authors at sentry.io are no longer maintaining it.
Meanwhile it seems I've been duplicating work you already did: I see you added support for S_FRAMEPROC, which I also did a few days ago. That shows that it would be much better to have one "definitive" crate with all of the features.

@michal-kapala
Copy link

@DanielT here's the pdb file i based my pr on way back if you want to experiment with my changes, comes from this repo

QuazalWV.zip

@DanielT
Copy link
Author

DanielT commented Nov 22, 2024

As far as I can tell I've addressed the open points with the updated PR

- The From<u8> implementation for SourceLanguage used the wrong constants for Rust and Go.
- characteristics of the SectionSymbol are of type SectionCharacteristics instead of u32.
- The comment for CoffGroupSymbol has been fixed.
- CoffGroupSymbol was defined incorrectly: the segment is alreday part of the
  PdbInternalSectionOffset and should not appear separetely.
- A unit test for S_COFFGROUP / CoffGroupSymbol has been added, to verify the handling of the segment
- VirtualTableShapeType now contains a Vec of VirtualTableShapeDescriptor, instead of u8.
  An internal conversion function from u8 to VirtualTableShapeDescriptor was added
@DanielT DanielT force-pushed the fixes_from_pdb_issue_tracker branch from 1b3a65a to ebecb2b Compare November 22, 2024 20:53
@DanielT
Copy link
Author

DanielT commented Dec 1, 2024

Hi, do you need anything else from me?
It's been a week, and I want to be sure you're not waiting for input from me.

@afranchuk afranchuk merged commit 32c1e8e into afranchuk:master Dec 2, 2024
@DanielT DanielT deleted the fixes_from_pdb_issue_tracker branch December 2, 2024 18:58
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.