Skip to content

lib: cgen: make _bf_round_next_power_of_2 correct on 64-bit systems#494

Open
pzmarzly wants to merge 1 commit intofacebook:mainfrom
pzmarzly:push-myunsuktnoky
Open

lib: cgen: make _bf_round_next_power_of_2 correct on 64-bit systems#494
pzmarzly wants to merge 1 commit intofacebook:mainfrom
pzmarzly:push-myunsuktnoky

Conversation

@pzmarzly
Copy link
Copy Markdown
Contributor

@pzmarzly pzmarzly commented Mar 31, 2026

size_t can be 32-bit or 64-bit integer. We can use a conditional macro to support both.

This was raised by Claude PR reviewer in unrelated PR.

This function is currently only used to calculate _BF_LOG_MAP_SIZE (which is small static number), and img_size (which is being removed by #487), so there isn't any danger at the moment. But let's avoid a future surprise.


Also, on both 32-bit and 64-bit platforms, it gives incorrect result for 2**0. Should be 1, is 0. Fixing.

@pzmarzly pzmarzly requested a review from qdeslandes as a code owner March 31, 2026 13:56
@meta-cla meta-cla bot added the cla signed label Mar 31, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 31, 2026

Claude review of PR #494 (e205435)

Clean, minimal fix. The #if SIZE_MAX > 0xFFFFFFFFU guard is the correct portable way to conditionally include the 64-bit shift — SIZE_MAX is available via the existing <stdint.h> include, the comparison correctly distinguishes 32-bit vs 64-bit size_t, and column-0 preprocessor directive placement matches the project convention (e.g. src/libbpfilter/bpf.c:21). Commit message follows the component: subcomponent: description format.

Suggestions

  • Guard against value == 0src/libbpfilter/cgen/program.c:68 — Pre-existing edge case where _bf_round_next_power_of_2(0) returns 0 via unsigned wrap-around. Current callers are safe, but a guard would make the helper defensive for future use.

Workflow run

value |= value >> 4;
value |= value >> 8;
value |= value >> 16;
#if SIZE_MAX > 0xFFFFFFFFU
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude: suggestion: Pre-existing: _bf_round_next_power_of_2(0) returns 0 due to unsigned wrap-around (0 - 1 wraps to SIZE_MAX, OR-shifts fill all bits, then ++value wraps back to 0). Since you're already touching this function, consider adding a guard:

static inline size_t _bf_round_next_power_of_2(size_t value)
{
    if (value == 0)
        return 1;

    value--;
    ...

Neither current call site can pass 0 (sizeof(struct bf_log) * _BF_LOG_MAP_N_ENTRIES is always positive), so this is not blocking — just a defensive improvement for future callers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's true, and it is something I stumbled upon already when using this function for bf_hashset (where initial capacity was 0).

2**0 == 1, but here _bf_round_next_power_of_2(0) == 0.

I'll address this here in this PR as well, but I'll let reviewer decide if this should be merged together.

size_t can be 32-bit or 64-bit integer. And 2**0 should be 1.
@pzmarzly pzmarzly force-pushed the push-myunsuktnoky branch from d77ac8d to e205435 Compare March 31, 2026 15:00
Copy link
Copy Markdown
Contributor

@qdeslandes qdeslandes left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants