Add -Wall -Werror for enclave code & fix issues #377
Conversation
|
I love this PR! |
| @@ -1,6 +1,8 @@ | |||
| #ifndef _LKL_SYSCALL_OVERRIDES_FUTEX_H | |||
| #define _LKL_SYSCALL_OVERRIDES_FUTEX_H | |||
|
|
|||
| #include <time.h> | |||
There was a problem hiding this comment.
I think this is the wrong include, we need the LKL types, not any system ones. They may be the same, but only by coincidence.
There was a problem hiding this comment.
Ideally yes, but other pieces pull in musl headers as well and if I make a change here then we'll get redefinition errors. Concretely, musl's pthread.h (included by enclave_oe.c) includes bits/alltypes.h which defines timespec. I'll leave it as it is, I'm sure this won't be an issue anymore after relayering.
There was a problem hiding this comment.
Really? This file should be included in only two places and neither of them should include musl.
That said, this file is actually dead code, so a simpler fix is to delete it and the #includes of it...
There was a problem hiding this comment.
This requires a change in sgx-lkl-musl (src/internal/syscall.h) which is currently at 393a467 but the futex syscall bypass removal is a few commits later. Very confusing... Should I create a PR against sgx-lkl-musl's oe_port and then bump the submodule to the newest commit?
There was a problem hiding this comment.
Yes please. I thought we'd bumped the musl version when we merged the change.
There was a problem hiding this comment.
lsds/sgx-lkl-musl#19. I bumped sgx-lkl-musl to the PR commit. If CI passes here, we can merge the sgx-lkl-musl PR and then bump to the merged commit in oe_port (which may or may not be the same).
There was a problem hiding this comment.
This is failing left and right... I have a feeling it's due to the sgx-lkl-musl bump. I'll open a PR with just the bump and get that merged first (or not, if it exposes the same issues).
There was a problem hiding this comment.
I opened #410 but I realized that this is already being done in another PR. Probably that's the one you were thinking of, it's not merged yet. I will have to wait for that first.
There was a problem hiding this comment.
We should undo the fstat byassing in sgx-lkl-musl. We merged the PR for sgx-lkl-musl, but not the corresponding PR to sgx-lkl. So if you bump sgx-lkl-msul, the orphen change will be included.
-Wall -Werror was only used for host-side code.