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

24 bit integer support, forms strx3, addrx3 #553

Merged
merged 6 commits into from
Apr 19, 2024
Merged

Conversation

sevaa
Copy link
Contributor

@sevaa sevaa commented Apr 18, 2024

Forms strx3, addrx3 were both TODOs.

I don't have a binary with either of those forms, but I have a crash report along the lines of KeyError: DW_FORM_strx3, so it exists in the wild.

There are currently no forms that correspond to signed 24 bit ints.

This functionality, I believe, was the original reason for the attempted migration to construct 2.10.


While at it, I've noticed some gaps in the the enums. I thought we concluded that we don't need a sample binary for every single constant. The LLVM vendor extensions for LNCT come from https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html .

@sevaa
Copy link
Contributor Author

sevaa commented Apr 19, 2024

Removed the global declarations. There might be a teeny tiny hit on performance.

@sevaa
Copy link
Contributor Author

sevaa commented Apr 19, 2024

Speaking of possible performance enhancements. We have a bunch of statements here and there along the lines of:

x = struct_parse(structs.Dwarf_uleb128(''), stream)

where there is an unnecessary overhead of constructing a new instance of the stateless ULEB128 parser. We could wring some performance out of constructing one early on (maybe even globally :) ) and reusing it throughout. What do you think, and if so, what should we call it?

@eliben
Copy link
Owner

eliben commented Apr 19, 2024

Speaking of possible performance enhancements. We have a bunch of statements here and there along the lines of:

x = struct_parse(structs.Dwarf_uleb128(''), stream)

where there is an unnecessary overhead of constructing a new instance of the stateless ULEB128 parser. We could wring some performance out of constructing one early on (maybe even globally :) ) and reusing it throughout. What do you think, and if so, what should we call it?

We should prove it actually matters for performance first :)

@eliben eliben merged commit 7336640 into eliben:main Apr 19, 2024
4 checks passed
@sevaa
Copy link
Contributor Author

sevaa commented Apr 19, 2024

We should prove it actually matters for performance first

UPD: it does. Replacing construction with instance reuse in just one place - the line where we read the DIE abbrev code - reduced the firehose test time from 4 sec to 3.5. It's on the hottest path, that's why it was in my sights.

@sevaa sevaa deleted the feat_int24 branch April 19, 2024 14:02
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.

2 participants