Fix 'context_regs' not showing regs in order #989
Conversation
| @@ -7354,10 +7353,10 @@ def context_title(self, m: Optional[str]) -> None: | |||
|
|
|||
| def context_regs(self) -> None: | |||
| self.context_title("registers") | |||
| ignored_registers = set(self["ignore_registers"].split()) | |||
There was a problem hiding this comment.
this can still be a set
There was a problem hiding this comment.
What value is there in converting to a set from what split returns?
There was a problem hiding this comment.
x not in ignored_registers is faster if ignored_registers is a set.
There was a problem hiding this comment.
I assume that there is some overhead to turning it into a set in the first place as it will need to hash all the entries. Is the saving from then comparing against a set significant enough that it's worth doing so?
There was a problem hiding this comment.
depends on the number of ignored registers, which we don't know since it is controlled by the user.
There was a problem hiding this comment.
In order to compare against a set, the value being compared will either need to be directly compared by value (at which point the set is needless overhead) or the value will need to be hashed, compared by hash, and then compared by value if the hashes match (unless the hashes are guaranteed to not collide, which seems unlikely). I'm currently struggling to see a scenario where making this a set does anything but increase processing overhead.
There was a problem hiding this comment.
Because you use not in ignored_registers you not only need to compare (or hash) but you also need to iterate the whole list and compare EACH, unless you use a set.
If you [reg for reg in gef.arch.all_registers if reg not in ignored_registers] and all_registers is 20 elements, but ignored_registers is, say, 1 billion then you have to do 20x1billion/2 comparisons with a list, and 20x1 with a set. Sure, this is contrived, but it's still O(NxM) vs O(N)
| @@ -7354,10 +7353,10 @@ def context_title(self, m: Optional[str]) -> None: | |||
|
|
|||
| def context_regs(self) -> None: | |||
| self.context_title("registers") | |||
| ignored_registers = set(self["ignore_registers"].split()) | |||
There was a problem hiding this comment.
x not in ignored_registers is faster if ignored_registers is a set.
removed a `ignored_registers` which was left by mistake
Grazfather
changed the title
set to list for registers
gef.py
Outdated
| all_regs = gef.arch.all_registers | ||
| requested_regs = args.registers | ||
| regs = [reg for reg in requested_regs if x in all_regs] | ||
| invalid_regs = [reg for reg in requested_regs if x not in all_regs] |
There was a problem hiding this comment.
this can use set logic
There was a problem hiding this comment.
| invalid_regs = [reg for reg in requested_regs if x not in all_regs] | |
| invalid_regs = set(requested_regs) - set(all_regs) |
There was a problem hiding this comment.
Without evidence that the process of turning into a set and then doing set operations is noticeably more performant, it doesn't seem to be worth changing to a set just so we have a set. It doesn't feel like adds any clarity and I suspect it doesn't add any performance either.
There was a problem hiding this comment.
Let me be the one who decides that.
|
Please feel free to profile the differences - I suspect they are negligible. You can see how we profile in the context perf script https://github.com/hugsy/gef/blob/main/tests/perf/context_times.sh |
|
Closing this as it was fixed by #993 |
Description
Reverted from
setto list comprehension so ordering of requested registers is maintained.Ordering of registers is based on the order provided. The default will use
gef.arch.all_registerswhich is correctly ordered, and the order provided by the user if manually called.Changed so that if all requested registers are invalid it returns nothing rather than every register.
Patch required as previous change meant registers were placed into an unsorted
setobject leading to indeterminate ordering.Checklist
fixes #986