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

Rewrite cmake files to simplify build system #309

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Conversation

rinon
Copy link
Collaborator

@rinon rinon commented Nov 15, 2023

Replaces the define_test and define_shared_lib functions with a more generic and hopefully cleaner add_ia2_compartment function.

@rinon rinon requested review from ayrtonm and fw-immunant November 15, 2023 22:38
@rinon rinon force-pushed the sjc/build_system_rewrite branch from 417c2f8 to 6ff5ab8 Compare November 15, 2023 22:39
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I don't understand the second commit. Were we padding the TLS segment of partition-alloc before? I don't think we should need to do this, because we don't pkey_mprotect its thread locals as it isn't itself "in" any compartment. That said, I don't think I thought hard about the interaction of TLS usage in partition-alloc with our TLS scheme; I don't think I had really realized it was using TLS at all.

It does, though, for partition_alloc::(anonymous namespace)::g_disallow_allocations, partition_alloc::internal::(anonymous namespace)::ReentrantScannerGuard::guard_, partition_alloc::internal::g_thread_cache, partition_alloc::internal::base::(anonymous namespace)::g_thread_id, and partition_alloc::internal::base::(anonymous namespace)::g_is_main_thread.

Copy link
Contributor

@ayrtonm ayrtonm left a comment

Choose a reason for hiding this comment

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

LGTM with mostly minor nits. There's just one case we need to fix where the -include flag is wrong and there's an issue with the partition alloc target that nginx tries to build.

cmake/ia2.cmake Outdated Show resolved Hide resolved
cmake/ia2.cmake Outdated Show resolved Hide resolved
cmake/ia2.cmake Show resolved Hide resolved
partition-alloc/CMakeLists.txt Outdated Show resolved Hide resolved
@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 19, 2023

@rinon I saw you effectively removed the NEEDS_LD_WRAP from the new cmake functions and I think passing an ld args file unconditionally is fine here. For the objcopy step added in #300 (that's implemented in a similar way) were you also planning on doing that unconditionally or keeping NEEDS_OBJCOPY_FILE?

@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 19, 2023

I'm rebasing #300 on this PR so for now I'll leave NEEDS_OBJCOPY as a no-op arg in the original cmake functions (like NEEDS_LD_WRAP) and do it unconditionally (or just for tests with more than two pkeys) in the new add_ia2_compartment. Then we can figure out how to check if it's necessary (maybe building the original targets and looking for duplicate symbols with nm).

@rinon rinon force-pushed the sjc/build_system_rewrite branch from 2542815 to a3d3028 Compare March 13, 2024 21:40
@rinon rinon changed the base branch from main to sjc/misc_fixes March 13, 2024 21:42
@rinon rinon force-pushed the sjc/build_system_rewrite branch 2 times, most recently from c479da0 to 1ce7fba Compare March 13, 2024 22:37
Base automatically changed from sjc/misc_fixes to main March 13, 2024 22:39
@rinon rinon force-pushed the sjc/build_system_rewrite branch from 1ce7fba to 7b2f78b Compare March 13, 2024 22:39
@rinon rinon requested a review from ayrtonm March 13, 2024 22:39
@ayrtonm
Copy link
Contributor

ayrtonm commented Mar 14, 2024

LGTM. Could you update the target name for PartitionAlloc in docs/build_instructions.md?

rinon added 3 commits March 25, 2024 16:56
Replaces the define_test and define_shared_lib functions with a more
generic and hopefully cleaner add_ia2_compartment function.
@ayrtonm ayrtonm force-pushed the sjc/build_system_rewrite branch 2 times, most recently from 0d07c0f to 6dab542 Compare March 25, 2024 20:59
@ayrtonm ayrtonm merged commit 944d8c6 into main Mar 25, 2024
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants