sqlite: Initial port #417

msdx321 · 2021-03-18T16:37:02Z

Summary of this Pull Request (PR)

Add description here.

Intent for your PR

Choose one (Mandatory):

This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)

Code Quality

As part of this pull request, I've considered the following:

Style:

Comments adhere to the Style Guide (SG)
Spacing adhere's to the SG
Naming adhere's to the SG
All other aspects of the SG are adhered to, or exceptions are justified in this pull request
I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

I've made an attempt to remove all redundant code
I've considered ways in which my changes might impact existing code, and cleaned it up
I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
I've commented appropriately where code is tricky
I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

micro_booter
unit_pingpong
unit_schedtests
...(add others here)

Signed-off-by: msdx321 <[email protected]>

Others · 2021-03-20T17:41:00Z

src/components/lib/posix/posix.c

@@ -31,7 +34,7 @@ libc_syscall_override(cos_syscall_t fn, int syscall_num)
 	cos_syscalls[syscall_num] = fn;
 }

-struct sl_lock stdout_lock = SL_LOCK_STATIC_INIT();
+//struct sl_lock stdout_lock = SL_LOCK_STATIC_INIT();


Is there demand for a single threaded version of this? Removing all this synchronization logic is going to lead to weird bugs as soon as you have more than one thread. Why doesn't it work as-is? Too tight coupling to SL?

Great question, and good intuition. We revamping libposix right now to use a generic interface that can either be co-located (via library) with sl, or accessed in another component. Given this, most of the synchronization facilities (and sl dependencies) in posix need to change backing APIs.

As such, Bite is trying to get sqlite working for a comparison case, and is not trying to solve other problems for now. As we update posix, I'm hoping we can add back in those features.

A lot of the cruft assuming single-component runtimes has been removed, but we have more to do, and libposix is one of the big places more love needs to be applied.

gparmer

I think that a lot of your changes are in the sqlite repo. How do you want to proceed with that. See my comment about having a main branch that tracks sqlite's upstream.

Some comments about how to avoid losing all of the sync and futex code.

gparmer · 2021-03-20T21:04:17Z

.gitmodules

@@ -4,3 +4,6 @@
 [submodule "src/components/lib/ck/ck"]
 	path = src/components/lib/ck/ck
 	url = https://github.com/gwsystems/ck.git
+[submodule "src/components/lib/sqlite/sqlite"]


You'll want to move this into gwsystems. Remember, we want the main branch of the repo be unmodifed, and have a cos branch that we pull in as a submodule. That way we have a path for updating the code as upstream changes.

gparmer · 2021-03-20T21:04:56Z

src/components/implementation/tests/sqlite_tests/doc.md

@@ -0,0 +1,9 @@
+## tests
+
+This is the skeleton interface used by the `mkcomponent.sh` script to aid in the creation of a new component.


Add documentation here, or remove the file.

gparmer · 2021-03-20T21:05:33Z

src/components/implementation/tests/sqlite_tests/sqlite_tests.c

+{
+}
+
+int result_print(void *v, int argc, char **argv, 


your line width wraparound seems too aggressive. I don't think that the argument needs to be on another line.

gparmer · 2021-03-20T21:06:01Z

src/components/implementation/tests/sqlite_tests/sqlite_tests.c

+#include <cos_defkernel_api.h>
+
+void
+cos_init(void)


If you remove this function, it should still work. If you don't have code in it, remove it!

gparmer · 2021-03-20T21:06:46Z

src/components/implementation/tests/sqlite_tests/sqlite_tests.c

+int
+main(void)
+{
+	printc("Calling sqlite functions\n");


inconsistent indentation styules. likely mixing spaces and tabs.

variable definitions should be at the top of the block.

gparmer · 2021-03-20T21:14:38Z

src/components/lib/posix/posix.c

-	completed_successfully = sl_thd_block_timeout(0, wakeup_deadline);
-	wakeup_time   = sl_now();
+	//wakeup_deadline        = sl_now() + sl_usec2cyc(time_to_microsec(req));
+	wakeup_deadline = 0;


I don't think that silently failing is the best policy here. We should probably fail loudly if a function that we don't support is called. BUG(), assert(0), after a printc to explain. etc...

I don't trust client checking for ENOTSUP which would be the normal solution. Failure is more appropriate if it isn't supported here.

gparmer · 2021-03-20T21:15:20Z

src/components/lib/posix/posix.c

@@ -263,23 +272,27 @@ cos_set_tid_address(int *tidptr)
 * };
 */

-void* backing_data[SL_MAX_NUM_THDS];
+//void* backing_data[SL_MAX_NUM_THDS];
+void* backing_data[1];


COS_NUM_THREADS, I think???

gparmer · 2021-03-20T21:16:23Z

src/components/lib/posix/posix.c

@@ -291,11 +304,13 @@ cos_clone(int (*func)(void *), void *stack, int flags, void *arg, pid_t *ptid, v
 		return -1;
 	}

-	struct sl_thd * thd = sl_thd_alloc((cos_thd_fn_t) func, arg);
+	//struct sl_thd * thd = sl_thd_alloc((cos_thd_fn_t) func, arg);
+	struct sl_thd * thd = 0;


This shouldn't compile, right? Where is the defn of sl_thd coming from here?

gparmer · 2021-03-20T21:18:25Z

src/components/lib/posix/posix.c

 	assert(0);
 }

 /* TODO: Cleanup empty futexes */

-struct sl_lock futex_lock = SL_LOCK_STATIC_INIT();
+//struct sl_lock futex_lock = SL_LOCK_STATIC_INIT();

 /*
 * precondition: futex_lock is taken
 */
 int
 cos_futex_wait(struct futex_data *futex, int *uaddr, int val, const struct timespec *timeout)
 {


This is all pretty challenging logic that Gregor spent a lot of time on. What I'd like: create an issue with a link to the commit in which this is removed, so that we can know how to easily bring back this code when hook up posix to the rest of the system.

I think there have to be ways to do this that don't sacrifice correctness. (ie that fault instead of being unsafe concurrently) I'm always nervous leaving things in a subtly broken state--it's easy to forget the exact brokenness and put yourself in debugging hell for no reason later

Great point.

Perhaps we replace the current lock implementation with something that simply errors out on contention:

struct lock { unsigned long owner; }; struct lock global_lock = (struct lock) { .owner = 0 }; void lock_take(void) { // failure here indicates that you're using concurrency/parallelism, which posix doesn't yet support! if (!ps_cas(&global_lock.owner, 0, cos_thdid())) assert(0); } void lock_release(void) { // failure here indicates unpaired take and release. assert(global_lock.owner == cos_thdid()); ps_store(&global_lock.owner, 0); }

Replace normal lock behavior which requires contention management with simple "error out".

gparmer · 2021-03-20T21:19:20Z

src/components/lib/sqlite/doc.md

@@ -0,0 +1,9 @@
+## sqlite
+
+This is the skeleton library used by the `mklib.sh` script to aid in the creation of a new library.


documentation or remove.

Signed-off-by: msdx321 <[email protected]>

msdx321 added 5 commits March 19, 2021 00:33

libc: Fix library output name

a2c3566

Signed-off-by: msdx321 <[email protected]>

posix: Nuke SL

cd5b02f

Signed-off-by: msdx321 <[email protected]>

lib: sqlite: Initial import

0707a4c

Signed-off-by: msdx321 <[email protected]>

sqlite: Update to latest

1ea129f

Signed-off-by: msdx321 <[email protected]>

sqlite_tests: Add initial test

8c79591

Signed-off-by: msdx321 <[email protected]>

Others reviewed Mar 20, 2021

View reviewed changes

gparmer requested changes Mar 20, 2021

View reviewed changes

msdx321 added 17 commits August 9, 2021 11:05

.gitmodules: Update

9d9dc43

Signed-off-by: msdx321 <[email protected]>

libc: Fix compile

cccfa82

Signed-off-by: msdx321 <[email protected]>

capmgr: Return mapped mem properly

dac0a2b

Signed-off-by: msdx321 <[email protected]>

ps: Set to my fork

564b08d

Signed-off-by: msdx321 <[email protected]>

posix: Make it work for now

0a8176a

Signed-off-by: msdx321 <[email protected]>

blockdev: Add blockdev interface

2690e4f

Signed-off-by: msdx321 <[email protected]>

blockdev: Add ramdisk implementation

e8581ad

Signed-off-by: msdx321 <[email protected]>

ramdisk: Add a small test

89da24f

Signed-off-by: msdx321 <[email protected]>

filesystem: Add filesystem interface

dd5410f

Signed-off-by: msdx321 <[email protected]>

filesystem: Add ext4 implementation

0ee4ec1

Signed-off-by: msdx321 <[email protected]>

ext4: Add a small test

0adc138

Signed-off-by: msdx321 <[email protected]>

lwext4: Add as a lib

f9914ab

Signed-off-by: msdx321 <[email protected]>

.gitignore: Update

0b7f9e4

Signed-off-by: msdx321 <[email protected]>

Merge branch 'loader' into filesystem

2fddd37

Merge branch 'loader' into sqlite

f8adc21

Merge branch 'filesystem' into sqlite

e6ea277

Signed-off-by: msdx321 <[email protected]>

ps: Fix merge error

7c5069d

Signed-off-by: msdx321 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlite: Initial port #417

sqlite: Initial port #417

msdx321 commented Mar 18, 2021

Others Mar 20, 2021

gparmer Mar 20, 2021

gparmer left a comment

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

gparmer Mar 20, 2021

Others Mar 20, 2021

gparmer Mar 21, 2021

gparmer Mar 20, 2021

		@@ -0,0 +1,9 @@
		## tests

		This is the skeleton interface used by the `mkcomponent.sh` script to aid in the creation of a new component.

		@@ -0,0 +1,9 @@
		## sqlite

		This is the skeleton library used by the `mklib.sh` script to aid in the creation of a new library.

sqlite: Initial port #417

Are you sure you want to change the base?

sqlite: Initial port #417

Conversation

msdx321 commented Mar 18, 2021

Summary of this Pull Request (PR)

Intent for your PR

Reviewers (Mandatory):

Code Quality

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gparmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment