Page MenuHomeFreeBSD

Fix bitstring allocation on 32-bit platforms
ClosedPublic

Authored by asomers on Jun 14 2016, 11:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 7, 5:39 PM
Unknown Object (File)
Dec 22 2023, 9:45 PM
Unknown Object (File)
Nov 26 2023, 5:26 PM
Unknown Object (File)
Nov 23 2023, 4:00 PM
Unknown Object (File)
Nov 23 2023, 12:33 AM
Unknown Object (File)
Nov 22 2023, 6:23 PM
Unknown Object (File)
Nov 13 2023, 6:18 PM
Unknown Object (File)
Nov 13 2023, 5:29 AM
Subscribers

Details

Summary

sys/sys/bitstring.h
Fix a rounding calculation that could undersize a bitstring on
32-bit platforms.

include/bitstring.h
Add sys/param.h for the roundup2 macro

Test Plan

Existing ATF test cases

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

asomers retitled this revision from to Fix bitstring allocation on 32-bit platforms.
asomers updated this object.
asomers edited the test plan for this revision. (Show Details)
asomers added a reviewer: gibbs.
gibbs edited edge metadata.

I see how this can over allocate on platforms with long > 8bits, but I'm missing how 32bit long platforms are special and can get too little space allocated. Can you provide an example in the checkin comment?

This revision is now accepted and ready to land.Jun 14 2016, 11:35 PM
asomers edited edge metadata.

Restore bitstr_size to its originally intended purpose of calculating the
actual number of bytes used by a bitstring, and fix bit_decl to round up
appropriately.

This revision now requires review to proceed.Jun 15 2016, 5:23 PM
asomers edited edge metadata.

The important thing about division is that you divide.

If I want to dynamically allocate a bitstr, I use bitstr_size() to tell me how much to allocate. Without the roundup, I'll potentially allocate too little memory.

sys/sys/bitstring.h
109 ↗(On Diff #17771)

#define bitstr_size(_nbits) (roundup2(_nbits, _BITSTR_BITS) / 8)

128 ↗(On Diff #17771)

No roundup is required here. Is it clearer to use howmany()? I'm not sure.

Should we inline roundup2? Prior to this change, at least in user space, bitstring.h only relied on functionality provided in the C standard. It might be nice to keep that.

In D6848#145456, @gibbs wrote:

If I want to dynamically allocate a bitstr, I use bitstr_size() to tell me how much to allocate. Without the roundup, I'll potentially allocate too little memory.

I would say that you should use bit_alloc. But plenty of callers are doing it themselves. I'll do as you suggest, which will basically revert the 2nd diff of this review.

sys/sys/bitstring.h
128 ↗(On Diff #17771)

Roundup most certainly is required here, unless the roundup is implemented in bitstr_size instead. howmany() is basically just division rounding up.

sys/sys/bitstring.h
128 ↗(On Diff #17771)

That's what I meant. If bitstr_size() has to roundup anyway, there's no need to roundup here too.

Move the roundup back into bitstr_size instead of bit_decl

Are you happy with this @gibbs? Due to my vacation, I must commit today or it won't get into 11.0.

gibbs edited edge metadata.

I'm just not sure about the new dependency on sys/param.h. I leave that for you to decide.

This revision is now accepted and ready to land.Jun 24 2016, 2:55 PM
asomers edited edge metadata.

Remove sys/param.h include.

This revision now requires review to proceed.Jun 24 2016, 4:03 PM
gibbs requested changes to this revision.Jun 24 2016, 4:48 PM
gibbs edited edge metadata.
gibbs added inline comments.
sys/sys/bitstring.h
107–110 ↗(On Diff #17861)

We don't want to prevent a future declaration of roundup2 in the user's program either. So something like this?

#ifdef roundup2
#define        _bit_roundup2 roundup2
#else
#define        _bit_roundup2(x, y)        (((x)+((y)-1))&(~((y)-1))) /* if y is powers of two */
#endif

Move to private section and update call sites to use the private macro name.

This revision now requires changes to proceed.Jun 24 2016, 4:48 PM
asomers edited edge metadata.

Don't define a publicly visible "roundup2"

gibbs edited edge metadata.

Please fix the extra whitespace and test both with and without a roundup2 defined prior to including the header before landing.

sys/sys/bitstring.h
78 ↗(On Diff #17862)

Extra newline.

This revision is now accepted and ready to land.Jun 24 2016, 5:06 PM
This revision was automatically updated to reflect the committed changes.