-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
Add count option in heap chunks command to limit the number of chunks to process / output. #1029
Conversation
|
||
if not summary and chunk.base_address == arena.top: | ||
if not ctx.summary and chunk.base_address == arena.top: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value of None
would cause this to throw an Exception
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the code to fix this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK looks good, but technically you're passing a mutable object (the context) as the default value, which is bound to the function object. If it changes (and you take care not to) then subsequent calls would have this modified default. https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument
Since you don't modify it, maybe it's fine, but we run the risk of someone modifying it in the future. Probably OK though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it will never be null. The default value is added there just to follow existing code. We can also remove the default value, but I will have to move the parameter before the ones with default value. not sure if it is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed the default value and lifted the ctx parameter to the front.
|
||
if not summary and chunk.base_address == arena.top: | ||
if not ctx.summary and chunk.base_address == arena.top: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK looks good, but technically you're passing a mutable object (the context) as the default value, which is bound to the function object. If it changes (and you take care not to) then subsequent calls would have this modified default. https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument
Since you don't modify it, maybe it's fine, but we run the risk of someone modifying it in the future. Probably OK though.
LGTM after Graz's comments |
gef.py
Outdated
@@ -6374,18 +6374,18 @@ def do_invoke(self, _: List[str], **kwargs: Any) -> None: | |||
ctx = GlibcHeapWalkContext(min_size=args.min_size, max_size=args.max_size, count=args.count, summary=args.summary) | |||
if args.all or not args.arena_address: | |||
for arena in gef.heap.arenas: | |||
self.dump_chunks_arena(arena, print_arena=args.all, allow_unaligned=args.allow_unaligned, ctx=ctx) | |||
self.dump_chunks_arena(arena, ctx, print_arena=args.all, allow_unaligned=args.allow_unaligned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use the context idiom, I wonder whether we should put it as the first arg (which is what we do in Go). Do you know if it's also done this way in python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i was hesitated on this one too - which name is better: context or options? eventually I chose context, because the remaining chunk count changes over time... options usually won't change.
for the order of parameters, i don't really have strong opinion on that. what i usually do is to put parameters that absolutely required but need to be part of the workflow in the front (in this case - arena), then followed by the context (which contains mostly everything).
If you like the code to be modified, i can definitely do that by moving print_arena and allow_unaligned in - so it will be dump_chunks_arena(arena, ctx)
, which is what i usually do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have updated the code now. let's see how it looks.
Last merge also caused a conflict, you'll need to rebase. |
6825389
to
afabab9
Compare
afabab9
to
3be1c00
Compare
yep! it is conflicting with the symbol change. i have rebased the branch now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with the current state of the PR. @Grazfather any last comment?
Thanks for your PR and patience @r12f , it's all good now 🎉 |
Woot! thanks a lot for the review @Grazfather and @hugsy ! And happy new year! 🎉 |
Happy new year to you too 😁 🎉 |
Description
This change add
--count
option in heap chunks command to limit the number of chunks to process / output.Currently, since we don't have the count limit, even with the size filter, we might get too many chunks in the output, especially when debugging large dumps.
With this change, we can use
--count
option to give us only a few samples to check what might be happening, e.g. large buffers with certain special size.Here is the screenshot that demos the usage (mixed with
--min-size
)L