Copilot commented on code in PR #3224:
URL: https://github.com/apache/brpc/pull/3224#discussion_r2903737651
##########
src/butil/logging.cc:
##########
@@ -129,22 +129,41 @@ DEFINE_bool(print_stack_on_check, true,
"Print the stack trace when a CHECK was failed");
BUTIL_VALIDATE_GFLAG(print_stack_on_check, butil::PassValidate);
+#if defined(OS_FREEBSD)
+// Rename flags to avoid gflags duplicate registration with system glog.
+// glog defines "v" and "vmodule"; we define "brpc_verbose" and "brpc_vmodule"
+// and create local aliases so brpc code can still use FLAGS_v / FLAGS_vmodule.
+DEFINE_int32(brpc_verbose, 0, "Show all VLOG(m) messages for m <= this."
+ " Overridable by --brpc_vmodule.");
+DEFINE_string(brpc_vmodule, "", "per-module verbose level.");
+#define FLAGS_v FLAGS_brpc_verbose
+#define FLAGS_vmodule FLAGS_brpc_vmodule
+#else
DEFINE_int32(v, 0, "Show all VLOG(m) messages for m <= this."
" Overridable by --vmodule.");
DEFINE_string(vmodule, "", "per-module verbose level."
" Argument is a comma-separated list of MODULE_NAME=LOG_LEVEL."
" MODULE_NAME is a glob pattern, matched against the filename
base"
" (that is, name ignoring .cpp/.h)."
" LOG_LEVEL overrides any value given by --v.");
+#endif
DEFINE_bool(log_pid, false, "Log process id");
DEFINE_bool(log_bid, true, "Log bthread id");
+#if defined(OS_FREEBSD)
+DEFINE_int32(brpc_minloglevel, 0, "Any log at or above this level will be "
+ "displayed. Anything below this level will be silently ignored. "
+ "0=INFO 1=NOTICE 2=WARNING 3=ERROR 4=FATAL");
+BUTIL_VALIDATE_GFLAG(brpc_minloglevel, butil::NonNegativeInteger);
+#define FLAGS_minloglevel FLAGS_brpc_minloglevel
+#else
DEFINE_int32(minloglevel, 0, "Any log at or above this level will be "
"displayed. Anything below this level will be silently ignored. "
"0=INFO 1=NOTICE 2=WARNING 3=ERROR 4=FATAL");
BUTIL_VALIDATE_GFLAG(minloglevel, butil::NonNegativeInteger);
+#endif
Review Comment:
On FreeBSD this file renames the gflags from `v`/`vmodule`/`minloglevel` to
`brpc_*` and only adds `#define FLAGS_v ...` aliases in this .cc file. However
`src/butil/logging.h` still has `DECLARE_int32(v);` and uses
`::logging::FLAGS_v` (and the code registers validators against
`FLAGS_vmodule`), so other translation units will still expect the
`logging::FLAGS_v`/`FLAGS_vmodule`/`FLAGS_minloglevel` symbols to exist and
will fail to link on FreeBSD. The aliases need to be made visible to all users
(e.g., add corresponding `#if defined(OS_FREEBSD)` mapping and `DECLARE_*`
updates in `logging.h`, and adjust any flag-name based code/tests as needed).
##########
CMakeLists.txt:
##########
@@ -147,6 +147,11 @@ endif()
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DBTHREAD_USE_FAST_PTHREAD_MUTEX
-D__const__=__unused__ -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
-DBRPC_REVISION=\\\"${BRPC_REVISION}\\\" -D__STRICT_ANSI__")
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} ${DEBUG_SYMBOL} ${THRIFT_CPP_FLAG}")
set(CMAKE_CXX_FLAGS "${CMAKE_CPP_FLAGS} ${CMAKE_CXX_FLAGS} -O2 -pipe -Wall -W
-fPIC -fstrict-aliasing -Wno-invalid-offsetof -Wno-unused-parameter
-fno-omit-frame-pointer")
+
+if(CMAKE_SYSTEM_NAME STREQUAL "FreeBSD")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DNO_PTHREAD_MUTEX_HOOK")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DNO_PTHREAD_MUTEX_HOOK")
Review Comment:
The FreeBSD `-DNO_PTHREAD_MUTEX_HOOK` addition to `CMAKE_C_FLAGS` is
effectively lost because `CMAKE_C_FLAGS` is re-assigned unconditionally a few
lines later (line 155), overwriting the earlier append (line 153). If this
define is intended for all C/C++ compilation, add it to `CMAKE_CPP_FLAGS` (so
both C and CXX inherit it) or move the FreeBSD block after the final
`set(CMAKE_C_FLAGS ...)` assignment.
```suggestion
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DNO_PTHREAD_MUTEX_HOOK")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
