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

build: use feature probes for CLOEXEC #4206

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Sep 18, 2023

Description of changes:

The current code always tries to enable _XOPEN_SOURCE around the fcntl.h include. This causes problems on certain platforms where this define can break. To mitigate this, this PR adds 2 new feature probes.

The first one is S2N_CLOEXEC_SUPPORTED, which detects if we can get the O_CLOEXEC variable without defining _XOPEN_SOURCE. If this passes, we don't do anything special at the include site. This is nice, since we naturally limit the scope of the include by not opting into anything extra.

The second one is S2N_CLOEXEC_XOPEN_SUPPORTED, which detects if we can get the O_CLOEXEC variable if we define _XOPEN_SOURCE. If this passes, but not the previous one, then we define _XOPEN_SOURCE.

If neither of the probes pass, then we just include the header as normal. The code later on will check if O_CLOEXEC is in scope anyway.

Call-outs:

I tried to accomplish this with 1 probe, but couldn't figure out a reliable way. Let me know if you can think of anything.

Testing:

In each of the build outputs you can see the new probes. I also manually tested each of the combinations by failing the probes and was able to compile in each mode.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Sep 18, 2023
@camshaft camshaft marked this pull request as ready for review September 19, 2023 19:21
@@ -0,0 +1 @@
-Werror
Copy link
Contributor

Choose a reason for hiding this comment

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

remind me why we need -Werror here. I see some .flags files have this and other dont

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some probes might want to narrow the scope of the errors to particular warnings, rather than getting "unused variables" or "unused functions", etc.

The thinking here is the probe is small enough that you wouldn't get any other warnings from anything but the include.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at code, it seems like GLOBAL_FLAGS and PROBE_FLAGS are both used in the try_compile.

To me it seems like the flags are additive rather than narrowing. Am I miss-reading cmake here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct; they are narrowing. But you may not want -Werror included globally, since it would be more difficult to change on an individual probe.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is additive.

You also still have to be careful of "unused variables" or you'll break make ;) #4187

@camshaft camshaft enabled auto-merge (squash) September 19, 2023 21:33
@camshaft camshaft merged commit 9d1e6c8 into aws:main Sep 19, 2023
23 checks passed
@camshaft camshaft deleted the cloexec-xopen branch September 19, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants