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

IDiaEnumSymbols::Next mismatch with docs #27

Closed
jdupak-ms opened this issue Jan 3, 2025 · 9 comments · Fixed by #29
Closed

IDiaEnumSymbols::Next mismatch with docs #27

jdupak-ms opened this issue Jan 3, 2025 · 9 comments · Fixed by #29

Comments

@jdupak-ms
Copy link

Hi, I think I have found mis-generated binding,

rgelt: *mut Option<IDiaSymbol>,

Rust says (out) pointer to optional (1x) and DIA (https://learn.microsoft.com/en-us/visualstudio/debugger/debug-interface-access/idiaenumsymbols-next?view=vs-2022) says pointer to array.

@riverar
Copy link
Collaborator

riverar commented Jan 7, 2025

I believe this is a documentation bug. IDiaEnumSymbols is an enumerator, and its backing array is not exposed to developers. Each invocation of IDiaEnumSymbols::Next provides a single IDiaSymbol through the rgelt parameter.

Closing for now, but feel free to continue the discussion.

@riverar riverar closed this as not planned Won't fix, can't repro, duplicate, stale Jan 7, 2025
@riverar
Copy link
Collaborator

riverar commented Jan 7, 2025

Oh, just realized this method accepts a fetch count. So this is indeed a bug.

@riverar riverar reopened this Jan 7, 2025
@kennykerr
Copy link
Collaborator

Possibly related: microsoft/windows-rs#3402 (comment)

@kennykerr
Copy link
Collaborator

kennykerr commented Jan 7, 2025

Note that the metadata does not indicate that this is an array:

Image

@riverar
Copy link
Collaborator

riverar commented Jan 7, 2025

Yep, something we can tweak in metadata generation (in this repository). Will try it out later today.

@tim-weis
Copy link

tim-weis commented Jan 9, 2025

That's a bug, but it isn't fatal in rendering the IDiaEnumSymbols API entirely unusable. It currently inhibits batch processing (in part), but there are ways to work around this:

  • The Item method gets properly translated, allowing for one-item-at-a-time processing:

    let symbols = session./* ... */;
    
    for i in 0..symbols.Count()? {
        println!("{}", symbols.Item(i as u32)?.name()?);
    }
  • The _NewEnum method is accurately expressed in the Rust bindings as well, which produces an IEnumVARIANT that does support batch retrieval. The number of hoops to jump through to go from a VARIANT to an IDiaSymbol is greater than zero, so I haven't personally verified this to work (it probably does, nonetheless).

I'm just leaving this here in case someone else is blocked on this bug.

@jdupak-ms
Copy link
Author

The next method does works for one element currently. But I am not sure how much is it accidental. I am using this abstractions:

pub struct IDiaSymbolIterator {
    symbols: IDiaEnumSymbols,
}

impl Iterator for IDiaSymbolIterator {
    type Item = IDiaSymbol;

    fn next(&mut self) -> Option<Self::Item> {
        unsafe {
            let mut result = ::core::mem::zeroed();
            let mut count = 0;
            self.symbols.Next(1, &mut result, &mut count).ok()?;

            if count == 0 || result.is_none() {
                None
            } else {
                result
            }
        }
    }
}

@tim-weis
Copy link

tim-weis commented Jan 9, 2025

Arrays in C are expressed as a pointer to the first element and a length. A pointer to a single object is identical to an array of length 1. The impl Iterator looks fine.

That aside, consider keeping unsafe blocks as narrow as possible. If you write let mut result = None; instead, you'll wind up with a single expression that needs to be wrapped inside an unsafe block.

@riverar
Copy link
Collaborator

riverar commented Jan 9, 2025

Am working on this method, but just a heads up--it's much easier to work with the Count/Item methods on the enumerator as such:

for i in 0..symbols.Count()? {
    println!("{}", symbols.Item(i as u32)?.name()?);
}

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 a pull request may close this issue.

4 participants