Skip to content

test: keep OOM tests compatible with newer compilers#138

Closed
yangzhg wants to merge 1 commit into
bytedance:masterfrom
yangzhg:gcc-compat
Closed

test: keep OOM tests compatible with newer compilers#138
yangzhg wants to merge 1 commit into
bytedance:masterfrom
yangzhg:gcc-compat

Conversation

@yangzhg
Copy link
Copy Markdown
Collaborator

@yangzhg yangzhg commented May 12, 2026

Summary

  • Avoid constant huge allocation sizes in OOM stack tests so GCC does not fail the test binary with -Walloc-size-larger-than before the test can exercise runtime OOM behavior.
  • Add headroom before padded lazy-parse test buffers and pass the shifted data pointer to avoid GCC 12/13 AVX2 inline -Warray-bounds diagnostics.
  • Mark DNode::MetaNode destruction as noinline to avoid GCC 12 incorrectly folding the object-size analysis through DOM teardown.

Why

This is a small compatibility prerequisite before the OOM split PRs. The current master fails locally under GCC 12/13 when compiling :unittest with warning-as-error, even before the oom-core changes can be validated.

Validation

  • bazel test :unittest --//:sonic_arch=haswell --//:sonic_dispatch=static --//:sonic_sanitizer=no --test_filter='*Allocator*:*WriteBuffer*' with GCC 13
  • CC=gcc-12 CXX=g++-12 bazel test :unittest --//:sonic_arch=haswell --//:sonic_dispatch=static --//:sonic_sanitizer=no --test_filter='*Allocator*:*WriteBuffer*'
  • CC=clang CXX=clang++ bazel test :unittest --//:sonic_arch=haswell --//:sonic_dispatch=static --//:sonic_sanitizer=no --test_filter='*Allocator*:*WriteBuffer*'
  • git diff --cached --check

Copilot AI review requested due to automatic review settings May 12, 2026 02:41
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.67%. Comparing base (92964a8) to head (213be6a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #138   +/-   ##
=======================================
  Coverage   69.67%   69.67%           
=======================================
  Files          27       27           
  Lines        3706     3706           
  Branches     1189     1189           
=======================================
  Hits         2582     2582           
  Misses        211      211           
  Partials      913      913           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates a few OOM-focused unit tests and a DOM teardown path to avoid GCC 12/13 warning diagnostics (treated as errors) that prevent the tests from building, while preserving the intent of exercising runtime OOM behavior.

Changes:

  • Add headroom before padded buffers used by ParseLazy OOM tests and pass the shifted data pointer to avoid GCC -Warray-bounds diagnostics.
  • Make the “huge allocation” size in stack OOM tests a runtime value to avoid GCC -Walloc-size-larger-than at compile time.
  • Mark DNode::MetaNode’s destructor as noinline to avoid GCC object-size analysis folding through DOM teardown.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/parser_oom_test.cpp Adds headroom + helper accessor so lazy-parse OOM tests use a shifted buffer pointer and avoid GCC bounds warnings.
tests/allocator_test.cpp Makes the huge allocation size runtime-derived to keep OOM stack tests buildable under GCC warning-as-error.
include/sonic/dom/dynamicnode.h Applies noinline to MetaNode destructor to prevent GCC mis-analysis during teardown.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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