diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd3f97520..a797f95d0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -958,6 +958,46 @@ jobs: echo "Running test_ops_matmul_elem with VT_CPU_MATMUL_TIER=${tier}" VT_CPU_MATMUL_TIER="${tier}" build/tests/test_ops_matmul_elem done + build-newest-gcc: + # COMPILE-ONLY coverage on a compiler NEWER than any other lane's. Every + # other Linux lane installs the distro `g++`, which on ubuntu-latest is + # gcc 13, so a header that only compiles because of a TRANSITIVE include + # goes green here and red on a current distro. That is not hypothetical: + # ::getpid without was fixed once and came back in five more + # files, one of them src/vllm/entrypoints/openai/server_main.cpp — a + # SHIPPED binary that does not build on gcc 16 (reported on issue #41). + # + # Deliberately does NOT run ctest: this lane exists to catch compile-time + # portability, and build-test-cpu already owns execution. Keeping it to a + # build is roughly one extra compile per PR. + concurrency: + group: ci-build-newest-gcc-${{ github.event_name }}-${{ github.ref }}-${{ github.repository }} + cancel-in-progress: ${{ github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }} + if: github.event.action != 'closed' + runs-on: ubuntu-latest + timeout-minutes: 90 + container: gcc:16 + steps: + - name: Install build tools + # The gcc image ships the toolchain only; git is needed BEFORE checkout + # so actions/checkout uses git rather than the slower tarball path. + run: | + apt-get update + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + ca-certificates cmake git ninja-build python3 + rm -rf /var/lib/apt/lists/* + - uses: actions/checkout@v4 + - name: Report the compiler this lane is actually pinning + run: g++ --version + - name: Configure + run: | + cmake -S . -B build-newest-gcc -G Ninja \ + -DCMAKE_BUILD_TYPE=Release \ + -DVLLM_CPP_BUILD_TESTS=ON + - name: Build + # Same bounded parallelism as build-test-cpu: a bare -j OOM-kills the + # runner during the parallel link of the test executables. + run: cmake --build build-newest-gcc -j 2 build-test-cpu-arm64: # Independent Arm execution evidence: the x86 lane cannot prove HWCAP # dispatch, Arm instructions, or the host ABI. The native runner exercises diff --git a/cmake/CompilerWarnings.cmake b/cmake/CompilerWarnings.cmake index 6a56933f6..0c7918306 100644 --- a/cmake/CompilerWarnings.cmake +++ b/cmake/CompilerWarnings.cmake @@ -15,12 +15,37 @@ function(vllm_cpp_set_warnings target) if(NOT VLLM_CPP_SANITIZE STREQUAL "OFF") set(_vllm_cpp_werror "") endif() + + # GCC >= 16 reports -Warray-bounds inside LIBSTDC++ and the vendored nlohmann + # json for code that is correct, so the diagnostic stays VISIBLE but stops + # being fatal on those compilers only. Everything <= 15 is unchanged and still + # fails the build on a real out-of-bounds. + # + # It is the same false-positive class this file already documents above for + # the sanitizer lanes, and it is not something the calling code can avoid: + # `_Sp_counted_base::_M_release()` is identical machine code for every + # shared_ptr type, so after inlining GCC attributes ONE instantiation's + # destructor to ANOTHER instantiation's allocation size. On gcc 16.1.1 that + # presents as "array subscript 'std::mutex[0]' is partly outside array bounds + # of 'unsigned char [32]'" pointing at a json.dump() call, in a translation + # unit that contains no shared_ptr at all. + # + # libstdc++ carries its own `#pragma GCC diagnostic` suppressions around that + # very destructor (bits/shared_ptr_base.h), i.e. the standard library treats + # this as a warning to silence rather than a bug to fix in user code. Upstream + # is GCC PR tree-optimization/122197; Eigen, assimp and CMSSW all disable the + # check the same way on the affected releases. + set(_vllm_cpp_array_bounds "") + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND + CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 16) + set(_vllm_cpp_array_bounds -Wno-error=array-bounds) + endif() if(MSVC) target_compile_options(${target} PRIVATE $<$:/W4 /WX>) else() target_compile_options(${target} PRIVATE - $<$:-Wall -Wextra ${_vllm_cpp_werror}> + $<$:-Wall -Wextra ${_vllm_cpp_werror} ${_vllm_cpp_array_bounds}> # OBJCXX (.mm — the Metal backend) is a SEPARATE COMPILE_LANGUAGE from CXX, # so the CXX genex above does not reach it. Without this line the Metal TUs # would be the only unwarned code in the tree (BACKEND-METAL-MLX W0). diff --git a/docs/USAGE.md b/docs/USAGE.md index 4838477f8..e9d4972ef 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -22,6 +22,22 @@ example targets are named after the directories they are built from, so an in-source build makes the linker write each executable over its own source directory (issue #85). +### Host compilers + +gcc 13 and 14 and clang are exercised by CI, and **gcc 16 builds the tree, +including the OpenAI server**. Before this it did not: several files, one of +them the server's own `main`, called `getpid()` without including `` +and compiled only because an older libstdc++ happened to pull that header in +for them. A compile-only CI lane on the newest released gcc now guards this, +because every other Linux lane uses the distro compiler and cannot see it. + +On gcc 16 the `array-bounds` warning is reported but is **not** treated as an +error, unlike on every earlier gcc. That release emits it inside libstdc++ and +the vendored JSON library for code that is correct, and no change to the +calling code avoids it (`cmake/CompilerWarnings.cmake` explains the mechanism +and cites the upstream gcc bug). A genuine out-of-bounds still fails the build +on gcc 15 and earlier, which is what the rest of CI enforces. + ### Setting the compiled build identity `vllm-server --version` reports the CMake project version by default. Release diff --git a/examples/ltx2_gen/main.cpp b/examples/ltx2_gen/main.cpp index b75b70df1..b447798e5 100644 --- a/examples/ltx2_gen/main.cpp +++ b/examples/ltx2_gen/main.cpp @@ -425,7 +425,17 @@ int main(int argc, char** argv) { // Each retake knob rides the SAME per-generation array. Supplying one without // the window is refused by the engine rather than ignored, so a half-typed // retake reports what is missing instead of rendering the ordinary path. - for (const auto& kv : {std::make_pair("retake_start_time", &retake_start), + // The knobs are a NAMED array rather than a braced-init-list iterated in + // place. Both are correct -- a braced-init-list bound to the range variable + // has its backing array lifetime-extended for the whole loop -- but the + // in-place form draws -Wdangling-reference from gcc 16, which + // `build-newest-gcc` found on its first run. The warning is a false positive + // and it is not silenced: the range now has automatic storage and a name, so + // no reference binds to a temporary and the question does not arise. Making + // the loop variable a copy does NOT help; the diagnostic is about the range + // reference, not the element. + const std::pair retake_knobs[] = { + std::make_pair("retake_start_time", &retake_start), std::make_pair("retake_end_time", &retake_end), std::make_pair("retake_frame_rate", &retake_fps), std::make_pair("regenerate_video", ®en_video), @@ -452,7 +462,8 @@ int main(int argc, char** argv) { std::make_pair("video_skip_step", &video_skip_step), std::make_pair("video_stg_blocks", &video_stg_blocks), std::make_pair("a2v_guidance_scale", &a2v_scale), - std::make_pair("v2a_guidance_scale", &v2a_scale)}) { + std::make_pair("v2a_guidance_scale", &v2a_scale)}; + for (const auto& kv : retake_knobs) { if (kv.second->empty()) continue; gen_keys.emplace_back(kv.first); gen_values.push_back(*kv.second); diff --git a/src/vllm/entrypoints/openai/server_main.cpp b/src/vllm/entrypoints/openai/server_main.cpp index 291e6bb21..550e0bab1 100644 --- a/src/vllm/entrypoints/openai/server_main.cpp +++ b/src/vllm/entrypoints/openai/server_main.cpp @@ -57,6 +57,7 @@ #include #include #include +#include // ::getpid below; guarded because MSVC has no such header #include #endif diff --git a/src/vllm/model_executor/models/qwen3_5.cpp b/src/vllm/model_executor/models/qwen3_5.cpp index f24db5807..bc4702d21 100644 --- a/src/vllm/model_executor/models/qwen3_5.cpp +++ b/src/vllm/model_executor/models/qwen3_5.cpp @@ -1,5 +1,6 @@ #if defined(__unix__) #include +#include // ::sysconf(_SC_PAGESIZE) in the readahead hint below #endif // vllm.cpp original; see qwen3_5.h. Forward math mirrored 1:1 from the pinned // upstream (qwen3_next.py::Qwen3NextDecoderLayer / Qwen3NextModel.forward, diff --git a/tests/support/process_id.h b/tests/support/process_id.h new file mode 100644 index 000000000..03488b411 --- /dev/null +++ b/tests/support/process_id.h @@ -0,0 +1,47 @@ +// vllm.cpp original (test harness); no upstream mirror. +// +// ONE portable process id for the whole test tree. +// +// WHY IT EXISTS. Tests name their temporary directories after the running +// process so two concurrent runs cannot collide. `::getpid()` is POSIX, and its +// declaration lives in ``, which MSVC does not ship at all. So the +// obvious spelling does not fail on Windows — it does not COMPILE, and it takes +// both `windows-msvc-*` lanes down with it. Exactly the shape of issue #603, +// which is why `tests/support/test_env.h` next door exists. +// +// It also fails on a CURRENT POSIX toolchain for the opposite reason. Several +// files called `::getpid()` while including nothing that declares it, and +// compiled anyway because an older libstdc++ pulled `` in for them. +// gcc 16 does not: +// +// error: '::getpid' has not been declared; did you mean 'getpt'? +// +// That was fixed once in three files and came back in five more, because each +// new loader test copies the temp-directory helper from the last one. A per-file +// `#ifdef` would be copied just as faithfully and get it wrong again, so the +// portable spelling lands ONCE, here, and new tests include it. +#pragma once + +#if defined(_WIN32) +#include +#else +#include +#endif + +namespace vllm_test { + +// The current process id, for building a name no other process will pick. +// +// NOT for anything but naming. The two platforms agree that this is a unique +// live-process identifier and agree on nothing else about it, so it is used for +// uniqueness only, never compared against a recorded value or reused after the +// process exits. +inline int ProcessId() { +#if defined(_WIN32) + return ::_getpid(); +#else + return static_cast(::getpid()); +#endif +} + +} // namespace vllm_test diff --git a/tests/vllm/models/test_indextts2_s2mel_loader.cpp b/tests/vllm/models/test_indextts2_s2mel_loader.cpp index 43ec72a4d..5df4d05cb 100644 --- a/tests/vllm/models/test_indextts2_s2mel_loader.cpp +++ b/tests/vllm/models/test_indextts2_s2mel_loader.cpp @@ -20,6 +20,7 @@ #include "vllm/model_executor/models/indextts2_config.h" #include "vllm/model_executor/models/indextts2_s2mel_loader.h" +#include "support/process_id.h" namespace { std::string U64Le(uint64_t v) { @@ -117,7 +118,7 @@ std::string BuildTail(int64_t hidden = 8, int64_t in_ch = 4, int64_t wn = 8, std::string WriteTemp(const std::string& bytes, const std::string& tag) { const std::filesystem::path path = std::filesystem::temp_directory_path() / - ("indextts2_s2mel_" + tag + "_" + std::to_string(::getpid()) + ".safetensors"); + ("indextts2_s2mel_" + tag + "_" + std::to_string(vllm_test::ProcessId()) + ".safetensors"); std::ofstream out(path, std::ios::binary); out.write(bytes.data(), static_cast(bytes.size())); out.close(); diff --git a/tests/vllm/models/test_indextts2_talker_loader.cpp b/tests/vllm/models/test_indextts2_talker_loader.cpp index e8920c124..bbdf56fa7 100644 --- a/tests/vllm/models/test_indextts2_talker_loader.cpp +++ b/tests/vllm/models/test_indextts2_talker_loader.cpp @@ -16,6 +16,7 @@ #include "vllm/model_executor/models/indextts2_config.h" #include "vllm/model_executor/models/indextts2_talker_loader.h" +#include "support/process_id.h" namespace { std::string U64Le(uint64_t v) { @@ -79,7 +80,7 @@ std::string BuildTalker(int64_t H = 8, int64_t L = 2, int64_t I = 16, int64_t TV std::string WriteTemp(const std::string& bytes, const std::string& tag) { const std::filesystem::path p = std::filesystem::temp_directory_path() / - ("indextts2_talker_" + tag + "_" + std::to_string(::getpid()) + ".safetensors"); + ("indextts2_talker_" + tag + "_" + std::to_string(vllm_test::ProcessId()) + ".safetensors"); std::ofstream out(p, std::ios::binary); out.write(bytes.data(), static_cast(bytes.size())); return p.string(); diff --git a/tests/vllm/models/test_ltx2_text_encoder.cpp b/tests/vllm/models/test_ltx2_text_encoder.cpp index b36f0b0f4..9ce58a802 100644 --- a/tests/vllm/models/test_ltx2_text_encoder.cpp +++ b/tests/vllm/models/test_ltx2_text_encoder.cpp @@ -37,6 +37,7 @@ #include "doctest/doctest.h" #include "support/max_abs_diff.h" +#include "support/process_id.h" #include "vllm/model_executor/model_loader/safetensors_reader.h" #include "vllm/model_executor/models/gemma4.h" #include "vllm/model_executor/models/ltx2_loader.h" @@ -1457,7 +1458,7 @@ TEST_CASE("ltx2 text: a non-f32 compute dtype is REFUSED, never silently widened TEST_CASE("ltx2 text: the tokenizer and HF sidecars come out of TENSORS, not files") { const fs::path dir = - fs::temp_directory_path() / ("ltx2_text_assets_" + std::to_string(::getpid())); + fs::temp_directory_path() / ("ltx2_text_assets_" + std::to_string(vllm_test::ProcessId())); fs::create_directories(dir); const std::string tokenizer = R"({"version":"1.0","model":{"type":"BPE"}})"; diff --git a/tests/vllm/models/test_minimax_music3_loader.cpp b/tests/vllm/models/test_minimax_music3_loader.cpp index 847ebca41..2afaef2d6 100644 --- a/tests/vllm/models/test_minimax_music3_loader.cpp +++ b/tests/vllm/models/test_minimax_music3_loader.cpp @@ -43,6 +43,7 @@ #include "vllm/model_executor/models/minimax_music3_loader.h" #include "vllm/model_executor/models/vocoder1d.h" +#include "support/process_id.h" using vllm::MiniMaxMusic3AccountReport; using vllm::MiniMaxMusic3AccountTensors; using vllm::MiniMaxMusic3Config; @@ -164,7 +165,7 @@ std::vector EntriesFor(const std::vector& spec std::string TempPath(const char* stem) { return (std::filesystem::temp_directory_path() / - (std::string("music3_") + stem + "_" + std::to_string(::getpid()) + ".safetensors")) + (std::string("music3_") + stem + "_" + std::to_string(vllm_test::ProcessId()) + ".safetensors")) .string(); } @@ -733,7 +734,7 @@ TEST_CASE("music3 load: a component whose file lost a tensor throws BY NAME") { TEST_CASE("music3 resolve: a NATIVE-arm checkpoint is refused, naming what is missing") { const std::filesystem::path root = std::filesystem::temp_directory_path() / - ("music3_native_" + std::to_string(::getpid())); + ("music3_native_" + std::to_string(vllm_test::ProcessId())); std::filesystem::remove_all(root); // The layout convert_minimax_music3_to_diffusers.py:30,34,38 reads, and the // one sglang_omni/models/minimax_music3/checkpoint.py:35-56 serves. @@ -776,7 +777,7 @@ TEST_CASE("music3 resolve: ONE native marker is enough to be diagnosed as the na for (const char* marker : {"flowmatching_vae.pth", "dav.pth", "qwen_7B"}) { const std::filesystem::path root = std::filesystem::temp_directory_path() / - ("music3_partial_" + std::string(marker) + "_" + std::to_string(::getpid())); + ("music3_partial_" + std::string(marker) + "_" + std::to_string(vllm_test::ProcessId())); std::filesystem::remove_all(root); std::filesystem::create_directories(root); if (std::string(marker) == "qwen_7B") { @@ -804,7 +805,7 @@ TEST_CASE("music3 resolve: ONE native marker is enough to be diagnosed as the na TEST_CASE("music3 resolve: a tree that is NEITHER arm names the components it lacks") { const std::filesystem::path root = - std::filesystem::temp_directory_path() / ("music3_empty_" + std::to_string(::getpid())); + std::filesystem::temp_directory_path() / ("music3_empty_" + std::to_string(vllm_test::ProcessId())); std::filesystem::remove_all(root); std::filesystem::create_directories(root); CHECK(!vllm::MiniMaxMusic3IsNativeArm(root.string()));