refactor: clean up CMakelists.txt #4779

jmayclin · 2024-09-18T01:57:36Z

Description of changes:

I was reading through out CMakeLists.txt and noticed a number of odd/unused configurations. This PR removes them.

~~Wno-missing-braces is not needed for libs2n~~ jk, GCC 4 strikes again
replace a regex with the simpler get_filename_component
remove unnecessary headers from testlib definition
remove the -fPIC compile option on tests, since it is already specified as a public option on libs2n
specify the compile definition _POSIX_C_SOURCE=200809L using the dedicated target_compile_definition syntax
specify linker options -fsanitize=fuzzer -lstdc++ using the dedicated target_link_options syntax
remove unnecessary target_include_directories(${TEST_NAME} PRIVATE ./)
remove unused LD_PRELOAD allocator_overrides

Call-outs:

Allocator Overrides

allocator_overrides is never used when tests are run through CMake. Original motivation for the test can be found in these PRs

From the above PRs, the goal with the allocator overrides was the help catch places that we were using uninitialized variables. My current take is the -Wuninitialized (which we currently use) and clang-static-analyze (which we don't) are more robust ways of catching these errors. As such, I would vote for fully removing the allocator overrides ones we have deprecated the make builds.

Section Headers

I know that my big section header comments might be polarizing, but the motivation was as follows

the BUILD_TESTING section is getting extremely long
there is no syntax highlighting, so it's just a wall of difficult to read text.

That's why I think the very hefty section headers are justified. A single comment is just going to get lost in the wash of monocolored text.

Compile Options: Individual Lines

The compile options were getting very difficult to read. Additionally, having them on a single line makes the git blame much more useful.

Testing:

All CI should pass

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

I actually can't replace the other shared lib with the one used for unit tests, because the fuzz tests need to be dynamically linked because they rely on LD_PRELOAD to clobber certain symbols.

no-missing-braces is required for GCC 4 support

jouho

Not disagreeing with the section header, but you can get syntax highlighting on CMake file if you get extension on VS Code which made my life x10 easier.

Thank you for cleaning this up!

lrstewart

Love the cleanup!

My suggestion for a PR like this would be to comment on your own PR in-line with explanations of the various changes, rather than summarizing in the description. I've found that helps when the reasoning behind line changes aren't clear, but a persistent code comment wouldn't make sense (particularly for deletes).

lrstewart · 2024-09-20T06:39:46Z

CMakeLists.txt

+    # GCC 4 fails to parse our macros with a "missing-braces" error
+    -Wno-missing-braces


Should we have an issue for this? ARE macros missing braces?

ARE macros missing braces?

My understanding is that the answer is "no" otherwise things would actually be broken.

/codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c: In function 'main': /codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c:116:34: warning: missing braces around initializer [-Wmissing-braces] DEFER_CLEANUP(struct s2n_stuffer cert_chain_stuffer = { 0 }, s2n_stuffer_free); ^ /codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/./utils/s2n_safety.h:62:43: note: in definition of macro 'DEFER_CLEANUP' __attribute__((cleanup(_thecleanup))) _thealloc ^

console link

Other people have run into similar false positives with old versions of GCC.
OP-TEE/optee_os#1998

I did find a bug report that seems related, but it says the fix was backported to GCC 4.8.3 and our CI version is 4.8.5

lrstewart · 2024-09-20T06:42:08Z

CMakeLists.txt

+        # NAME_WE: name without extension
+        get_filename_component(test_case_name ${test_case} NAME_WE)


Neat, a very cmake way to do it!

lrstewart · 2024-09-20T06:44:25Z

CMakeLists.txt

+            -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized 
+            -Wshadow  -Wcast-align -Wwrite-strings -Wformat-security
+            -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated -std=gnu99


Probably out of scope of this refactor, but it'd be nice if we had a list of common warnings we want enabled. It's not clear whether this matches the warnings on the main library, and whether the differences are intentional or just a mistake because someone forgot to update everywhere.

I agree. I think the proper way to do this is to define

S2N_COMMON_WARNINGS

And then if S2N_WERROR_ALL is set then you would just make the compile options on libs2n public, ensuring that all of the associated items (test utility library, fuzz tests, unit tests) all just get them by default. I'll open an issue for it.

Created an action item for this in #4792

lrstewart · 2024-09-20T06:49:12Z

CMakeLists.txt

-        target_include_directories(${test_case_name} PRIVATE ./)
        target_link_libraries(${test_case_name} PRIVATE testss2n)


Any idea what these target_include_directories are trying to do?

target_include_directories is used to indicate the header file locations that are necessary for that specific target.

This is an odd directive to use for an individual test binary, because those test binaries don't have associated header files.

I think the generic thought was "test binary needs access to the test headers" but those test headers are already publicly associated with testss2n. So when testss2n is linked to the test binary, the test binary will also get the test headers.

s2n-tls/CMakeLists.txt

Line 453 in 2467416

target_include_directories(testss2n PUBLIC tests)

CMakeLists.txt

jmayclin · 2024-09-20T20:10:36Z

CMakeLists.txt

+    # GCC 4 fails to parse our macros with a "missing-braces" error
+    -Wno-missing-braces


ARE macros missing braces?

My understanding is that the answer is "no" otherwise things would actually be broken.

/codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c: In function 'main': /codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c:116:34: warning: missing braces around initializer [-Wmissing-braces] DEFER_CLEANUP(struct s2n_stuffer cert_chain_stuffer = { 0 }, s2n_stuffer_free); ^ /codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/./utils/s2n_safety.h:62:43: note: in definition of macro 'DEFER_CLEANUP' __attribute__((cleanup(_thecleanup))) _thealloc ^

console link

Other people have run into similar false positives with old versions of GCC.
OP-TEE/optee_os#1998

I did find a bug report that seems related, but it says the fix was backported to GCC 4.8.3 and our CI version is 4.8.5

CMakeLists.txt

jmayclin · 2024-09-20T20:27:24Z

CMakeLists.txt

-        target_include_directories(${test_case_name} PRIVATE ./)
        target_link_libraries(${test_case_name} PRIVATE testss2n)


target_include_directories is used to indicate the header file locations that are necessary for that specific target.

This is an odd directive to use for an individual test binary, because those test binaries don't have associated header files.

I think the generic thought was "test binary needs access to the test headers" but those test headers are already publicly associated with testss2n. So when testss2n is linked to the test binary, the test binary will also get the test headers.

s2n-tls/CMakeLists.txt

Line 453 in 2467416

target_include_directories(testss2n PUBLIC tests)

CMakeLists.txt

jmayclin · 2024-09-20T20:29:42Z

CMakeLists.txt

        add_custom_command(
            TARGET testss2n POST_BUILD
            COMMAND
                objcopy --redefine-syms libcrypto.symbols lib/libtestss2n.a
        )
    endif()

-    #run unit tests
-    file (GLOB TEST_LD_PRELOAD "tests/LD_PRELOAD/*.c")


Removing this because it is unused. I searched the entire codebase (not just CMakeLists.txt) and found that this library is only used when unit tests are run through make. See the PR overview for my thoughts on why we should totally delete this library.

jmayclin · 2024-09-20T20:30:56Z

CMakeLists.txt


-    add_library(testss2n STATIC ${TESTLIB_HEADERS} ${TESTLIB_SRC} ${EXAMPLES_SRC})


Headers shouldn't be included as part of the add_library directive. Generally the add_library directive is just used for things that generate object files. Header should then be added through target_include_directories, but that is already configured to add the tests/ path.

jmayclin · 2024-09-20T20:33:02Z

CMakeLists.txt

+            -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized 
+            -Wshadow  -Wcast-align -Wwrite-strings -Wformat-security
+            -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated -std=gnu99


I agree. I think the proper way to do this is to define

S2N_COMMON_WARNINGS

And then if S2N_WERROR_ALL is set then you would just make the compile options on libs2n public, ensuring that all of the associated items (test utility library, fuzz tests, unit tests) all just get them by default. I'll open an issue for it.

refactor: clean up CMakelists.txt

0f4ee06

github-actions bot added the s2n-core team label Sep 18, 2024

jmayclin added 2 commits September 18, 2024 18:12

address ci failure

d5cb871

I actually can't replace the other shared lib with the one used for unit tests, because the fuzz tests need to be dynamically linked because they rely on LD_PRELOAD to clobber certain symbols.

address ci failure

9fea9b2

no-missing-braces is required for GCC 4 support

jmayclin marked this pull request as ready for review September 19, 2024 23:17

jmayclin requested review from jouho and lrstewart September 19, 2024 23:17

jouho approved these changes Sep 19, 2024

View reviewed changes

lrstewart reviewed Sep 20, 2024

View reviewed changes

camshaft reviewed Sep 20, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

try removing POSIX_SOURCE compile definition

1002b60

jmayclin commented Sep 20, 2024

View reviewed changes

camshaft approved these changes Sep 20, 2024

View reviewed changes

jmayclin requested a review from lrstewart September 20, 2024 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean up CMakelists.txt #4779

refactor: clean up CMakelists.txt #4779

jmayclin commented Sep 18, 2024 •

edited

Loading

jouho left a comment

lrstewart left a comment •

edited

Loading

lrstewart Sep 20, 2024

jmayclin Sep 20, 2024

lrstewart Sep 20, 2024

lrstewart Sep 20, 2024

jmayclin Sep 20, 2024

jmayclin Sep 20, 2024

lrstewart Sep 20, 2024

jmayclin Sep 20, 2024

jmayclin Sep 20, 2024

jmayclin Sep 20, 2024

jmayclin Sep 20, 2024 •

edited

Loading

jmayclin Sep 20, 2024

jmayclin Sep 20, 2024

		# GCC 4 fails to parse our macros with a "missing-braces" error
		-Wno-missing-braces

		# NAME_WE: name without extension
		get_filename_component(test_case_name ${test_case} NAME_WE)

		target_include_directories(${test_case_name} PRIVATE ./)
		target_link_libraries(${test_case_name} PRIVATE testss2n)


		add_library(testss2n STATIC ${TESTLIB_HEADERS} ${TESTLIB_SRC} ${EXAMPLES_SRC})

refactor: clean up CMakelists.txt #4779

Are you sure you want to change the base?

refactor: clean up CMakelists.txt #4779

Conversation

jmayclin commented Sep 18, 2024 • edited Loading

Description of changes:

Call-outs:

Allocator Overrides

Section Headers

Compile Options: Individual Lines

Testing:

jouho left a comment

Choose a reason for hiding this comment

lrstewart left a comment • edited Loading

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

jmayclin Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmayclin commented Sep 18, 2024 •

edited

Loading

lrstewart left a comment •

edited

Loading

jmayclin Sep 20, 2024 •

edited

Loading