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 clang build error in bitops.c #1794

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

rjd15372
Copy link
Member

@rjd15372 rjd15372 commented Feb 27, 2025

In a recent PR #1741 the new header <immintrin.h> was added, which transitively includes <mm_malloc.h> header, where a function called _mm_malloc(...) makes a call to the malloc function.

The Valkey server code explicitly sets the malloc function as a deprecated function in server.h:

void *malloc(size_t size) __attribute__((deprecated));

The Valkey server code is then compiled with
-Werror=deprecated-declarations option to detect the uses of deprecated functions like malloc, and due to this, when the bitops.c file is compiled with Clang, fails with the following error:

In file included from bitops.c:33:
In file included from /usr/lib/llvm-18/lib/clang/18/include/immintrin.h:26:
In file included from /usr/lib/llvm-18/lib/clang/18/include/xmmintrin.h:31:
/usr/lib/llvm-18/lib/clang/18/include/mm_malloc.h:35:12: error: 'malloc' is deprecated [-Werror,-Wdeprecated-declarations]
   35 |     return malloc(__size);
      |            ^
./server.h:3874:42: note: 'malloc' has been explicitly marked deprecated here
 3874 | void *malloc(size_t size) __attribute__((deprecated));

There is a difference in behavior though, between GCC and Clang. The bitops.c file compiles successfully with GCC.

I don't know exactly why GCC does not issue a warning in this case. My best guess is that GCC does not issue warnings from code of the standard library.

To fix the build error in clang, we explicitly use pragma macro to tell clang to ignore deprecated declarations warnings in bitops.c.

In a recent PR valkey-io#1741 the new
header `<immintrin.h>` was added, which transitively includes
`<mm_malloc.h>` header, where a function called `_mm_malloc(...)` makes
a call to the `malloc` function.

The Valkey server code explicitly sets the malloc function as a
deprecated function in `server.h`:
```c
void *malloc(size_t size) __attribute__((deprecated));
```

The Valkey server code is then compiled with
`-Werror=deprecated-declarations` option to detect the uses of
deprecated functions like `malloc`, and due to this, when the `bitops.c`
file is compiled with Clang, it fails with the following error:

```
In file included from bitops.c:33:
In file included from /usr/lib/llvm-18/lib/clang/18/include/immintrin.h:26:
In file included from /usr/lib/llvm-18/lib/clang/18/include/xmmintrin.h:31:
/usr/lib/llvm-18/lib/clang/18/include/mm_malloc.h:35:12: error: 'malloc' is deprecated [-Werror,-Wdeprecated-declarations]
   35 |     return malloc(__size);
      |            ^
./server.h:3874:42: note: 'malloc' has been explicitly marked deprecated here
 3874 | void *malloc(size_t size) __attribute__((deprecated));
```

There is a difference in behavior though, between GCC and Clang. The
`bitops.c` file compiles successfully with GCC.

I don't know exactly why GCC does not issue a warning in this case. My
best guess is that GCC does not issue warnings from code of the standard
library.

To fix the build error in clang, we explicitly use `pragma` macro to
tell clang to ignore deprecated declarations warnings in `bitops.c`.

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@rjd15372 rjd15372 requested a review from zuiderkwast February 27, 2025 16:41
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.13%. Comparing base (4966357) to head (bae74c6).
Report is 3 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1794      +/-   ##
============================================
+ Coverage     70.97%   71.13%   +0.16%     
============================================
  Files           123      123              
  Lines         65645    65648       +3     
============================================
+ Hits          46590    46698     +108     
+ Misses        19055    18950     -105     
Files with missing lines Coverage Δ
src/bitops.c 94.05% <ø> (+4.05%) ⬆️

... and 14 files with indirect coverage changes

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ricardo Dias <rjd15372@gmail.com>
@hwware hwware merged commit e23a7d6 into valkey-io:unstable Feb 27, 2025
51 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