Memory relocation by gabrielbosio · Pull Request #36 · lambdaclass/cairo-vm.c

gabrielbosio · 2023-07-27T21:49:42Z

NOTE: To merge this, #26 has to be merged.

…o-runner-and-initialization

… lib in docker

…ch-lib

…o switch-lib

Start with nested for loops that iterate over segments and cells.

Oppen

Besides the individual comments, I left a few other missing NULL checks in the output parameters of hash table and array accesses without comment, as it's generalized and I was spamming too much already.

Oppen · 2023-07-31T13:15:13Z

src/cairo_runner.c

+void runner_get_segment_sizes(cairo_runner *runner, CC_Array *segment_sizes) {
+	int num_segments = runner->vm.memory.num_segments;
+	unsigned int *zero;
+	for (int i = 0; i < num_segments; i++) {
+		zero = malloc(sizeof(unsigned int));
+		*zero = 0;
+		cc_array_add(segment_sizes, zero);
+	}
+
+	unsigned int *new_size;
+	CC_HashTableIter memory_iter;
+	cc_hashtable_iter_init(&memory_iter, runner->vm.memory.data);
+	TableEntry *entry;
+	while (cc_hashtable_iter_next(&memory_iter, &entry) != CC_ITER_END) {
+		relocatable *ptr = entry->key;
+		unsigned int *segment_size;
+		assert(cc_array_get_at(segment_sizes, ptr->segment_index, (void **)&segment_size) == CC_OK);
+
+		if (*segment_size < ptr->offset + 1) {
+			new_size = malloc(sizeof(unsigned int));
+			*new_size = ptr->offset + 1;
+			cc_array_replace_at(segment_sizes, new_size, ptr->segment_index, NULL);
+		}
+	}
+}


There are a few issues and suggestions with this function.

Suggestion: store values instead of pointer, as it simplifies memory management and avoids any chance of leaks. The array doesn't dereference the pointers and the value fits in the pointer size, so you can convert the values to uintptr_t and then void *. Document it appropriately.

Issue: the call to cc_array_replace_at is leaking the old value. With suggestion 1 this automatically gets fixed.

Suggestion: cc_array_get_at already gives you the pointer to the value and doesn't remove it, so if you choose to keep the heap allocated values you can overwrite the value pointer by it instead, also fixing the leaks.

Issue: you're not checking for NULL for segment_size before dereferencing it. If it's the first inserted value then you hit undefined behavior. You should check for NULL and store unconditionally in that case, since it's the first value you see for that segment.

Oppen · 2023-07-31T13:20:07Z

src/cairo_runner.c

+// Fills relocation table with the first relocated address of each memory segment
+// Warning: Can fail if runner_initialize_main_entrypoint is not called before
+void runner_fill_relocation_table(cairo_runner *runner, CC_Array *segment_sizes) {
+	int *first_addr = malloc(sizeof(int));
+	*first_addr = 1;
+	cc_array_add(runner->relocation_table, first_addr);
+	int *last_segment_size_added;
+	assert(cc_array_get_at(runner->relocation_table, 0, (void **)&last_segment_size_added) == CC_OK);
+
+	int relocation_table_size = runner->vm.memory.num_segments + 1;
+	for (int i = 1; i < relocation_table_size; i++) {
+		int segment_idx = i - 1;
+		int *segment_size;
+		assert(cc_array_get_at(segment_sizes, segment_idx, (void **)&segment_size) == CC_OK);
+		int *address = malloc(sizeof(int));
+		*address = *last_segment_size_added + *segment_size;
+		cc_array_add(runner->relocation_table, address);
+		assert(cc_array_get_at(runner->relocation_table, i, (void **)&last_segment_size_added) == CC_OK);
+	}
+}


Another batch of suggestions:

Suggestion: rather than warn about possible errors in a comment, it may be a good idea to return an error instead.

Suggestion: use values instead of heap pointers, as mentioned in the other comment.

Issue: not checking segment_size for NULL.

Suggestion: no need to check insertion with cc_array_get_at, you can check the output of cc_array_add instead.

Oppen · 2023-07-31T13:20:36Z

src/cairo_runner.c

+int runner_get_relocated_address(cairo_runner *runner, relocatable *relocatable) {
+	int *address;
+	assert(cc_array_get_at(runner->relocation_table, relocatable->segment_index, (void **)&address) == CC_OK);
+	return *address + relocatable->offset;


Issue: not checking address for NULL.

Oppen · 2023-07-31T13:22:20Z

src/cairo_runner.c

+	return *address + relocatable->offset;
+}
+
+void runner_get_relocated_value(cairo_runner *runner, maybe_relocatable *value, felt_t out) {


If we're modifying out, shouldn't it be a pointer so the caller sees the changes? Ignore this if felt_t is already defined to be a pointer.

Oppen · 2023-07-31T13:25:57Z

src/cairo_runner.c

+
+	for (int i = 0; i < num_segment_sizes; i++) {
+		int *segment_size;
+		assert(cc_array_get_at(segment_sizes, i, (void **)&segment_size) == CC_OK);


Issue: not checking segment_size for NULL.

Oppen · 2023-07-31T13:26:32Z

src/cairo_runner.c

+		for (int j = 0; j < *segment_size; j++) {
+			relocatable ptr = {i, j};
+			maybe_relocatable *cell;
+			if (cc_hashtable_get(runner->vm.memory.data, &ptr, (void **)&cell) == CC_OK) {


Same for cell.

Oppen · 2023-07-31T13:29:14Z

src/memory.c

-	if (cell->is_some) {
-		ResultMemory ok = {.is_error = false, .value = {.memory_value = cell->memory_value.value}};
+	maybe_relocatable *value = NULL;
+	if (cc_hashtable_get(mem->data, &ptr, (void *)&value) == CC_OK) {


value could be NULL.

fmoletta added 30 commits July 17, 2023 08:11

Add basic structs and runner initialization

3cbb748

Start implementing tests

c32247d

Add initializers + move vm into runner + minor fixes

93ff635

Minor fixes

871638a

Improve runner test

768049f

Fix memory new size

683eca8

Add memory tests

6982d8b

Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into cair…

330c509

…o-runner-and-initialization

Implement memory_get + run new formatter

74d742c

Implement memory_insert

2c5b7d6

fixes'

d62a338

Add file

1ae32e5

Finish load_data tests

44f91eb

Fix bug

9b83d6d

Fix bug

0bdf1e1

Add boolean is_felt to maybe_relocatable type

d193b3f

Fixes

1f43986

Add fix + test

49805ee

Add more tests

4353f64

Add test

1b22061

Handle memory gaps in memory functions

2f11a7e

Add more tests

65cb901

Fix bug

8093541

Add tests

d026137

Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into cair…

3ab79b3

…o-runner-and-initialization

Use felt_t in maybe_relocatable + adapt tests

d1b7492

Rename function & change parameter type

27ad5c5

Remove unused import

c3da197

Remove unused import

f147768

Add memory_free function

1726141

fmoletta and others added 21 commits July 26, 2023 17:23

Uncomment all tests

265df47

uncomment free

14ffeda

Comment tets

cb671d2

Destroy array

7fa51a1

Uncomment test

e39c562

Install necessary pakages & add makefile command to build collections…

42cfc8e

… lib in docker

Run collections-lib build in Dockerfile

ce5495c

Switch conditional branches

e567907

Add lib dependencies to Dependencies

8311f66

Add note

f56cec6

Destroy stack in the fn that creates it

f717da3

Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into swit…

2e7826d

…ch-lib

Fix conflict in readme

8d567ce

Merge branch 'build-collections-lib-in-docker' into switch-lib

3c753b8

Add Collections-C folder to gitignore

7be35cc

Add targets for building collections lib in docker

2f7d610

Merge branch 'switch-lib' of github.com:lambdaclass/cairo-vm_in_C int…

1c94c58

…o switch-lib

Update .gitignore

968dfbb

Fix target name

85c424a

Use iterator in load_data

8cdb836

Merge branch 'switch-lib' of github.com:lambdaclass/cairo-vm_in_C int…

d756c34

…o switch-lib

gabrielbosio force-pushed the relocate_segments branch from c09c7cd to 89e966d Compare July 28, 2023 15:59

gabrielbosio added 2 commits July 28, 2023 14:42

Add skeleton and relocation test

8bbf39a

Start implementing relocation

2674e1b

Start with nested for loops that iterate over segments and cells.

gabrielbosio force-pushed the relocate_segments branch from 89e966d to 2674e1b Compare July 28, 2023 17:43

gabrielbosio added 3 commits July 28, 2023 17:41

Implement memory relocation

f48ef0d

Free runner arrays on exit

0b4a017

Try to solve memory leaks

8f98213

Oppen requested changes Jul 31, 2023

View reviewed changes

Merge branch 'main' into relocate_segments

fbe0f9e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Memory relocation#36

Memory relocation#36
gabrielbosio wants to merge 152 commits intomainfrom
relocate_segments

gabrielbosio commented Jul 27, 2023 •

edited

Loading

Uh oh!

Oppen left a comment

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Oppen Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

gabrielbosio commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Oppen left a comment

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Oppen Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gabrielbosio commented Jul 27, 2023 •

edited

Loading