c-vector icon indicating copy to clipboard operation
c-vector copied to clipboard

Issue 40: Fix alignment memory accesses

Open julianhartmer opened this issue 1 year ago • 1 comments

PR is still in early state. It's not fully tested and still buggy. Here is how I would try to tackle the issue https://github.com/eteran/c-vector/issues/40. I introduced several helper macros that help with the calculation.

The PR uses the macro ALIGN_UP_TYPE(addr, type), which returns the aligned address addr to the type type if the addr is not aligned (see example at bottom).

Here is how I verified my changes.

$ cat test_changes.c 
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include "cvector.h"

struct complex_struct {
	uint64_t i;
	char *allocated_string;
};

void free_fn(void *elem)
{
	struct complex_struct *a = elem;
	free(a->allocated_string);
}

int main() {
	cvector_vector_type(struct complex_struct) vec = NULL;

	struct complex_struct a = {
		10,
		NULL
	};

	a.allocated_string = strdup("hello World");
	cvector_push_back(vec, a);
	cvector_set_elem_destructor(vec, free_fn);

	a.allocated_string = strdup("hello github");
	a.i++;
	cvector_push_back(vec, a);

	printf("%lu: %s\n", vec[0].i, vec[0].allocated_string);
	printf("%lu: %s\n", vec[1].i, vec[1].allocated_string);
	printf("size offset: %lu\n", cvector_size_offset);
	printf("capcity offset: %lu\n", cvector_capacity_offset);
	printf("data offset: %lu\n", cvector_data_offset(vec));
	printf("sizeof(*data) = %lu\n", sizeof(*vec));
	printf("%p -> elem destructor %p\n", vec, &cvector_elem_destructor_rval(vec));

	cvector_pop_back(vec);
	cvector_free(vec);
}
jh@jh-ThinkPad-X250:~/GameDev/c-vector
$ gcc test_changes.c -g -Wall -Wextra -pedantic && valgrind ./a.out
test_changes.c: In function ‘main’:
test_changes.c:39:11: warning: format ‘%p’ expects argument of type ‘void *’, but argument 2 has type ‘struct complex_struct *’ [-Wformat=]
   39 |  printf("%p -> elem destructor %p\n", vec, &cvector_elem_destructor_rval(vec)); \
      |          ~^                           ~~~
      |           |                           |
      |           void *                      struct complex_struct *
test_changes.c:39:33: warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘void (**)(void *)’ [-Wformat=]
   39 |  printf("%p -> elem destructor %p\n", vec, &cvector_elem_destructor_rval(vec)); \
      |                                ~^
      |                                 |
      |                                 void *
==5308== Memcheck, a memory error detector
==5308== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==5308== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==5308== Command: ./a.out
==5308== 
10: hello World
11: hello github
size offset: 8
capcity offset: 16
data offset: 24
sizeof(*data) = 16
0x4a59168 -> elem destructor 0x4a59150
==5308== 
==5308== HEAP SUMMARY:
==5308==     in use at exit: 0 bytes in 0 blocks
==5308==   total heap usage: 5 allocs, 5 frees, 1,145 bytes allocated
==5308== 
==5308== All heap blocks were freed -- no leaks are possible
==5308== 
==5308== For lists of detected and suppressed errors, rerun with: -s
==5308== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Here is my short test for the ALIGN_UP_TYPE macro:

$ cat tmp.c 
#include <stdio.h>
#include <stdint.h>

#include "cvector.h"

int main()
{
	for (uint64_t i = 0; i < 30; ++i) {
		printf("align up %lu->%lu\n", i, ALIGN_UP_TYPE(i, uint64_t));
	}
	uint64_t *v = NULL;
	printf("%lu, %lu, %lu\n", sizeof (cvector_elem_destructor_t), sizeof(size_t), sizeof(uint64_t));
	printf("&0->capacity = %lu\n", cvector_capacity_offset);
	printf("&0->size = %lu\n", cvector_size_offset);
	printf("&0->data = %lu\n", cvector_data_offset(v));
}
jh@jh-ThinkPad-X250:~/GameDev/c-vector
$ gcc tmp.c -Wall -Wextra -pedantic -o test_align_up && ./test_align_up
align up 0->0
align up 1->8
align up 2->8
align up 3->8
align up 4->8
align up 5->8
align up 6->8
align up 7->8
align up 8->8
align up 9->16
align up 10->16
align up 11->16
align up 12->16
align up 13->16
align up 14->16
align up 15->16
align up 16->16
align up 17->24
align up 18->24
align up 19->24
align up 20->24
align up 21->24
align up 22->24
align up 23->24
align up 24->24
align up 25->32
align up 26->32
align up 27->32
align up 28->32
align up 29->32
8, 8, 8
&0->capacity = 16
&0->size = 8
&0->data = 24

julianhartmer avatar Jul 14 '22 07:07 julianhartmer

fixed naming of macro: rval is actually a lval :man_facepalming:

julianhartmer avatar Jul 14 '22 07:07 julianhartmer

I believe that the alignment issue is resolved by another PR which uses a struct to represent the meta-data. Please reopen the issue/PR if you still have concerns.

eteran avatar Feb 22 '23 18:02 eteran