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

Fix sanitizer builds with clang #1402

Merged

Conversation

zuiderkwast
Copy link
Contributor

By including <stdatomic.h> after the other includes in the unit test, we can avoid redefining a macro which led to a build failure.

Fixes #1394

By including <stdatomic.h> after our other files, we avoid
redefining a macro.

Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast requested a review from madolson December 6, 2024 21:40
@zuiderkwast
Copy link
Contributor Author

We have #include <stdatomic.h> in several files now, so I wonder why we still have this conditional code in the Makefile:

# Detect if the compiler supports C11 _Atomic.
# NUMBER_SIGN_CHAR is a workaround to support both GNU Make 4.3 and older versions.
NUMBER_SIGN_CHAR := \#
C11_ATOMIC := $(shell sh -c 'echo "$(NUMBER_SIGN_CHAR)include <stdatomic.h>" > foo.c; \
	$(CC) -std=gnu11 -c foo.c -o foo.o > /dev/null 2>&1; \
	if [ -f foo.o ]; then echo "yes"; rm foo.o; fi; rm foo.c')
ifeq ($(C11_ATOMIC),yes)
	STD+=-std=gnu11
else
	STD+=-std=c99
endif

If we can't compile the foo.c test file here, we can't compile replication.c and other files, so we're effectively requiring it and we're always building with -std=gnu11 now?

@zuiderkwast
Copy link
Contributor Author

@uriyage FYI this fixes it.

@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.82%. Comparing base (6df376d) to head (17b07d0).
Report is 13 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1402      +/-   ##
============================================
- Coverage     70.82%   70.82%   -0.01%     
============================================
  Files           118      118              
  Lines         63561    63561              
============================================
- Hits          45020    45016       -4     
- Misses        18541    18545       +4     

see 13 files with indirect coverage changes

@zuiderkwast zuiderkwast merged commit f20d629 into valkey-io:unstable Dec 7, 2024
50 of 57 checks passed
@zuiderkwast zuiderkwast deleted the fix-clang-sanitizer-builds branch December 7, 2024 09:26
@zuiderkwast zuiderkwast added the test-failure An issue indicating a test failure label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) test-failure An issue indicating a test failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[test-failure] Sanitizer builds, '_DEFAULT_SOURCE' macro redefined
2 participants