Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory relocation #36

Draft
wants to merge 152 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 151 commits
Commits
Show all changes
152 commits
Select commit Hold shift + click to select a range
3cbb748
Add basic structs and runner initialization
fmoletta Jul 17, 2023
c32247d
Start implementing tests
fmoletta Jul 17, 2023
93ff635
Add initializers + move vm into runner + minor fixes
fmoletta Jul 17, 2023
871638a
Minor fixes
fmoletta Jul 17, 2023
768049f
Improve runner test
fmoletta Jul 17, 2023
683eca8
Fix memory new size
fmoletta Jul 18, 2023
6982d8b
Add memory tests
fmoletta Jul 18, 2023
330c509
Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into cair…
fmoletta Jul 18, 2023
74d742c
Implement memory_get + run new formatter
fmoletta Jul 18, 2023
2c5b7d6
Implement memory_insert
fmoletta Jul 18, 2023
d62a338
fixes'
fmoletta Jul 18, 2023
1ae32e5
Add file
fmoletta Jul 19, 2023
44f91eb
Finish load_data tests
fmoletta Jul 19, 2023
9b83d6d
Fix bug
fmoletta Jul 19, 2023
0bdf1e1
Fix bug
fmoletta Jul 19, 2023
d193b3f
Add boolean is_felt to maybe_relocatable type
fmoletta Jul 20, 2023
1f43986
Fixes
fmoletta Jul 20, 2023
49805ee
Add fix + test
fmoletta Jul 20, 2023
4353f64
Add more tests
fmoletta Jul 20, 2023
1b22061
Add test
fmoletta Jul 20, 2023
2f11a7e
Handle memory gaps in memory functions
fmoletta Jul 21, 2023
65cb901
Add more tests
fmoletta Jul 21, 2023
8093541
Fix bug
fmoletta Jul 21, 2023
d026137
Add tests
fmoletta Jul 21, 2023
3ab79b3
Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into cair…
fmoletta Jul 21, 2023
d1b7492
Use felt_t in maybe_relocatable + adapt tests
fmoletta Jul 24, 2023
27ad5c5
Rename function & change parameter type
fmoletta Jul 24, 2023
c3da197
Remove unused import
fmoletta Jul 24, 2023
f147768
Remove unused import
fmoletta Jul 24, 2023
1726141
Add memory_free function
fmoletta Jul 24, 2023
e778bb9
Free memory in memory_tests
fmoletta Jul 24, 2023
31ffde5
fmt
fmoletta Jul 24, 2023
8d16b3f
Use clear in stead of free
fmoletta Jul 24, 2023
2dc7b1e
Free runner
fmoletta Jul 24, 2023
b9cbaf5
Add function comments
fmoletta Jul 24, 2023
d3c7165
Add missing execution segment data loading
fmoletta Jul 24, 2023
073b9c2
Add error handling to runner init & memory load_data
fmoletta Jul 24, 2023
5ea00bc
Revert "Add error handling to runner init & memory load_data"
fmoletta Jul 24, 2023
b366829
add error handling to memory_load_data
fmoletta Jul 24, 2023
59aa6cd
Add err case result
fmoletta Jul 24, 2023
1641508
Add warning, move comment to header
fmoletta Jul 24, 2023
4a0fce9
Move comments to header
fmoletta Jul 24, 2023
364bb88
Free program
fmoletta Jul 24, 2023
fd379ad
Free data used in load_data tests
fmoletta Jul 24, 2023
34d73ec
Swap clear for free
fmoletta Jul 24, 2023
e9ebde7
Copy name collision quickfix from parser pr
fmoletta Jul 24, 2023
91fc3af
Free felts stored in memory
fmoletta Jul 24, 2023
276c7ed
Fix bug
fmoletta Jul 24, 2023
87908c5
Revert "Copy name collision quickfix from parser pr"
fmoletta Jul 24, 2023
5ce16e1
Revert "Free felts stored in memory"
fmoletta Jul 24, 2023
27d58db
Revert "Revert "Copy name collision quickfix from parser pr""
fmoletta Jul 24, 2023
5d1cb15
Add free in memory_add_segment
fmoletta Jul 24, 2023
ecc539f
Add clear
fmoletta Jul 24, 2023
5003cf4
Revert "Add clear"
fmoletta Jul 24, 2023
156b6bc
Test
fmoletta Jul 24, 2023
0e06021
fmt
fmoletta Jul 24, 2023
39fa31e
Revert change
fmoletta Jul 24, 2023
fb62c88
Test
fmoletta Jul 24, 2023
9339225
Revert "Test"
fmoletta Jul 24, 2023
ad17642
Test
fmoletta Jul 24, 2023
d51f564
Test
fmoletta Jul 24, 2023
1215e12
Revert
fmoletta Jul 24, 2023
e95a506
Test
fmoletta Jul 24, 2023
2868221
Test
fmoletta Jul 24, 2023
04f4bb3
Revert
fmoletta Jul 24, 2023
9a908f0
Initial progress
fmoletta Jul 24, 2023
ddd7327
Complete replace
fmoletta Jul 24, 2023
b6eae99
Merge branch 'main' into cairo-runner-and-initialization
jrchatruc Jul 24, 2023
46080d8
Test
jrchatruc Jul 25, 2023
7a3eab0
Merge branch 'main' into cairo-runner-and-initialization
jrchatruc Jul 25, 2023
7a9c388
format
jrchatruc Jul 25, 2023
7733f88
Fix references
fmoletta Jul 25, 2023
df1ba72
Switch memory structure to hashtable
fmoletta Jul 25, 2023
397258a
Handle keys and values using pointers in memory funcs
fmoletta Jul 25, 2023
a6aa156
Fix unintended double pointer
fmoletta Jul 25, 2023
1914b4d
Fix unintended double pointer
fmoletta Jul 25, 2023
8011708
Misc fixes
fmoletta Jul 25, 2023
7b9d02f
Fix bug
fmoletta Jul 25, 2023
1619478
Fix some tests, reuse ptr in load_data
fmoletta Jul 25, 2023
5004cb5
Restore ptr value after load
fmoletta Jul 25, 2023
37be285
Add print_memory fn
fmoletta Jul 25, 2023
3578b67
Partial fmt
fmoletta Jul 25, 2023
558c7c0
Add test
fmoletta Jul 25, 2023
1758adc
fmt
fmoletta Jul 25, 2023
c96fc24
Fix wrong test being ran
fmoletta Jul 25, 2023
1f60b89
Add test
fmoletta Jul 25, 2023
04ba4a9
Add comment explaining the hacky function
jrchatruc Jul 25, 2023
26fc93c
Allocate memory for values in memory_insert
fmoletta Jul 26, 2023
149d477
Restore memory_get signature
fmoletta Jul 26, 2023
f9b910e
Restore memory_load_data signature
fmoletta Jul 26, 2023
3554bb9
Merge branch 'cairo-runner-and-initialization' into switch-lib
fmoletta Jul 26, 2023
73f09d3
Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into swit…
fmoletta Jul 26, 2023
5f93b3d
Add lib build to makefile
fmoletta Jul 26, 2023
a0b99da
test
fmoletta Jul 26, 2023
fd931ed
test
fmoletta Jul 26, 2023
d292af9
Fix dir name
fmoletta Jul 26, 2023
ee8d84a
Install pkg-config in CI
fmoletta Jul 26, 2023
0850b8e
Install cpputest in CI
fmoletta Jul 26, 2023
4ee1bc8
Add sudo make install
fmoletta Jul 26, 2023
337d4e1
Improve collections lib installation
fmoletta Jul 26, 2023
4cfda73
Cast values to avoid problems due to architectural differences
fmoletta Jul 26, 2023
0b4ccc3
Remove unused fn
fmoletta Jul 26, 2023
52eeec2
test
fmoletta Jul 26, 2023
3002695
Test
fmoletta Jul 26, 2023
ac4f34e
Handle CI stuff in CI file
fmoletta Jul 26, 2023
8cbe5a5
Allocate program data in test
fmoletta Jul 26, 2023
0debb94
Fix memory free
fmoletta Jul 26, 2023
ec7b4e5
comment tests
fmoletta Jul 26, 2023
c0e262f
fmt
fmoletta Jul 26, 2023
719a954
Uncomment one test
fmoletta Jul 26, 2023
c25e065
Improve memory_free
fmoletta Jul 26, 2023
6de2333
fmt
fmoletta Jul 26, 2023
be9f432
Uncomment memory tests
fmoletta Jul 26, 2023
dc4d509
Free keys
fmoletta Jul 26, 2023
6ef4928
Check if we can remove this line
fmoletta Jul 26, 2023
6ab52c5
Remove all
fmoletta Jul 26, 2023
50af070
Revert "Uncomment memory tests"
fmoletta Jul 26, 2023
983a436
Uncomment tests
fmoletta Jul 26, 2023
5bd52ff
Dont perform insertion when overwriting with same value
fmoletta Jul 26, 2023
68085d2
Uncomment tests
fmoletta Jul 26, 2023
ef18e4e
Remove commented print debug
fmoletta Jul 26, 2023
3b917ac
Uncomment memory tests
fmoletta Jul 26, 2023
351a712
Destroy array used by load_data tests
fmoletta Jul 26, 2023
dae2cf1
Destroy stack
fmoletta Jul 26, 2023
711d8d3
Fix
fmoletta Jul 26, 2023
265df47
Uncomment all tests
fmoletta Jul 26, 2023
14ffeda
uncomment free
fmoletta Jul 26, 2023
cb671d2
Comment tets
fmoletta Jul 26, 2023
7fa51a1
Destroy array
fmoletta Jul 26, 2023
e39c562
Uncomment test
fmoletta Jul 26, 2023
42cfc8e
Install necessary pakages & add makefile command to build collections…
fmoletta Jul 26, 2023
ce5495c
Run collections-lib build in Dockerfile
fmoletta Jul 26, 2023
e567907
Switch conditional branches
fmoletta Jul 27, 2023
8311f66
Add lib dependencies to Dependencies
fmoletta Jul 27, 2023
f56cec6
Add note
fmoletta Jul 27, 2023
f717da3
Destroy stack in the fn that creates it
fmoletta Jul 27, 2023
2e7826d
Merge branch 'main' of github.com:lambdaclass/cairo-vm_in_C into swit…
fmoletta Jul 27, 2023
8d567ce
Fix conflict in readme
fmoletta Jul 27, 2023
3c753b8
Merge branch 'build-collections-lib-in-docker' into switch-lib
fmoletta Jul 27, 2023
7be35cc
Add Collections-C folder to gitignore
fmoletta Jul 27, 2023
2f7d610
Add targets for building collections lib in docker
fmoletta Jul 27, 2023
1c94c58
Merge branch 'switch-lib' of github.com:lambdaclass/cairo-vm_in_C int…
fmoletta Jul 27, 2023
968dfbb
Update .gitignore
fmoletta Jul 27, 2023
85c424a
Fix target name
fmoletta Jul 27, 2023
8cdb836
Use iterator in load_data
fmoletta Jul 28, 2023
d756c34
Merge branch 'switch-lib' of github.com:lambdaclass/cairo-vm_in_C int…
fmoletta Jul 28, 2023
8bbf39a
Add skeleton and relocation test
gabrielbosio Jul 27, 2023
2674e1b
Start implementing relocation
gabrielbosio Jul 27, 2023
f48ef0d
Implement memory relocation
gabrielbosio Jul 28, 2023
0b4a017
Free runner arrays on exit
gabrielbosio Jul 28, 2023
8f98213
Try to solve memory leaks
gabrielbosio Jul 28, 2023
fbe0f9e
Merge branch 'main' into relocate_segments
gabrielbosio Jul 31, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ jobs:
- uses: dtolnay/[email protected]
- uses: actions/checkout@v3
- run: make check_fmt
- run: sudo apt install cpputest -y
- run: make Collections-C
- run: sudo ldconfig
- run: make test
- run: sudo apt install valgrind -y
- run: make SANITIZER_FLAGS=-fno-omit-frame-pointer valgrind
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,5 @@ dkms.conf
build/
.vscode/
.gdb_history

Collections-C
2 changes: 2 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
FROM rust:latest

RUN apt update && \
apt install cmake -y && \
apt install cpputest -y && \
apt install valgrind -y

WORKDIR /usr/cairo-vm_in_C
Expand Down
37 changes: 32 additions & 5 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.PHONY: clean fmt check_fmt valgrind compile_rust deps_macos docker_build docker_run docker_test_and_format docker_clean
.PHONY: clean fmt check_fmt valgrind compile_rust deps_macos build_collections_lib docker_build docker_run docker_test_and_format docker_clean docker_build_collections_lib docker_valgrind

TARGET=cairo_vm
TEST_TARGET=cairo_vm_test
Expand All @@ -9,7 +9,7 @@ SANITIZER_FLAGS=-fsanitize=address -fno-omit-frame-pointer
CFLAGS=-std=c11 -Wall -Wextra -Wimplicit-fallthrough -Werror -pedantic -g -O0
CXX_FLAGS=-std=c++14 -Wall -Wextra -Wimplicit-fallthrough -Werror -pedantic -g -O0
CFLAGS_TEST=-I./src
LN_FLAGS=-L./lambdaworks/lib/lambdaworks/target/release/ -Bstatic -llambdaworks -ldl -lpthread -lm
LN_FLAGS=-L./lambdaworks/lib/lambdaworks/target/release/ -Bstatic -llambdaworks -lcollectc -ldl -lpthread -lm

BUILD_DIR=./build
SRC_DIR=./src
Expand All @@ -30,6 +30,16 @@ OBJECTS_CPP = $(patsubst $(SRC_DIR)/%.cpp, $(BUILD_DIR)/%.o, $(SOURCE_CPP))
TEST_OBJECTS = $(patsubst $(TEST_DIR)/%.c, $(BUILD_DIR)/%.o, $(TEST_SOURCE))
LIB_OBJECTS_CPP = $(patsubst $(LIB_DIR)/%.cpp, $(BUILD_DIR)/%.o, $(LIB_SOURCE_CPP))

COLLECTIONS_LIB_DIR= Collections-C
$(COLLECTIONS_LIB_DIR):
git clone https://github.com/srdja/Collections-C.git
cd Collections-C && \
mkdir build && \
cd build && \
cmake .. && \
make && \
sudo make install

# Gcc/Clang will create these .d files containing dependencies.
DEP = $(OBJECTS:%.o=%.d)

Expand Down Expand Up @@ -77,11 +87,12 @@ deps_macos:
run: default
$(BUILD_DIR)/$(TARGET)

test: compile_rust $(TEST_TARGET)
test: compile_rust build_collections_lib $(TEST_TARGET)
$(BUILD_DIR)/$(TEST_TARGET)

clean:
rm -rf $(BUILD_DIR) && \
rm -rf $(COLLECTIONS_LIB_DIR) && \
cd lambdaworks/lib/lambdaworks && cargo clean

fmt:
Expand All @@ -90,12 +101,14 @@ fmt:
check_fmt:
clang-format --style=file -Werror -n $(SOURCE) $(TEST_SOURCE) $(HEADERS) $(TEST_HEADERS)

valgrind: clean compile_rust $(TEST_TARGET)
valgrind: clean compile_rust build_collections_lib $(TEST_TARGET)
valgrind --leak-check=full --show-reachable=yes --show-leak-kinds=all --track-origins=yes --error-exitcode=1 ./build/cairo_vm_test

compile_rust:
cd lambdaworks/lib/lambdaworks && cargo build --release

build_collections_lib: | $(COLLECTIONS_LIB_DIR)

docker_build:
docker build . -t cairo-vm_in_c

Expand All @@ -105,9 +118,23 @@ docker_run:
docker_test_and_format:
docker create --name test -t -v `pwd`:/usr/cairo-vm_in_C cairo-vm_in_c
docker start test
docker exec -t test bash -c "make && make test && make SANITIZER_FLAGS=-fno-omit-frame-pointer valgrind"
docker exec -t test bash -c "make clean && make docker_build_collections_lib && make test && make SANITIZER_FLAGS=-fno-omit-frame-pointer docker_valgrind"
docker stop test
docker rm test

docker_clean:
docker rmi cairo-vm_in_c

docker_build_collections_lib:
git clone https://github.com/srdja/Collections-C.git
cd Collections-C && \
mkdir build && \
cd build && \
cmake .. && \
make && \
make install && \
ldconfig

docker_valgrind: clean compile_rust docker_build_collections_lib $(TEST_TARGET)
valgrind --leak-check=full --show-reachable=yes --show-leak-kinds=all --track-origins=yes --error-exitcode=1 ./build/cairo_vm_test

11 changes: 10 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ This is a work in progress implementation of the [Cairo VM](https://github.com/l
- A working C compiler with `C11` support.
- `clang-format` installed in the system.
- [Rust](https://www.rust-lang.org/tools/install)
- [cmake](https://cmake.org/install/)
- pkg-config
- [cpputest](http://cpputest.github.io)
- Docker (for valgrind on MacOS)

## Local development
Expand Down Expand Up @@ -38,6 +41,12 @@ To remove all compilation objects:
make clean
```

Note: When building outside of macos, you may have to run this command after building the collections lib to make the system's runtime aware of the location of the new library:

```
sudo ldconfig
```

### Valgrind/Asan on MacOS

To run `valgrind` on MacOS, first run:
Expand All @@ -57,7 +66,7 @@ This will run a new container from the built image and will execute bash by usin
Finally, run:

```
make SANITIZER_FLAGS=-fno-omit-frame-pointer valgrind
make SANITIZER_FLAGS=-fno-omit-frame-pointer docker_valgrind
```

## Tests
Expand Down
134 changes: 125 additions & 9 deletions src/cairo_runner.c
Original file line number Diff line number Diff line change
@@ -1,20 +1,35 @@
#include "cairo_runner.h"
#include "collectc/cc_common.h"
#include "collectc/cc_hashtable.h"
#include "memory.h"
#include "program.h"
#include "relocatable.h"
#include "vm.h"
#include <assert.h>
#include <collectc/cc_array.h>
#include <stdio.h>
#include <stdlib.h>

cairo_runner runner_new(struct program program) {
virtual_machine vm = vm_new();
cairo_runner runner = {program, vm, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}};
CC_Array *relocated_memory;
cc_array_new(&relocated_memory);
CC_Array *relocation_table;
cc_array_new(&relocation_table);
cairo_runner runner = {program, vm, relocated_memory, relocation_table, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}};
return runner;
}

void runner_free(cairo_runner *runner) {
memory_free(&runner->vm.memory);
program_free(&runner->program);
cc_array_remove_all_free(runner->relocated_memory);
cc_array_destroy(runner->relocated_memory);
cc_array_remove_all_free(runner->relocation_table);
cc_array_destroy(runner->relocation_table);
}

// Creates program, execution and builtin segments
// Creates program, execution and builtin segments.
void runner_initialize_segments(cairo_runner *runner) {
// Program Segment
runner->program_base = memory_add_segment(&runner->vm.memory);
Expand All @@ -25,7 +40,7 @@ void runner_initialize_segments(cairo_runner *runner) {

// Initializes the program segment & initial pc
// Warning: Can fail if used outside of runner_initialize
void runner_initialize_state(cairo_runner *runner, unsigned int entrypoint, CList *stack) {
void runner_initialize_state(cairo_runner *runner, unsigned int entrypoint, CC_Array *stack) {
runner->initial_pc = runner->program_base;
runner->initial_pc.offset += entrypoint;
// We ignore the error cases when loading data as these cases are unreachable when called after
Expand All @@ -39,15 +54,15 @@ void runner_initialize_state(cairo_runner *runner, unsigned int entrypoint, CLis
// Initializes memory, initial register values & returns the end pointer (final pc) to run from a given pc offset
// (entrypoint)
// Warning: Can fail if runner_initialize_segments is not called before
relocatable runner_initialize_function_entrypoint(cairo_runner *runner, unsigned int entrypoint, CList *stack,
relocatable runner_initialize_function_entrypoint(cairo_runner *runner, unsigned int entrypoint, CC_Array *stack,
relocatable return_fp) {
relocatable end = memory_add_segment(&runner->vm.memory);
maybe_relocatable return_fp_mr = {.is_felt = false, .value = {.relocatable = return_fp}};
maybe_relocatable end_mr = {.is_felt = false, .value = {.relocatable = end}};
stack->add(stack, &return_fp_mr);
stack->add(stack, &end_mr);
cc_array_add(stack, &return_fp_mr);
cc_array_add(stack, &end_mr);
runner->initial_fp.segment_index = runner->execution_base.segment_index;
runner->initial_fp.offset = stack->count(stack);
runner->initial_fp.offset = cc_array_size(stack);
runner_initialize_state(runner, entrypoint, stack);
runner->final_pc = end;
return end;
Expand All @@ -56,12 +71,13 @@ relocatable runner_initialize_function_entrypoint(cairo_runner *runner, unsigned
// Initializes memory, initial register values & returns the end pointer (final pc) to run from the main entrypoint
// Warning: Can fail if runner_initialize_segments is not called before
relocatable runner_initialize_main_entrypoint(cairo_runner *runner) {
CList *stack = CList_init(sizeof(maybe_relocatable));
CC_Array *stack;
cc_array_new(&stack);
// Handle builtin initial stack
// Handle proof-mode specific behaviour
relocatable return_fp = memory_add_segment(&runner->vm.memory);
relocatable end = runner_initialize_function_entrypoint(runner, runner->program.main, stack, return_fp);
stack->free(stack);
cc_array_destroy(stack);
return end;
}

Expand All @@ -74,10 +90,110 @@ void runner_initialize_vm(cairo_runner *runner) {
// Apply validation rules to memory
}

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);
}
}
}
Comment on lines +93 to +117
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few issues and suggestions with this function.

  1. 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.
  2. Issue: the call to cc_array_replace_at is leaking the old value. With suggestion 1 this automatically gets fixed.
  3. 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.
  4. 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.


// 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);
}
}
Comment on lines +119 to +138
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another batch of suggestions:

  1. Suggestion: rather than warn about possible errors in a comment, it may be a good idea to return an error instead.
  2. Suggestion: use values instead of heap pointers, as mentioned in the other comment.
  3. Issue: not checking segment_size for NULL.
  4. Suggestion: no need to check insertion with cc_array_get_at, you can check the output of cc_array_add instead.


relocatable runner_initialize(cairo_runner *runner) {
// runner_initialize_builtins
runner_initialize_segments(runner);
relocatable end = runner_initialize_main_entrypoint(runner);
runner_initialize_vm(runner);
return end;
}

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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: not checking address for NULL.

}

void runner_get_relocated_value(cairo_runner *runner, maybe_relocatable *value, felt_t out) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (value->is_felt) {
zero(out);
add(out, value->value.felt, out);
} else {
int out_number = runner_get_relocated_address(runner, &value->value.relocatable);
from(out, out_number);
}
}

void runner_relocate_memory(cairo_runner *runner) {
assert(cc_array_size(runner->relocated_memory) == 0);
relocated_memory_cell *first_cell = malloc(sizeof(relocated_memory_cell));
first_cell->is_some = false;
first_cell->memory_value.none = 0;
cc_array_add(runner->relocated_memory, first_cell);

CC_Array *segment_sizes;
cc_array_new(&segment_sizes);
runner_get_segment_sizes(runner, segment_sizes);
runner_fill_relocation_table(runner, segment_sizes);

int num_segment_sizes = cc_array_size(segment_sizes);

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: not checking segment_size for NULL.


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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for cell.

int relocated_addr = runner_get_relocated_address(runner, &ptr);
relocated_memory_cell *relocated_value = malloc(sizeof(relocated_memory_cell));
relocated_value->is_some = true;
runner_get_relocated_value(runner, cell, relocated_value->memory_value.value);
cc_array_add_at(runner->relocated_memory, relocated_value, relocated_addr);
} else {
relocated_memory_cell *none = malloc(sizeof(relocated_memory_cell));
none->is_some = false;
none->memory_value.none = 0;
cc_array_add(runner->relocated_memory, none);
}
}
}
}
16 changes: 15 additions & 1 deletion src/cairo_runner.h
Original file line number Diff line number Diff line change
@@ -1,14 +1,26 @@
#ifndef RUNNER_H
#define RUNNER_H

#include "clist.h"
#include "collectc/cc_array.h"
#include "program.h"
#include "relocatable.h"
#include "vm.h"

typedef union relocated_memory_value {
felt_t value;
int none;
} relocated_memory_value;

typedef struct relocated_memory_cell {
relocated_memory_value memory_value;
bool is_some;
} relocated_memory_cell;

typedef struct cairo_runner {
struct program program;
virtual_machine vm;
CC_Array *relocated_memory;
CC_Array *relocation_table;
relocatable program_base;
relocatable execution_base;
relocatable initial_pc;
Expand All @@ -26,4 +38,6 @@ void runner_free(cairo_runner *runner);
// Performs the intialization step, leaving the runner ready to run a loaded cairo program from a main entrypoint
relocatable runner_initialize(cairo_runner *runner);

void runner_relocate_memory(cairo_runner *runner);

#endif
Loading