Skip to content

Windows Thrdscan: Fix broken tuple unpacking #1807

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

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

dgmcdona
Copy link
Contributor

Timeliner fails due to an incorrect unpacking of this tuple, which needs
3 additional dictionary items for start path, win32 start path, and
win32 start address.

Timeliner fails due to an incorrect unpacking of this tuple, which needs
3 additional dictionary items for start path, win32 start path, and
win32 start address.
@ikelos
Copy link
Member

ikelos commented May 14, 2025

Could we please get a minimal test with the existing linux image as part of the PR to ensure this doesn't happen again please?

This is enough to ensure that the return code is nonzero and there was
some valid output.
@dgmcdona
Copy link
Contributor Author

I added a test locally, but it took ~226 seconds to run. I'm happy to push that if you're okay with adding that much time to test runs, but what if we also/instead updated _generator() to return a named tuple instead, and access the values by field name in generate_timeline()? That would make this kind of bug a little harder to trigger.

@ikelos
Copy link
Member

ikelos commented May 16, 2025

Errrr, yeah, I'm happy with that? I dunno if that should become a core coding style (in which case we should add it to the CODING_STYLE.md and explain the rationale so people know why they're doing it). It might also bulk out a bunch of plugins unnecessarily, so we might want to figure out the cheapest way of doing it (namedtuples/dataclasses/etc).

@atcuno
Copy link
Contributor

atcuno commented May 16, 2025

Tests taking another 4 minutes seem fine with me to avoid these issues.

Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

Ok, looks good, thanks 5:)

@ikelos ikelos merged commit 76db04c into develop Jun 11, 2025
26 checks passed
@ikelos ikelos deleted the bugfix/windows_thrdscan_tuple_unpacking branch June 11, 2025 13:06
@ikelos
Copy link
Member

ikelos commented Jun 11, 2025

We can still do the namedtuple change later if you think it'd be an improvement, but I'd like to keep API churn to a minimum so only if you think it'd be valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants