| Message ID | 2e3e3a7145a5851fcf5c485b38d14344c9b824d7.1679342296.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5abc4c1c892c1040c38522f490ca81187e7262d6 |
| Headers | show |
| Series | pack-bitmap: miscellaneous mmap read hardening | expand |
On Mon, Mar 20, 2023 at 04:02:46PM -0400, Taylor Blau wrote: > Both `read_be32()` and `read_u8()` are defined as inline dating back > to b5007211b6 (pack-bitmap: do not use gcc packed attribute, > 2014-11-27), though that commit does not hint at why the functions were > defined with that attribute. I think any non-header inline like this can implicitly be assumed to be "I thought it would make things faster". ;) > However (at least with GCC 12.2.0, at the time of writing), the > resulting pack-bitmap.o contains the same instructions with or without > the inline attribute applied to these functions: > > $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.before} > [ apply this patch ] > $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.after} > $ objdump -d pack-bitmap.o.before >before > $ objdump -d pack-bitmap.o.after >after > $ diff -u pack-bitmap.o.{before,after} > --- before 2023-03-15 18:54:17.021580095 -0400 > +++ after 2023-03-15 18:54:21.853552218 -0400 > @@ -1,5 +1,5 @@ > > -pack-bitmap.o.before: file format elf64-x86-64 > +pack-bitmap.o.after: file format elf64-x86-64 > > Disassembly of section .text: > > So defining these functions as inline is at best a noop, and at worst > confuses the reader into thinking that there is some trickier reason > that they are defined as inline when there isn't. Nice digging. The "inline" is really just a hint to the compiler here, and obviously it does not need that hint. I do wonder if that is still true after you make them more complicated in a later patch in the series. On the other hand, I doubt that these need to be very optimized at all. If there were a tight loop of single-byte reads, the function overhead might be noticeable. But generally we're reading only a few items from the beginning of each entry, and then reading the bulk of the data via ewah_read_mmap(). So I think the overall argument is "let the compiler decide what is good to inline and what is not". -Peff
diff --git a/pack-bitmap.c b/pack-bitmap.c index 794aaf5b02..1d12f90ff9 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -244,14 +244,14 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index, return stored; } -static inline uint32_t read_be32(struct bitmap_index *bitmap_git) +static uint32_t read_be32(struct bitmap_index *bitmap_git) { uint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos); bitmap_git->map_pos += sizeof(result); return result; } -static inline uint8_t read_u8(struct bitmap_index *bitmap_git) +static uint8_t read_u8(struct bitmap_index *bitmap_git) { return bitmap_git->map[bitmap_git->map_pos++]; }
Both `read_be32()` and `read_u8()` are defined as inline dating back to b5007211b6 (pack-bitmap: do not use gcc packed attribute, 2014-11-27), though that commit does not hint at why the functions were defined with that attribute. However (at least with GCC 12.2.0, at the time of writing), the resulting pack-bitmap.o contains the same instructions with or without the inline attribute applied to these functions: $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.before} [ apply this patch ] $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.after} $ objdump -d pack-bitmap.o.before >before $ objdump -d pack-bitmap.o.after >after $ diff -u pack-bitmap.o.{before,after} --- before 2023-03-15 18:54:17.021580095 -0400 +++ after 2023-03-15 18:54:21.853552218 -0400 @@ -1,5 +1,5 @@ -pack-bitmap.o.before: file format elf64-x86-64 +pack-bitmap.o.after: file format elf64-x86-64 Disassembly of section .text: So defining these functions as inline is at best a noop, and at worst confuses the reader into thinking that there is some trickier reason that they are defined as inline when there isn't. Since this pair of functions does not need to be inlined, let's drop that attribute from both `read_u8()` and `read_be32()`. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)