Fix sign-compare warnings under xlog backend#620
Open
afrind wants to merge 4 commits into
Open
Conversation
Summary: folly's `Compression.cpp` `dynamic_cast`s `snappy::Source` / `snappy::Sink`, which requires their typeinfo symbols. The snappy revision pinned in our manifest (`27ab5f7f...`) hardcodes `-fno-rtti` in `CMAKE_CXX_FLAGS` and even strips any `-frtti` we try to inject, so `libsnappy.so` ships without typeinfo for those polymorphic bases. Any downstream that actually pulls in folly's Snappy codec (proxygen, mvfst, folly's own `compression_compression_test` which is gated behind `BUILD_SLOW_TESTS`) fails to link with: undefined reference to `typeinfo for snappy::Source' Override via `CMAKE_CXX_FLAGS_<CONFIG>`, which CMake appends after `CMAKE_CXX_FLAGS` and snappy doesn't touch — the trailing `-frtti` wins over the earlier `-fno-rtti` in gcc. Differential Revision: D106941114
Summary:
Introduces `proxygen/lib/utils/LogShim.h`, a header-only macro layer that lets downstream builds choose at compile time which backend services log emission:
- `PRX_LOGGING_GLOG=1` (default) — glog via `<folly/GLog.h>`. Behavior identical to today for files that previously used `LOG`/`CHECK`; behavior change for files that previously used `XLOG`/`XCHECK` (lose folly category-based runtime filtering and folly's formatter-based ostream).
- `PRX_LOGGING_XLOG=1` — folly::xlog via `<folly/logging/xlog.h>`. Routes through folly's category tree.
- `PRX_LOGGING_DISABLED=1` — fully stripped. `PRX_CHECK` -> `if (!e) abort()`, `PRX_DCHECK` -> `assert(e)`, log streams become a noop sink.
The pattern mirrors `fizz/util/Logging.h` and the wangle shim in D106528033. The default is glog because every existing wangle/fizz call site uses glog today; opting in to xlog or noop lets OSS users ship without a glog dependency. Proxygen had a mix of `LOG`/`CHECK` (older code) and `XLOG`/`XCHECK` (newer `lib/http/coro/`, `lib/transport/qmux/`, `lib/http/webtransport/`, `lib/http/sink/`); both families are routed through the unified `PRX_*` set so the backend choice applies everywhere.
All ~370 OSS call sites converted to the `PRX_*` macros via codemod. Scope is OSS dirs only — `proxygen/lib`, `proxygen/httpserver`, `proxygen/httpclient`, `proxygen/external`, `proxygen/fuzzers`, `proxygen/public_tld`. `proxygen/facebook/` is untouched except for one missing `#include <folly/logging/xlog.h>` in `HostResources.h` that was previously satisfied transitively.
The codemod also normalized xlog-vocab levels to glog vocab inside `PRX_*` calls so the GLOG backend resolves the tokens: `PRX_LOG(ERR)` -> `PRX_LOG(ERROR)`, `PRX_LOG(DBG<n>)` -> `PRX_VLOG(<n>)` (and the same for `*_IF`, `*_EVERY_MS`, `*_EVERY_N`).
A handful of hand fixes for behavioral differences between folly's xlog and glog's LOG:
- Replaced three `XLOG_FILENAME` use sites with `__FILE__` (xlog-only macro).
- Added `operator<<` for `HTTPStreamSource::ReadState` (inline friend), `WriteHandleState`, and `ReadHandleState` (in WtStreamManager.cpp anonymous namespace) so `PRX_CHECK_{EQ,NE}` of those enums compiles under glog's `MakeCheckOpString`.
- Switched `PRX_CHECK_NE(it, deque.end())` to `PRX_CHECK(it != deque.end())` since `std::_Deque_iterator` has no `operator<<`.
- Replaced five `PRX_LOG(FATAL) << "..."; return X;` in `HTTPSourceTests.cpp` lambdas with `ADD_FAILURE() << "..."; return X;` — glog's `LOG(FATAL)` is `[[noreturn]]` which made the trailing return dead code under `-Werror=unreachable-code-return`.
- Removed one dead `return ""` after `PRX_LOG(FATAL)` in a switch default in `HTTPCoroSessionTests.cpp`.
CMake/getdeps integration mirrors the BUCK story: top-level `proxygen/public_tld/CMakeLists.txt` exposes `PROXYGEN_LOGGING_BACKEND` (`GLOG`/`XLOG`/`DISABLED`, default `GLOG`) and conditionally `find_package`s glog, generates `<proxygen/proxygen-config.h>` from `proxygen/lib/utils/proxygen-config.h.in`, and narrows `proxygen_utils_log_shim`'s `INTERFACE_LINK_LIBRARIES` per backend. Per-subdir `CMakeLists.txt` regenerated by `proxygen/facebook/generate_cmake.py`. Manually-maintained CMake files (`httpserver/CMakeLists.txt`, `httpserver/samples/hq/CMakeLists.txt`, `httpserver/samples/hq/devious/CMakeLists.txt`) got `proxygen_utils_log_shim` added by hand.
Buck: new `:config` (header_namespace="" remap of `<proxygen/proxygen-config.h>` to the appropriate per-build variant in `lib/utils/facebook/`) and `:log_shim` `cpp_library` targets in `proxygen/lib/utils/TARGETS`. AUTODEPS added `:log_shim` to consumers; matching `:proxygen_config` and `:proxygen_lib_utils_log_shim` `fb_xplat_cxx_library` targets added to `xplat/proxygen/BUCK`, plus `:proxygen_lib_utils_log_shim` propagated to every `fb_xplat_cxx_library`/`fb_xplat_cxx_test` in that file.
`PRX_PCHECK` loses the `: <strerror(errno)>` suffix in xlog and disabled modes; TODO to fix once folly exposes `XPCHECK`.
Differential Revision: D106960643
Summary: `PRX_VLOG(n)` requires `n` to be a literal so the token-paste path (`PRX_LOGGING_DBG_##n` → `DBG0..DBG9`) hits XLOG's fast static-cached check. Callers that pass a runtime int (e.g. `PRX_VLOG(level)` where `level` is a parameter) can't use that form under the xlog backend — folly's `XLOG()` requires a compile-time `folly::LogLevel` (the level is used in a template non-type argument for `LogStreamVoidify<isLogLevelFatal(level)>`). Add `PRX_VLOG_LEVEL(n)` / `PRX_DVLOG_LEVEL(n)` for those callers. Under xlog the implementation routes through `FB_LOG_RAW` with a per-call-site cached `folly::Logger` (a static inside a non-capturing IIFE), so the category lookup happens once per call site rather than once per call. Under glog and disabled backends these are aliases for `VLOG`/`PRX_VLOG`. The compile-time `PRX_VLOG(literal_n)` path is unchanged — same fast XLOG static-cached check as before. No runtime cost added for the common case. Convert the three known runtime-`n` call sites: - `proxygen/lib/http/HTTPMessage.cpp` — `dumpMessage(int vlogLevel)` - `proxygen/lib/http/coro/filters/Logger.cpp` — `logWithVlog(int level)` - `proxygen/lib/http/codec/test/TestUtils.h` — `dumpCounters(int verbosity)` (27 sites) Differential Revision: D106960645
Summary: folly's `XCHECK_OP` (and the `XCHECK_GT`/`XCHECK_EQ`/... family) wraps the comparison in a lambda that expands inline at the call site. That makes the user's TU responsible for `-Wsign-compare`. glog's `CHECK_OP` puts the same comparison inside a function template defined in glog's headers, which are typically `-isystem`-included so the warning is suppressed for glog's own code — the legacy `CHECK_GT(unsigned_thing, 0)` pattern was always actually sign-mismatched, glog just hid it. Switching proxygen's `PRX_CHECK_OP` family to xlog under `proxygen.logging_backend=xlog` therefore surfaces ~100 latent mismatches. Each one is the same shape: an unsigned LHS (`size_t`, `uint32_t`, `uint64_t`, etc.) compared against a signed integer literal. Fix them all at the call site by giving the literal a `u` suffix (or, for `numeric_limits<int64_t>::max()`, cast to the LHS's `size_t`). Matches moxygen's xlog convention. Also marks two `inline operator<<` helpers in `WtStreamManager.cpp` as `[[maybe_unused]]`. glog's `CHECK_EQ` references them for failure-diff formatting; xlog's `XCHECK_EQ` does not, so `-Wunused-function` fires under xlog. The attribute keeps them available for glog without warning under xlog. Differential Revision: D106960646
|
@afrind has exported this pull request. If you are a Meta employee, you can view the originating Diff in D106960646. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
folly's
XCHECK_OP(and theXCHECK_GT/XCHECK_EQ/... family) wraps the comparison in a lambda that expands inline at the call site. That makes the user's TU responsible for-Wsign-compare. glog'sCHECK_OPputs the same comparison inside a function template defined in glog's headers, which are typically-isystem-included so the warning is suppressed for glog's own code — the legacyCHECK_GT(unsigned_thing, 0)pattern was always actually sign-mismatched, glog just hid it.Switching proxygen's
PRX_CHECK_OPfamily to xlog underproxygen.logging_backend=xlogtherefore surfaces ~100 latent mismatches. Each one is the same shape: an unsigned LHS (size_t,uint32_t,uint64_t, etc.) compared against a signed integer literal. Fix them all at the call site by giving the literal ausuffix (or, fornumeric_limits<int64_t>::max(), cast to the LHS'ssize_t). Matches moxygen's xlog convention.Also marks two
inline operator<<helpers inWtStreamManager.cppas[[maybe_unused]]. glog'sCHECK_EQreferences them for failure-diff formatting; xlog'sXCHECK_EQdoes not, so-Wunused-functionfires under xlog. The attribute keeps them available for glog without warning under xlog.Differential Revision: D106960646