Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
To: Willy Tarreau <w@1wt.eu>
Cc: "Paul E. McKenney" <paulmck@kernel.org>,
	Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>,
	Nugraha <richiisei@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	GNU/Weeb Mailing List <gwml@vger.gnuweeb.org>
Subject: Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`
Date: Sun, 20 Mar 2022 23:36:55 +0700	[thread overview]
Message-ID: <c7129520-5e9a-f9d1-cc12-5af9456c917f@gnuweeb.org> (raw)
In-Reply-To: <20220320161644.GF8067@1wt.eu>

On 3/20/22 11:16 PM, Willy Tarreau wrote:
> Ammar,
> 
> a few points below:
> 
> On Sun, Mar 20, 2022 at 04:37:49PM +0700, Ammar Faizi wrote:
>> +struct nolibc_heap {
>> +	size_t	len;
>> +	char	user_p[] __attribute__((__aligned__));
>> +};
> 
> Note that many programs assume that malloc() returns a field aligned
> to 2*sizeof(pointer) and unless I'm mistaken, above the user pointer
> will only be aligned by one pointer. This may have an impact when the
> compiler decides to use SIMD instructions.

Section 7.20.3 of C99 states this about `malloc()`:
"""
   The pointer returned if the allocation succeeds is suitably aligned
   so that it may be assigned to a pointer to any type of object.
"""

And this is what GCC doc says about __attribute__((__aligned__)):
"""
   The aligned attribute specifies a minimum alignment for the variable
   or structure field, measured in bytes. When specified, alignment must
   be an integer constant power of 2. Specifying no alignment argument
   implies the maximum alignment for the target, which is often, but by
   no means always, 8 or 16 bytes.
"""

Link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

Simple experiment on Linux x86-64...

```
ammarfaizi2@integral2:/tmp$ cat > test.c
#include <stdio.h>
int main(void)
{
	printf("alignof = %zu\n", __alignof__(long double));
	return 0;
}
ammarfaizi2@integral2:/tmp$ gcc -o test test.c
ammarfaizi2@integral2:/tmp$ ./test
alignof = 16
ammarfaizi2@integral2:/tmp$
```

We have `long double` which requires 16 byte alignment. So
__attribute__((__aligned__)) should cover this. And yes, it's true that
it's 2*sizeof(void*), but importantly for the above reason.

>> +#ifndef offsetof
>> +#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
>> +#endif
>> +
>> +#ifndef container_of
>> +#define container_of(PTR, TYPE, FIELD) ({			\
>> +	__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
>> +	(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
>> +})
>> +#endif
> 
> These ones are independent on the malloc code and should move to a
> different patch and likely to a different file. I'm seeing we already
> have a few macros in types.h and since it's shared by almost everything
> it might be more suitable there.

OK, will do it in the v2. Thanks!

-- 
Ammar Faizi