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

Bitops improvements #363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

brenns10
Copy link
Contributor

In the spirit of (incrementally) reducing the verbosity of drgn, I noticed that the bitops APIs could use a bit of improvement.

>>> b = prog["boot_cpu_data"]
>>> # x86_capability is an array of __u32 ...
>>> bitfield = cast("unsigned long *", b.x86_capability[0].address_of_())
>>> bits = list(for_each_set_bit(bitfield, 8 * sizeof(b.x86_capability)))

First, the documentation indicated that they take an unsigned long *, so I assumed I needed to cast my array to a pointer, which is a pretty verbose operation. But reading the code, I could see that it worked perfectly fine on arrays, and also on pointers/arrays to any unsigned integer type. (And later, I noticed even the tests use an arrray rather than pointer). So this PR updates that documentation and explicitly tests a wider range of integer types (just to be sure). That way future users would know they can provide the bitfield directly without the casting.

Second, it seems like sized array types are relatively common in the kernel for bit fields (e.g. cpuinfo_x86.x86_capability link), and for these, we can infer the size parameter. So this PR makes it optional to provide that parameter.

Now with the clearer documentation and optional parameter, it's a lot easier:

>>> bits = list(for_each_set_bit(prog["boot_cpu_data"].x86_capability))

Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

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

FWIW, this slightly less verbose version is equivalent to your cast:

cast("unsigned long *", prog["boot_cpu_data"].x86_capability)

But I like these improvements anyways, modulo my endianness concerns in the comments.

drgn/helpers/linux/bitops.py Outdated Show resolved Hide resolved
drgn/helpers/linux/bitops.py Outdated Show resolved Hide resolved
The docs currently state that the bitmap must be an "unsigned long *".
This is similar to the kernel API. It may not be immediately obvious
"unsigned long[]" arrays are acceptable as well, so clarify that.

Also, update the module documentation to reflect the fact that on
little-endian architectures, the helpers work just as well with any
other unsigned integer type, and update the tests to verify this.

Signed-off-by: Stephen Brennan <[email protected]>
The Linux API for for_each_set_bit() involves a size, but when dealing
with complete array types (a relatively common case), we can default to
the size of the array in bits. Add this ability to the helper.

Signed-off-by: Stephen Brennan <[email protected]>
@brenns10 brenns10 force-pushed the bitops_improvements branch from 606e6a3 to 40de87e Compare November 2, 2023 20:41
@brenns10 brenns10 requested a review from osandov November 3, 2023 00:25
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