Syslinux - Feb 2018 - Mismatch between code comments and reality in malloc.h

Hi Devs,

I've been doing some deep code diving and I seem to have found a discrepancy
between code comments and reality. This difference could hint at a bug or
performance degradation, but I doubt it. I thought to ask to get clarification.

In syslinux/core/mem/malloc.h, the comments and definition for arena_header,
ARENA_PADDING and free_arena_header are:


/*
 * This structure should be a power of two.  This becomes the
 * alignment unit.
 */
struct arena_header {
    malloc_tag_t tag;
    size_t attrs; /* Bits 0..1:  Type
        2..3:  Heap,
4..31: MSB of the size  */
    struct free_arena_header *next, *prev;

#ifdef DEBUG_MALLOC
    unsigned long _pad[3];
    unsigned int magic;
#endif
};

/* Pad to 2*sizeof(struct arena_header) */
#define ARENA_PADDING ((2 * sizeof(struct arena_header)) - \
       (sizeof(struct arena_header) + \
sizeof(struct free_arena_header *) + \
sizeof(struct free_arena_header *)))

/*
 * This structure should be no more than twice the size of the
 * previous structure.
 */
struct free_arena_header {
    struct arena_header a;
    struct free_arena_header *next_free, *prev_free;
    size_t _pad[ARENA_PADDING];
};

In my environment, the size of int and pointer are both 32-bits (x86 32-bits).
When compiled with DEBUG_MALLOC undefined; I get

sizeof(arena_header) == 16
sizeof(free_arena_header) == 56

Which disagrees with the comments. The comments do make a lot of sense to ensure
good memory block alignment. To make the code agree with the comments (the
easier option); the definition of ARENA_PADDING needs to be altered to:

#define ARENA_PADDING (((2 * sizeof(struct arena_header)) - \
       (sizeof(struct arena_header) + \
sizeof(struct free_arena_header *) + \
sizeof(struct free_arena_header *))) / sizeof(size_t))

This includes a reduction in the size of the padding by the unit size of the
padding, i.e sizeof(size_t). Given that this is the memory sub-system, this
small change could have a significant impact.

What are your thoughts?

Feedback most welcome,
Brett

On 02/03/18 04:28, Brett Walker via Syslinux wrote:
> Hi Devs,
> 
> I've been doing some deep code diving and I seem to have found a
discrepancy between code comments and reality. This difference could hint at a
bug or performance degradation, but I doubt it. I thought to ask to get
clarification.
> 
> In syslinux/core/mem/malloc.h, the comments and definition for
arena_header, ARENA_PADDING and free_arena_header are:
> 
> 
> /*
>  * This structure should be a power of two.  This becomes the
>  * alignment unit.
>  */
> struct arena_header {
>     malloc_tag_t tag;
>     size_t attrs; /* Bits 0..1:  Type
>         2..3:  Heap,
> 4..31: MSB of the size  */
>     struct free_arena_header *next, *prev;
> 
> #ifdef DEBUG_MALLOC
>     unsigned long _pad[3];
>     unsigned int magic;
> #endif
> };
> 
> /* Pad to 2*sizeof(struct arena_header) */
> #define ARENA_PADDING ((2 * sizeof(struct arena_header)) - \
>        (sizeof(struct arena_header) + \
> sizeof(struct free_arena_header *) + \
> sizeof(struct free_arena_header *)))
> 
> /*
>  * This structure should be no more than twice the size of the
>  * previous structure.
>  */
> struct free_arena_header {
>     struct arena_header a;
>     struct free_arena_header *next_free, *prev_free;
>     size_t _pad[ARENA_PADDING];
> };
> 
> In my environment, the size of int and pointer are both 32-bits (x86
32-bits). When compiled with DEBUG_MALLOC undefined; I get
> 
> sizeof(arena_header) == 16
> sizeof(free_arena_header) == 56
> 
> Which disagrees with the comments. The comments do make a lot of sense to
ensure good memory block alignment. To make the code agree with the comments
(the easier option); the definition of ARENA_PADDING needs to be altered to:
> 
> #define ARENA_PADDING (((2 * sizeof(struct arena_header)) - \
>        (sizeof(struct arena_header) + \
> sizeof(struct free_arena_header *) + \
> sizeof(struct free_arena_header *))) / sizeof(size_t))
> 
> This includes a reduction in the size of the padding by the unit size of
the padding, i.e sizeof(size_t). Given that this is the memory sub-system, this
small change could have a significant impact.
> 
Right, either ARENA_PADDING needs to be changed as you say, or _pad
needs to be defined as char (or uint8_t) instead of size_t.  The latter
might be better.

	-hpa