-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add support for more than two compartments #300
Conversation
See this comment for latest status. |
92473ed
to
79e9d4f
Compare
CI is failing because I added a dummy |
|
99f179c
to
df2df21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake changes look alright to me after a quick pass
I've hit some serious roadblocks with the current cmake rules (before this PR), so I'm working on rewriting those. I can integrate these changes pretty easily once that is done, so I'm happy to rebase this. I just wanted to let you know that I'm working on the build system now, @ayrtonm, so you don't end up putting time into something that is changing. |
3a39c51
to
2fbc712
Compare
2fbc712
to
d7a98d6
Compare
rebased on main but I think I dropped the nginx commit. The three pkeys test builds but I didn't get a chance to run it on an MPK machine |
I'm not currently working on this @fw-immunant in case you have time to take a look at the test failure or make changes to this PR to minimize conflicts with the library-only mode stuff. Also the last two commits can probably be split off and merged independently though we should double check that the |
e1ed606
to
c2f4799
Compare
@ayrtonm could I get review here on just the commit "rewriter: find filenames of macros more often"? Everything else I'm pretty confident about, but I want to make sure that that change isn't contradicting what you were trying to handle with "tools/rewriter: Fix ignore_function". |
The objcopy args file is used to rename colliding ELF symbol names to avoid collisions with call gate names. This can happen with more than two compartments when compartment A and B both call function foo in C. With less than three compartments we normally generate one call gate for foo named __wrap_foo. With two cross-compartment callers, we need unique call gates since the pkeys are baked into the call gate .text so we need to suffix the call gate names with the pkeys. Since ld --wrap replace references to bar with __wrap_bar we also need to suffix the symbol names as seen in the caller DSOs.
This simplifies our CMake test infrastructure though it's not strictly necessary to run the objcopy step for all non-prebuilt object files.
Currently indirect call control flow is not tested and memory access tests are missing asserts so this test is expected to fail.
main binary must have pkey 1
c2f4799
to
5d9aa91
Compare
Rebased now that #349 has landed. |
get_filename doesn't seem to work for macros and the three_keys_minimal test declared most of the lib functions in a macro
5d9aa91
to
3f57ca8
Compare
@@ -103,6 +103,10 @@ static Filename get_filename(const clang::SourceLocation loc, | |||
abort(); | |||
} | |||
} | |||
// if we could not get a filename for the spelling loc, use the expansion filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to explicitly use get_expansion_filename
at the callsites instead of silently falling back to the expansion loc? I haven't looked at the uses in detail enough to know which is cleaner.
…not a file we cannot actually rewrite these if we don't have a useful location; in this case, we would add the annotation to the start of files, where it would generally precede a #include and break compilation entirely
4fea557
to
4a7b6ae
Compare
… rewrite file in root directory
4a7b6ae
to
72987cb
Compare
Right now this just separates functions that need direct call gates based on whether they'll need one or more call gates, modifies
define_shared_lib
to allow invoking it more than once per tests and updatesread_config
to use 3 compartments. The expected behavior is an ld error when building theread_config_main_wrapped
target sincedefine_test
doesn't pull in the third compartment DSO yet. Closes #276.