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

Improve Leaf Record Errors #360

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Sep 30, 2022

No description provided.

Copy link
Owner

@Washi1337 Washi1337 left a comment

Choose a reason for hiding this comment

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

I like the idea, but I think if we want to capture the unknown / unsupported type records, then we should be consistent with this and use it everywhere. E.g., SerializedArrayTypeRecord could benefit from the same type of construction.

However, simply replacing code like this with a call to the proposed GetLeafRecord method loses contextual information if an error occurs. E.g., there is a difference between a message such as SerializedArrayTypeRecord contains an incorect leaf type at index 01234. and Array type 5678 contains an invalid element type index 1234.. The second one is more specific, since it also indicates which property failed to be parsed, rather than simply stating that some index within the leaf was invalid. Furthermore, the first does not include the type index of the faulty leaf itself either.

Can we somehow rephrase / refactor this such that we do not lose this contextual information?

src/AsmResolver.Symbols.Pdb/Records/CodeViewSymbol.cs Outdated Show resolved Hide resolved
@Washi1337 Washi1337 added this to the 5.0.0 milestone Oct 5, 2022
@Washi1337 Washi1337 added enhancement pdb Issues related to AsmResolver.Symbols.Pdb labels Oct 5, 2022
@ds5678
Copy link
Contributor Author

ds5678 commented Oct 13, 2022

How do you want to handle the SerializedArrayTypeRecord situation?

@Washi1337
Copy link
Owner

@ds5678 The array type record was just an example of how a leaf could reference multiple type indices, and thus it would be unclear in the error message of the proposed change which type index was invalid.

Some simple idea would be to add a string parameter to the GetLeafRecord method, indicating which type index was attempted to be looked up, so it could be used in the error string format. I am open for other ideas as well though.

@Washi1337 Washi1337 removed this from the 5.0.0 milestone Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pdb Issues related to AsmResolver.Symbols.Pdb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants