Skip to content

Conversation

jan-auer
Copy link
Member

This is an attempt to provide a better API for module references in symbol data. My initial findings were described in #89 (comment):

  • Code for resolving a module reference can be seen here.
  • This subtracts 1 via imodForXimod to get a 0-based index. I'm assuming a literal 0 means no reference.
  • openModByImod then runs pmodiForImod.
  • pmodiForImod indexes into rgpmodi.
  • rgpmodi is built sequentially here.

So I think it should be safe to assume that you can take dbi.modules().nth(proc_ref.module - 1).

In this implementation, I'm taking the safe route and parsing this into an Option<usize> instead of erroring, but I'm not sure if that's even necessary. Most likely, a value of 0 is invalid an a reference symbol, so we could also error out instead of making this an Option.

Can someone validate my assumptions?

@jan-auer jan-auer requested a review from willglynn October 30, 2020 13:06
@jan-auer jan-auer self-assigned this Oct 30, 2020
@jan-auer
Copy link
Member Author

@willglynn since there are breaking changes lined up the next release on master, what do you think about this PR?

@jan-auer
Copy link
Member Author

jan-auer commented May 20, 2022

I'll start looking into this again ahead of another "major" release.

@Swatinem @gabrielesvelto @mstange -- do you have thoughts on this change, since this is being used in some of your projects.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm. though I’m looking at this PR in isolation, so I have no idea what this change means in the grand scheme of things.

@jan-auer jan-auer force-pushed the fix/symbol-modi branch 2 times, most recently from e9b3119 to 8bb6889 Compare May 20, 2022 11:34
@mstange
Copy link
Collaborator

mstange commented May 21, 2022

I checked pdb-addr2line to see what it's using module indexes for, and found the following instances:

  • It uses DBISectionContribution::module and stores it as a u16.
  • It looks up this module index without subtracting 1 in a Vec of pdb::Modules which it collected from pdb.debug_information()?.modules().
  • The only other time it deals with module indexes is when resolving cross-module ItemIndex references, but those use a case-insensitive comparison of the module name and not a module index.

(I'm actually a bit surprised by these findings - I distinctly recall that I had a bug because I forgot to subtract 1 in some place, but maybe it was in code which I ended up removing.) Edit: I found the thing I remembered, it was a section index, not a module index. sections.get((section_index - 1) as usize)

So it seems that pdb-addr2line is not making use of any of the module_index properties that this PR modifies. Maybe it should be using some of those to get better results? For example, ProcedureReferenceSymbol seems like something that pdb-addr2line might be interested in.

I think this PR looks good. The only suggestion I have is that maybe DBISectionContribution::module should also be changed to a usize for consistency - it's currently a u16 which doesn't make it clear whether you need to subtract 1 or not.

@gabrielesvelto
Copy link

I don't think we have other dependencies on this API. dump_syms uses the pdb crate but never uses this particular field.

@jan-auer jan-auer merged commit ad39f23 into master Jun 2, 2022
@jan-auer jan-auer deleted the fix/symbol-modi branch June 2, 2022 08:04
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.

4 participants