Since #1206, on macOS x86_64 with Valgrind installed, clang emits a massive amount of -Wreserved-identifier and -Wreserved-macro-identifier warnings from the valgrind/valgrind.h and valgrind/memcheck.h headers.
This PR prevents warnings emitted for the Valgrind code.
56@@ -57,7 +57,7 @@ case $host_os in
57 # Homebrew where each one is located, then adjust paths accordingly.
58 if $BREW list --versions valgrind >/dev/null; then
59 valgrind_prefix=$($BREW --prefix valgrind 2>/dev/null)
60- VALGRIND_CPPFLAGS="-I$valgrind_prefix/include"
61+ VALGRIND_CPPFLAGS="-nostdinc -isystem $valgrind_prefix/include"
-nostdinc but I don’t see it.
/usr/local/include directory is included by default. Packages been installed via Homebrew have alternative symlinks in it, in addition to ones in $(brew --prefix ${package})/include.
I wonder if anyone out there uses a compiler that doesn’t understand -isystem. But writing a check seems overkill too… Maybe just check if this is gcc or clang ("x$GCC" = "xyes" should do it)?
Yet another alternative is to wrap the #include in pragmas.
brew install valgrind on macOS for more than 2 years (https://github.com/Homebrew/homebrew-core/pull/67341).
And we should really bring https://github.com/bitcoin-core/secp256k1/commit/c0ae48c9950a908b637bff27791fabbe2833c4a5 back, at least the valgrind part.
I’m not sure if that’ll be possible? The fork you’ve mentioned doesn’t support arm64, and Cirrus only supports arm64 macOS instances.
Maybe just check if this is gcc or clang (
"x$GCC" = "xyes"should do it)?
I tend to lean towards that suggestion. And we should probably add a comment that explains why we have -isystem in the first place, and why we need -nostdinc. :)
Yet another alternative is to wrap the
#includein pragmas.
I’ve taken this approach for the following reasons:
Here is an example of Valgrind usage on macOS Monterey 12.6.6 (x86_64):
0% valgrind ./build/src/ctime_tests
1==18400== Memcheck, a memory error detector
2==18400== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
3==18400== Using Valgrind-3.21.0.GIT-lbmacos and LibVEX; rerun with -h for copyright info
4==18400== Command: ./build/src/ctime_tests
5==18400==
6--18400-- run: /usr/bin/dsymutil "./build/src/ctime_tests"
7--18400-- run: /usr/bin/dsymutil "/Users/hebasto/secp256k1/build/src/libsecp256k1.2.0.2.dylib"
8==18400==
9==18400== HEAP SUMMARY:
10==18400== in use at exit: 7,982 bytes in 165 blocks
11==18400== total heap usage: 169 allocs, 4 frees, 8,262 bytes allocated
12==18400==
13==18400== LEAK SUMMARY:
14==18400== definitely lost: 4,160 bytes in 130 blocks
15==18400== indirectly lost: 0 bytes in 0 blocks
16==18400== possibly lost: 576 bytes in 2 blocks
17==18400== still reachable: 3,246 bytes in 33 blocks
18==18400== suppressed: 0 bytes in 0 blocks
19==18400== Rerun with --leak-check=full to see details of leaked memory
20==18400==
21==18400== For lists of detected and suppressed errors, rerun with: -s
22==18400== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 122 from 21)
where we’ll have x86_64 macOS runners (see #1394).
I might be missing something, but that combination (x86_64 & macos 13) isn’t supported by Valgrind, or the fork mentioned above?
Also, have we followed up upstream about actually fixing the cause of the warnings, if possible?
I might be missing something, but that combination (x86_64 & macos 13) isn’t supported by Valgrind, or the fork mentioned above?
It is supported by the fork mentioned above, see the table at https://github.com/LouisBrunner/valgrind-macos#status, column amd64.
Also, have we followed up upstream about actually fixing the cause of the warnings, if possible?
No, and I suspect it won’t be fruitful in that case. I think the usual convention is that warnings should be off when compiling system headers. Or at least that only essential warnings should be enabled, and our -Wreserved-identifier and -Wreserved-macro-identifier are rather uncommon. This is why we have -isystem and the like.
In this case here, the issue is that with the homebrew on macOS, the headers end up at a normal include path (-I) and not at a system include path (-isystem). So this is just about convincing clang to treat these includes as system headers. I think reporting to Valgrind (or the fork) won’t make much sense. In theory, we could report to Apple clang and complain about their decision to treating /usr/local/bin as a normal include path, but I don’t think anyone will care.
It’s hard to know, given the PR doesn’t actually outline what the warnings where, but I’m pretty sure the issue is that the valgrind/memcheck headers have identifiers starting with __, and any warnings are correct, regardless of anything (Apple) Clang, or macOS, or system include related.
i.e compiling this code:
0#include <valgrind.h>
1#include <memcheck.h>
2
3int main() { return 0;}
with clang on ubuntu: clang test.c -I/usr/include/valgrind -Wreserved-macro-identifier -Wreserved-identifier, outputs warnings as expected:
0In file included from test.c:1:
1/usr/include/valgrind/valgrind.h:74:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
2#define __VALGRIND_H
3 ^
4/usr/include/valgrind/valgrind.h:91:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
5#define __VALGRIND_MAJOR__ 3
6 ^
7/usr/include/valgrind/valgrind.h:92:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
8#define __VALGRIND_MINOR__ 19
9 ^
10/usr/include/valgrind/valgrind.h:416:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
11#define __SPECIAL_INSTRUCTION_PREAMBLE \
12 ^
13/usr/include/valgrind/valgrind.h:1666:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
14#define __CALLER_SAVED_REGS /*"rax",*/ "rcx", "rdx", "rsi", \
15 ^
16/usr/include/valgrind/valgrind.h:1724:11: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
17# define __FRAME_POINTER \
18 ^
19In file included from test.c:2:
20/usr/include/valgrind/memcheck.h:61:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
21#define __MEMCHECK_H
22 ^
23/usr/include/valgrind/memcheck.h:103:7: warning: identifier '_VG_USERREQ__MEMCHECK_RECORD_OVERLAP_ERROR' is reserved because it starts with '_' followed by a capital letter [-Wreserved-identifier]
24 _VG_USERREQ__MEMCHECK_RECORD_OVERLAP_ERROR
25 ^
268 warnings generated.
but I’m pretty sure the issue is that the valgrind/memcheck headers have identifiers starting with
__,
Right.
any warnings are correct, regardless of anything (Apple) Clang, or macOS, or system include related.
Okay, what do you suggest doing? Reporting it to Valgrind?
Okay, what do you suggest doing? Reporting it to Valgrind?
I’ve had a look, and I think it’s unlikey that Valgrind will refactor to fix any of these, as many are part of external APIs. I’ll see if there are any that can be cleaned up and send a patch upstream if so.
Makes sense.
Note that while we try to be strict about this, even the C committee itself went a step back and reworked the “reserved identifier” stuff for C23. Now identifiers like __VALGRIND_H are only “potentially reserved”, which just means “don’t use them because future C version may make them reserved”. This at least solves the (rather theoretical) problem that any program declaring __VALGRIND_H simply had UB by any literal reading of the standard. Now with C23, this is not UB anymore, but programmers are warned that their programs may have UB in future versions of C.
Labels
build