Skip to content

Add autotest option to run unit tests under asan / run them in CI / fix bugs foud - #33976

Merged
tridge merged 20 commits into
ArduPilot:masterfrom
peterbarker:pr-claude/ci-unit-tests-asan
Oct 17, 2026
Merged

tridge merged 20 commits into
ArduPilot:masterfrom
peterbarker:pr-claude/ci-unit-tests-asan

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

Fixes bugs found by running the unit tests under asan

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Description

Adds infrastructure to run unit tests under asan and fixes bugs found...

@github-actions github-actions Bot added CI github_actions Pull requests that update GitHub Actions code Python labels Aug 7, 2026

@khancyr khancyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like we don't have libclang-rt-18-dev on the container ...
do you prefer I update the container and we keep the same tag, or I create another tag and we change the tag version on the CI workflow ?

Comment thread .github/workflows/test_unit_tests.yml Outdated
examples,
sitl
]
exclude:
# boards.py only adds the sanitizer link flag when building

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we add support for asan on gcc ? from the docs it should be supported

@peterbarker

Copy link
Copy Markdown
Contributor Author

looks like we don't have libclang-rt-18-dev on the container ... do you prefer I update the container and we keep the same tag, or I create another tag and we change the tag version on the CI workflow ?

Maybe we should just take the test fixes for now?

I mean, 4 errors in all of the tests over how many years - and the errors were only in the tests.

So I think I should just archive the asan thing for later....

@khancyr

khancyr commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

well, did you try to run on some SITL test ? our unittests are far from covering significant part of the codebase.

@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch 2 times, most recently from 09e810a to 3e4ac4a Compare August 20, 2026 06:31
@peterbarker

Copy link
Copy Markdown
Contributor Author

well, did you try to run on some SITL test ? our unittests are far from covering significant part of the codebase.

Somewhere here I think I have more stuff. We do have this:

pbarker@crun:~/rc/ardupilot-claude(pr-claude/ci-unit-tests-asan)$ ./Tools/autotest/autotest.py --help | grep asan
    --asan              enable ASAN error checking (binary must be built with
                        --asan --debug)
pbarker@crun:~/rc/ardupilot-claude(pr-claude/ci-unit-tests-asan)$ 

@peterbarker

Copy link
Copy Markdown
Contributor Author

looks like we don't have libclang-rt-18-dev on the container ... do you prefer I update the container and we keep the same tag, or I create another tag and we change the tag version on the CI workflow ?

I'm kind of tempted to just apt-install it. Thoughts?

@khancyr

khancyr commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

libclang-rt-18-dev is 3 772 ko, so should be ok.
I will add it on container so it will be in next time we update them

@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch 2 times, most recently from 5165a2f to 16d2c10 Compare September 2, 2026 09:12
@peterbarker

Copy link
Copy Markdown
Contributor Author
  Copter final: 370 passed, 0 ASAN logs, 0 new suppressions, 8 ASAN-timing
  failures that pass without the sanitizer.

  Complete verification across the branch:

  ┌───────────────────────────────────┬───────┬──────────┬─────────────────┐
  │               suite               │ tests │  ASAN    │      new        │
  │                                   │       │   logs   │  suppressions   │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ unit tests (gcc + clang)          │ 67/67 │ —        │ 1               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ AntennaTracker                    │ 20    │ 0        │ 3               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ Blimp                             │ 5     │ 0        │ 0               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ ArduSub                           │ 56    │ 0        │ 1               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ Rover                             │ 125   │ 0        │ 0               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ ArduPlane                         │ 205   │ 0        │ 0               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ ArduCopter                        │ 370   │ 0        │ 0               │
  ├───────────────────────────────────┼───────┼──────────┼─────────────────┤
  │ networking/PPP (the 5 that        │ 5     │ 0        │ 0               │
  │ failed)                           │       │          │                 │
  └───────────────────────────────────┴───────┴──────────┴─────────────────┘

@github-actions github-actions Bot added the EKF label Sep 3, 2026
@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch from 16d2c10 to 632ace1 Compare September 22, 2026 04:55
@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Global Size Summary (Total Flash delta in bytes)

Built from b22e283 rebased onto 8ab6afd

Board Periph tracker blimp bootloader copter heli iofw plane rover sub
DrotekP3Pro _ _ _ * _ _ _ _ _ _
Durandal _ * * _ * * _ * * *
Hitec-Airspeed * _ _ _ _ _ _ _ _ _
KakuteF7 _ * * _ * * _ * * *
MambaH743v4 _ * * _ * * _ * * *
MatekF405-CAN _ _ _ * _ _ _ _ _ _
MatekF405 _ * * _ * * _ * * *
MatekH743-bdshot _ * * _ * * _ * * *
Pixhawk1-1M _ * * _ * * _ * * *
disco _ * * _ * * _ * * *
f103-GPS * _ _ _ _ _ _ _ _ _

@AP-Review

AP-Review commented Sep 22, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-22)

Automated review note — AI-generated (Claude), cross-checked by a second independent Claude pass against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_22_AIReview/devcall_pr_reviews.html#pr33976

Reviewed at head 632ace159c. First AI review on this PR. Verdict: REQUEST CHANGES — reached independently by both passes, and every finding below was confirmed by the second rather than adjusted away. The design is sound and the four test fixes are genuine bugs, but three of the four new CI jobs are red at this head, the fourth will still fail once it builds, and there is one coverage gap neither of us spotted first time round (section 6).

The good news first, since it is the load-bearing question for a sanitizer job: a real asan error does fail both new jobs, and both passes tested it independently on two toolchains. I injected a leaked new char[123] into a unit test, rebuilt under --asan and ran it with exactly the environment run_unit_test() builds: SUMMARY: AddressSanitizer: 123 byte(s) leaked in 1 allocation(s), exit status 1, so check_call raises, run_unit_tests() returns False and autotest reports a FAILED STEP. Reports go to stderr with no log_path, so they land in the CI log. The examples collection path works too — restoring AP_Declination_test.cpp to its master version and running it through examples.run_example(asan=True) produced a collected global-buffer-overflow … READ of size 2 … get_declination AP_Declination_test.cpp:69, and the fixed version produced none. So the bug this PR fixes is real and the harness sees it.

Also worth saying: khancyr's "should we add support for asan on gcc?" is answered well. Dropping the forced clang is a real coverage win, and your comment about it is accurate — AP_Networking_Config.h:11-17 does disable AP_NETWORKING_ENABLED for clang on non-Apple, so choosing the compiler was silently compiling all the networking code out of every sanitizer build.

1. BUG — examples-asan fails on a clean run, because the examples ASAN options lack print_suppressions=0 (Tools/autotest/examples.py:47)

run_example() sets log_path=%s:symbolize=1:verbosity=0 and run_examples() treats any non-empty log as a failure — but LSan writes its suppression summary into that same log. Full local run of autotest.py --asan --no-clean run.examples at this head: 36 examples ran, autotest exited 1, with

ASAN failure in AP_Compass_test:
-----------------------------------------------------
Suppressions used:
  count      bytes template
      5       5000 Compass::_detect_backends
-----------------------------------------------------
ASAN failure in GPS_AUTO_test:
   ... 35  88592 create_serial_sim / 7  280 SITL::GPS::check_backend_allocation ...
Failed examples:
    AP_Compass_test
    GPS_AUTO_test
>>>> FAILED STEP: run.examples

Both logs (201 and 248 bytes) contain nothing but the summary of the suppressions this PR itself adds — no error. This is the exact failure mode commit 37c99b31d3 fixed for SITL: Tools/autotest/pysim/util.py:793 already carries print_suppressions=0 and examples.py:47 does not. The machinery is demonstrably live in the CI container too (the gcc unit-tests-asan job log prints Suppressions used: 3 69 EKF_Buffer.cpp), so this reproduces there the moment the build is fixed.

Scope correction from the second pass: two examples fail, not one — AP_Compass_test and GPS_AUTO_test, both reproduced end to end through the PR's own entry point. Fix: add print_suppressions=0 to our_opts in examples.py. Worth hardening the check while you are there — "non-empty file means failure" makes any future benign runtime chatter a red job; matching SUMMARY: AddressSanitizer in the log is both stricter and safer. Phrase it that way and not as ERROR: AddressSanitizer — a leak report says ERROR: **Leak**Sanitizer, so an ERROR: AddressSanitizer-only grep would silently disable leak detection for every example; leak reports do end SUMMARY: AddressSanitizer: N byte(s) leaked, so the SUMMARY form catches both. Verified alongside: with print_suppressions=0 in place the false-positive logs are not written at all, while a real overflow log is 2872 bytes and a real leak log 781 bytes either way with full stacks retained — so the fix hides nothing.

2. BUG — examples-asan cannot build with gcc (RCProtocolTest.cpp:333)

The example carries its own #pragma GCC diagnostic error "-Wframe-larger-than=2000", guarded only for clang, and asan's redzones push the frame past it:

libraries/AP_RCProtocol/examples/RCProtocolTest/RCProtocolTest.cpp:546:1: error: the frame size of 3040 bytes is larger than 2000 bytes [-Werror=frame-larger-than=]

That is the build (base, examples-asan) failure, and I reproduced the same 3040 bytes locally with gcc 15.2. Suggested fix at :330-334:

#if defined(__clang_major__) || defined(__SANITIZE_ADDRESS__)
// clang doesn't understand -Wframe-larger-than=; asan inflates frames past it
#else
#pragma GCC diagnostic error "-Wframe-larger-than=2000"
#endif

Checked specifically, because it would matter if the two arms overlapped: __SANITIZE_ADDRESS__ does not exist under clang (clang defines only __has_feature(address_sanitizer)), so the added term is not redundant with the existing clang guard — it does exactly and only the new work. Confirmed as the real CI cause too, from the job log of run 35688702096.

3. BUG — the sanitizer displaces AP's operator new, and malloc_fill_byte=0 is what hides it (Tools/autotest/autotest.py:138)

Measured with nm -C on the same target at the same head:

  • non-asan build: build/sitl/tests/test_cpp has T operator new(unsigned long) — AP_Common/c++.cpp's calloc-based allocator is linked in;
  • --asan build: the same binary has U operator new(unsigned long) — satisfied at runtime by libasan/libstdc++, because the sanitizer runtime precedes the AP archive on the link line, so the c++.cpp archive member is never pulled.

16 of the 70 built sitl test binaries are in this state (test_ap_common, test_averagefilter, test_better_stream, test_calculate_mah, test_cpp, test_csvreader, test_dsm, test_float16, test_getopt, test_own_ptr, test_replace, test_ringbuffer, test_sorting, test_sparse_endian, test_time, test_tsindex). test_cpp is the worst case because it is the test for that allocator — libraries/AP_Common/tests/test_cpp.cpp:20-24 asserts that raw ::operator new returns zeroed memory. Under asan without the new option it fails ([ FAILED ] AP_Common.TEST_CPP, exit 1); with malloc_fill_byte=0 it passes, while exercising the sanitizer's allocator rather than ArduPilot's. So part of the asan job's green is bought by making a test pass without the code under test.

Preferable fix: force the AP allocator into the link for sanitizer builds — e.g. -Wl,-u,_Znwm in the asan LINKFLAGS next to boards.py:891, or naming c++.cpp in the test link — after which malloc_fill_byte=0 is not needed at all. If the option stays, note it is also incomplete: asan's max_malloc_fill_size defaults to 4096 (confirmed via ASAN_OPTIONS=help=1, and measured — with malloc_fill_byte=190 an 8192-byte new comes back with exactly 4096 filled bytes and 4096 untouched), so the emulation stops at 4 KB. I checked the sources of those 16 tests and none currently allocates more than 4 KB, so that part is latent rather than live; max_malloc_fill_size=<large> would close it.

4. ISSUE — both clang asan jobs fail, in a way that hides the cause (test_unit_tests.yml:129, :131)

In clang/unit-tests-asan the --board linux configure is clean (header byteswap.h: yes, HAVE_MEMRCHR: yes, …) but the sitl configure — the one that gets --asan — fails every check: HAVE_CMATH_ISFINITE: no, header endian.h: not found, header byteswap.h: not found, HAVE_MEMRCHR: no. waf then silently falls back to libraries/AP_Common/missing/byteswap.h, which collides with glibc: fatal error: redefinition of '__bswap_16'. The same thing kills clang/examples-asan. gcc+asan passes all those checks and clang-without-asan passes them, so the trigger is clang+asan failing to link the configure probes — almost certainly the clang asan runtime still being absent from ardupilot/ardupilot-dev-clang:v0.2.0, which is what khancyr raised on 2026-08-07 ("we don't have libclang-rt-18-dev on the container") and said he would add next time the images were updated. Until that image is rebuilt and the tag bumped here, exclude: the clang × *-asan matrix combinations, or pin those two configs to the base toolchain.

Two secondary points, both worth having:

  • A failed sanitizer configure check degrades the build rather than failing it, and this is the actionable half in this tree even though the container fix is not. All four checks are mandatory=False, and nothing anywhere asserts that --asan produced a working sanitizer. A mandatory=True check for __asan_init under if cfg.options.asan would turn a 25-minute mystery build failure into an immediate configure error.
  • The failure artifact uploads /tmp/buildlogs, which does not contain build/sitl/config.log, so the actual link error is in nothing CI keeps.

5. ISSUE — the leak:SITL_State::_parse_command_line suppression is much broader than its comment

The comment says "paths taken from the command line are kept for the life of the process", i.e. the two strdup()s at libraries/AP_HAL_SITL/SITL_cmdline.cpp:500 and :503. But LSan matches the template against every frame of the allocation stack, and SITL_State::_parse_command_line spans SITL_cmdline.cpp:238-736 — 499 lines inclusive — constructing the SITL vehicle model (sitl_model = model_constructors[i].constructor(…), :662) and calling _sitl_setup() (:735). So every allocation anywhere under SITL model construction and SITL setup is now unchecked, permanently — which contradicts the file's own "keep the list as tight as possible". Tighter alternative: #include <sanitizer/lsan_interface.h> and __lsan_ignore_object(autotest_dir) / __lsan_ignore_object(defaults_path) at the two strdups — exactly what is intended, and it cannot grow. leak:Compass::_detect_backends has the same shape (each backend's whole constructor tree), just smaller.

6. ISSUE — -fsanitize=address never reaches CFLAGS, so 305 C objects are uninstrumented (Tools/ardupilotwaf/boards.py:873)

This one neither pass spotted first time and it is the easiest fix here. :873-876 appends -fsanitize=address and -fno-omit-frame-pointer to env.CXXFLAGS only. Read back from the configured build's build/c4che/sitl_cache.py:

CFLAGS    -> fsanitize=address present: False
CXXFLAGS  -> fsanitize=address present: True
LINKFLAGS -> fsanitize=address present: True

and that build compiles 305 .c objects. Heap errors on allocations made by C code are still caught, because asan interposes malloc at runtime — but memory accesses inside those 305 objects are not instrumented, so an overflow in a C source is invisible. In a PR whose entire purpose is sanitizer coverage that is worth closing: one line, env.CFLAGS += [...] alongside :873.

7. NOTE — not every example gets a leak check, and the run is silent about it

examples.py relies on the example exiting normally for LSan's atexit check to run. Most SITL examples do handle SIGTERM and exit 0 — which is exactly why the suppression summaries in section 1 appear at all. But some ignore it and are SIGKILLed: in a full local run AC_PID_test gave retcode: None → retcode2: -9 (examples.py:88-92). Those examples get no leak check, and nothing reports the gap. Pre-existing in shape, but this PR is what makes it matter.

Notes

  • CI cost. unit-tests-asan (gcc) took 24m56s against 8m57s for unit-tests; part is the cold ccache for the new key, but the run step alone was 8m23s against 5m37s. A good slice is duplicate work: run_unit_tests() runs both boards, so the asan job runs the 66 uninstrumented linux binaries as well as the 69 instrumented sitl ones. The linux NotchFilterTest alone is 156 s in both jobs; the sitl one goes 158 s → 317 s under asan. Restricting the asan target to the sitl board would save roughly three minutes per asan job with no loss of coverage. Four new matrix entries is also four new ccache keys against the repo's cache budget.
  • The new declination unit test would not have caught the bug it ships with. It exercises AP_Declination::get_mag_field_ef()/get_declination(), which already clamp their indexes and were not buggy — and to be precise about where, since citing the wrong line invites you to look and conclude I misread: the pole and antimeridian cases are handled at AP_Declination.cpp:54-57 and :64-67, while the constrain_int32 at :70-71 covers genuinely out-of-range input — the out-of-bounds read asan found was in the example's private copy of the table and lookup, which the new test does not touch. It is still a fair regression guard for the library and adds pole/antimeridian coverage test_magfield.cpp lacks. The example is a poor guard in its own right: running the fixed binary for 60 s produced 449 result lines of which 446 are FAIL and not one is a Pass (the output is 16 KB-capped, so that is the visible window rather than the whole sweep), because its private 5-degree table does not agree with the library's 10-degree one under exact float comparison. (Pre-existing, since the clamp is a no-op away from the edges.) Having the example call the library instead of duplicating the table and the lookup would be worth more than either change. Two things in fairness, though. The new test is a real regression guard for the analogous library edges — it locks in a clamp that has been there since 2022 — and the thing that would have caught the example bug is the other half of this same PR: examples-asan, since the example reaches lat == 90 well inside its 10-second window. And the example fix is pure hygiene with zero behavioural change: the master-built and PR-built example outputs are byte-identical (md5 11460a70312c171f973131da621d2545), because at the edges the interpolation weight is exactly 0 before the fix and 1 after, so the out-of-bounds garbage was always multiplied by zero.
  • The test_prescaler.cpp off-by-one fix strengthens the test rather than weakening it — with test_index++ in the middle, the DSHOT/DSHOT_S sub-assertions were reading the next row, and one past the end of test_results on the last case, which is what asan saw. Similarly the test_expandingstring changes are right (the explicit ~ExpandingString() followed by a read was a genuine use-after-free, and terminating long_string fixes a real over-read). One leftover: the surviving EXPECT_STRNE("Test\n", …) is now tautological in both files — in the first the string is "Test\nTest2\n" at that point, and in the second get_string() is nullptr and has already been asserted as such two lines above. Either drop them or assert the content.
  • asan_log_filepath() returns a bare filename, so the example reports land in whatever the process cwd is (the repo root in CI) and are not removed once collected — my run left AP_Compass_test-None-asan.<pid> and GPS_AUTO_test-None-asan.<pid> in the checkout. Collection is self-consistent, but they are untracked droppings in the source tree.
  • DEBUGINFOD_URLS="" is a real fix rather than superstition — llvm-symbolizer blocks for ~90 s a frame fetching debug info when that is set, and it is set by default on Ubuntu. Sixteen commits, one subsystem each with the right prefixes; the three autotest files are flake8-clean; and build.examples correctly forces asan=False for the three non-sitl boards.

Reviewed by two independent Claude passes. Codex was unavailable this run (usage limit until 2026-09-27), so the second pass was a fresh-context Claude agent rather than a different vendor.

@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch 2 times, most recently from 94d5d1d to 56921c0 Compare September 24, 2026 02:41
@AP-Review

AP-Review commented Sep 24, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Re-reviewed at head 56921c0b50; my earlier comment above is superseded. The verdict moves from REQUEST CHANGES to COMMENT — you pushed three commits while this review was running and they fix seven of the ten findings, including every blocking one. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_24_AIReview/devcall_pr_reviews.html#pr33976

Resolved by that push

  1. Tools/autotest/examples.py:58 now carries print_suppressions=0, so a suppression-only run no longer writes a log that examples.py:27 reads as a failure.
  2. c2957fa18f adds an #elif defined(__SANITIZE_ADDRESS__) arm at RCProtocolTest.cpp:330-333, so the gcc examples-asan build no longer dies on the 3040-byte frame.
  3. malloc_fill_byte is gone from Tools/autotest/ entirely.
  4. Tools/scripts/build_ci.sh:94 adds install_asan_runtime(), which installs libclang-rt-<v>-dev and then probes with a two-line link — so a missing runtime now fails readably instead of surfacing thousands of steps later as the byteswap.h redefinition. That was the root cause I could only infer last round.
  5. The over-broad leak:SITL_State::_parse_command_line suppression is removed and replaced with two targeted LSAN_IGNORE_OBJECT() calls on the retained strdup'd paths (libraries/AP_HAL_SITL/SITL_cmdline.cpp:501,504). That is the right fix rather than a wider mask.
  6. Tools/ardupilotwaf/boards.py now appends -fsanitize=address to env.CFLAGS as well as env.CXXFLAGS, so C translation units are instrumented.
  7. boards.py:908 adds -Wl,-u,__lsan_default_suppressions, so the suppression list can no longer vanish silently.

A correction to my own previous advice on that last one. I suggested either -Wl,-u,_Znwm or -Wl,-u,__lsan_default_suppressions. The Codex cross-check built a controlled archive probe and showed -u,_Znwm does not actually fix extraction, while -u,__lsan_default_suppressions pulls in both the suppression function and ArduPilot's allocator. You picked the one that works; my alternative was wrong.

Still open (all minor)

  • Tools/autotest/pysim/util.py:148-149 — if os.environ.get('CXX') or os.environ.get('CC'): configure_env = dict(os.environ) sets configure_env to exactly what the child inherits anyway, since run_cmd passes env= straight to subprocess and env=None already means "inherit os.environ". Redundant rather than dead. The explanatory comment above it about AP_Networking_Config.h is worth keeping either way.
  • Tools/autotest/autotest.py:117,143 — unit-tests-asan still iterates ['linux', 'sitl'], and --asan is handled only by SITLBoard, so the linux half is uninstrumented duplicate work. build_ci.sh:147-148 now says so in a comment, which is a fair answer; restricting the asan run to sitl would still remove about half its run-step work at no coverage cost. Treat the job timings I quoted last round as indicative only — they were measured at the superseded head.
  • Tools/autotest/examples.py:103 — an example that ignores SIGTERM is SIGKILLed and gets no leak check. The comment there explains the intent is about coverage .gcda files, which is fair; under asan the consequence is just that such an example is not leak-checked. Pre-existing.

Checked and clean

--asan --debug really does reach the waf configure line and the binaries really are instrumented — confirmed from a passing build (base, unit-tests-asan) log printing a live LSan Suppressions used: 3 69 EKF_Buffer.cpp summary. Exit-code propagation is sound (build_ci.sh:19 is set -ex, so the trailing ccache -s cannot mask a failure). The workflow paths-ignore correctly re-includes !.github/workflows/test_unit_tests.yml and excludes neither Tools/autotest/** nor Tools/scripts/**. The AP_Declination_test.cpp change is behaviour-preserving: dec_tbl is [37][73], so at lat 90 the old code used index 36 with weight 0 — the out-of-bounds row 37 was multiplied by zero — and the new index 35 with weight 1.0 selects the same values. Commit split is clean, with the Tools: commits landing before the .github: commits that enable the matrix entries, so bisection never has an enabled-but-unimplemented target.

CI at this head was still running when I snapshotted (35 passing, 72 pending, 0 failing) — in particular the four asan jobs that were red last round had not reported yet.

@peterbarker
peterbarker marked this pull request as draft October 15, 2026 08:37
@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch from 56921c0 to 15b8eb4 Compare October 15, 2026 08:56
@AP-Review

AP-Review commented Oct 15, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-15)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: REQUEST CHANGES

Reviewed at head 15b8eb46d5.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33976/1.html#pr33976

Re-reviewed at 15b8eb46d5. The rebase changed no code compared with 56921c0b50, but CI at that head has since finished with three failures caused by this PR, and each has been traced to a cause. The verdict moves from COMMENT to REQUEST CHANGES.

Blocking

  1. Tools/ardupilotwaf/boards.py:963: -Wl,-u,__lsan_default_suppressions breaks every clang --asan build. clang's ASAN runtime is a static archive that only weakly references that symbol, so -u makes it a hard undefined reference. The flag is in env.LINKFLAGS before ap_common_checks() runs, and none of waf's configure checks link c++.cpp, so with clang every check fails. With clang-18 here, configure records HAVE_CMATH_ISFINITE: no and endian.h/byteswap.h not found, and the build later dies on the AP_Common/missing/byteswap.h __bswap_16 redefinition. Removing only that line gives yes/yes/yes. gcc is fine because libasan is shared. Options are to add the -u only after the configure checks, or only for gcc; I haven't built either.
  2. Tools/scripts/build_ci.sh:104: the clang asan jobs can't install the runtime. The container runs --user 1001, which has no passwd entry, so sudo fails ("you do not exist in the passwd database"), || /bin/true hides that, and the probe link then fails on the missing libclang_rt.asan-x86_64.a. Both build (clang, unit-tests-asan) and build (clang, examples-asan) failed this way at 56921c0b50. ardupilot-dev-clang:v0.2.0 was last pushed on 2026-06-25, so the package khancyr offered to add isn't in that tag yet. Either bump the image tag once it is, or keep the asan configs on base for now. Even with the runtime installed, item 1 would still break these jobs.
  3. libraries/AP_HAL_SITL/SITL_cmdline.cpp:501: test clang-scan-build now fails. Adding LSAN_IGNORE_OBJECT(autotest_dir) exposes an existing leak (a second --autotest-dir overwrites the first strdup) as Memory leak [8e1b7c5e894a5c979132d77618ea819e], which isn't in scan_build_suppressions.py. That job is green on master. Adding free(autotest_dir); before the strdup clears the report under clang 18 and 21 --analyze, and also frees the default from line 416.

Non-blocking

  • On macOS the same -u probably can't work either. Mach-O names the C symbol ___lsan_default_suppressions (three underscores), but ld receives -u __lsan_default_suppressions unchanged, so it names a symbol nothing defines. I checked this by cross-compiling only, not with a real Apple link. Whatever fixes item 1 should cover Darwin as well.
  • libraries/AP_Common/c++.cpp:161: this #endif closes the CYGWIN/Apple/malloc-wrapper #if from line 87, not AP_ADDRESS_SANITIZER_ENABLED.
  • Two commit messages describe earlier versions of their commits: 7fe6b2b760 says it fills allocations with zero, and f102b7e855 mentions command-line paths and per-function entries.
  • Still open from last time: Tools/autotest/pysim/util.py:148-149 copies os.environ, which env=None already does; Tools/autotest/autotest.py:117,143 also run the uninstrumented linux tests in the asan job; and Tools/autotest/examples.py:103 gives SIGKILLed examples no leak check.

Checked and clean

With gcc --asan --debug, all 75 sitl unit tests pass with no reports and the EKF_Buffer.cpp suppression is live. The four test fixes each remove a real sanitizer error that the base versions hit. The gcc examples-asan run is clean, and its log collection catches the old declination overflow. The declination fix gives the same values as before. The -Werror=undef-safe sanitizer detection and the per-subsystem commit split are fine. An earlier note about intermediate commits passing asan= too early is withdrawn, since AGENTS.md says intermediate commits need not build.

CI for 15b8eb46d5 was still queued when checked, so items 2 and 3 rely on the identical previous head's logs.

@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch from 15b8eb4 to 8633e03 Compare October 15, 2026 10:28
@AP-Review

AP-Review commented Oct 15, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-15)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 8633e0332d.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33976/2.html#pr33976

Re-reviewed at 8633e0332d (the head moved from 15b8eb46d5). All three blockers from last time are fixed, so the verdict moves from REQUEST CHANGES to COMMENT.

Fixed

  • The -u __lsan_default_suppressions flag is now added after the configure checks (Tools/ardupilotwaf/boards.py:873). With clang-18 the feature checks pass again, all 75 sitl unit tests pass and 55 examples run clean under ASAN.
  • The clang asan CI legs are excluded until the container has libclang-rt (.github/workflows/test_unit_tests.yml:134). Because of that, CI doesn't cover the clang fix above; only local runs do.
  • free(autotest_dir) (libraries/AP_HAL_SITL/SITL_cmdline.cpp:502): test clang-scan-build now passes in CI at this head. The base unit-tests-asan and examples-asan jobs pass too.
  • Also fixed: the Darwin _ symbol prefix, the #endif label, the 7fe6b2b760 message and the util.py env copy.

Non-blocking

  • Tools/scripts/build_ci.sh:134: 8633e0332d doesn't take effect in CI. unit_test_boards() checks autotest's own --asan, but 'Unit Tests ASAN' only passes --asan inside --waf-configure-args. The CI log at this head shows about 6 minutes spent building and running the uninstrumented linux tests first. Setting ao="--asan" for that job too gives a sitl-only run (checked locally). The comment at line 129 would need updating as well.
  • libraries/AP_Common/c++.cpp:215: commit 0bf8ce4740 says the suppressions are listed by function rather than by file, but this entry is leak:EKF_Buffer.cpp.
  • FYI, Tools/autotest/examples.py:180: 7 of 55 examples are killed (6 by SIGKILL, 1 by SIGTERM) before LSan's exit check runs, so their leaks aren't reported. This comes from the existing kill logic, not the new code.

Not run: full vehicle SITL under ASAN (your Copter/Plane/etc. results weren't reproduced), a native macOS link, and the vehicle autotest CI jobs, which were still queued.

@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch from 8633e03 to 68baabd Compare October 16, 2026 01:28
@AP-Review

AP-Review commented Oct 16, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-16)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 68baabdb44.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33976/3.html#pr33976

Re-reviewed at 68baabdb44 (the head moved from 8633e0332d). The only code change since last time is in build_ci.sh. The verdict stays COMMENT.

Fixed

  • Tools/scripts/build_ci.sh:134: 'Unit Tests ASAN' now passes --asan to autotest. A local run of the job's command built and ran only the 75 sitl tests, all passing.
  • libraries/AP_Common/c++.cpp:215: the 3edc48b commit message now explains the file-based EKF_Buffer.cpp entry.
  • The earlier blocker fixes are unchanged and still pass locally with GCC (75 unit tests, 55 examples).

Non-blocking

  • Tools/autotest/examples.py:33: if the 10 s SIGTERM lands while ASAN is still writing a report, the log has ERROR: AddressSanitizer but no SUMMARY line. That log is treated as clean and then deleted. With GCC the window is small: a real report took about 0.11 s on a SITL example, and an earlier error still fails the example through its rc=1 exit. The comment at line 62 shows termination has cut reports short before, though. Also matching ERROR: AddressSanitizer / ERROR: LeakSanitizer would close the gap. Suppressed leaks write no ERROR line with print_suppressions=0, so this adds no false failures.
  • FYI, unchanged: examples killed by SIGKILL, or dying on SIGTERM, still never reach LSan's exit check. This comes from the existing kill logic.

Not run: full vehicle SITL under ASAN, a macOS link, and clang ASAN at this head. CI at this head was still pending.

…ing test

The test explicitly called the destructor and then read the freed
buffer, and filled a 2048 byte array with 'a' with no terminator before
passing it to printf.  Both are found by address sanitizer.
…e test

The string was replaced three times without being freed, and the last
one was read back after its destructor had been called.
The index was advanced before the last two checks, so they tested the
following result and read past the end of the table on the final call.
Two tests allocated an ekf_imu_buffer and never deleted it.  Note that
deleting one does not free the memory init() allocated inside it, as
these classes have no destructor; that allocation is covered by the
leak suppressions instead.
The link flag was only added for clang, so a GCC ASAN build got
instrumented objects and an uninstrumented link line.  That broke waf's
own configure checks, each of which links a test program: they failed
with undefined references to __asan_init and the features were recorded
as unavailable.  HAVE_BYTESWAP_H going missing that way made
AP_Common/missing/byteswap.h define __bswap_16 itself, which then
collided with the system header.
The framework treats a non-empty sanitizer log as a failure, so anything
written to it which is not an error turns a good run into a bad one.
Turn off the summary of suppressed allocations for the same reason
verbosity is already zero here.

Also stop llvm-symbolizer fetching debug info over the network, which it
does by default on Ubuntu; it blocks for about ninety seconds a frame,
so the report is never finished before the process goes away.
gcc and clang spell the sanitizer test differently and neither
understands the other's form, so the __has_feature test has to be
nested; on gcc a flat "defined(__has_feature) && __has_feature(...)" is
a syntax error rather than a short circuit.

LSAN_IGNORE_OBJECT marks a single allocation as deliberately kept.  It
is preferable to a suppression naming the function which allocated it,
as a suppression is matched against every frame of the allocation stack
and so also hides everything else allocated below that function.
These two are kept for the life of the process.  Marking the objects
themselves is exact, where a suppression naming _parse_command_line
would cover the whole of that function - including constructing the
simulated vehicle and running _sitl_setup().
Some allocations are made once and deliberately kept for the life of the
vehicle: the EKF buffers, the sensor backends and the simulated serial
devices.  Listing them here means leak checking can be left on rather
than turned off wholesale.  The sanitizer finds this function by itself,
so no environment variable or external suppressions file is needed.

Each entry is matched against every frame of the allocation stack, so
naming the allocating function keeps the rest of its file checked.  The
EKF buffers are named by file because the two classes which allocate
them have no destructor, so every allocation in it is deliberate.
Stop llvm-symbolizer fetching debug info over the network, which it does
by default on Ubuntu and which blocks for about ninety seconds a frame.
Ignored by tests which were not built with --asan.
--asan searched for a clang to build with, which has a surprising side
effect: AP_Networking_Config.h disables AP_NETWORKING_ENABLED for clang
on linux, so every sanitized build had all of the networking code
compiled out of it.  That is why the networking tests fail under --asan
rather than being run.

The sanitizer works with gcc as well, so honour CXX/CC when the caller
has set them and otherwise leave the choice to waf.
build_examples did not take the asan argument that autotest.py passes
it, so --asan build.examples died with a TypeError which the broad
exception handler reported as a build failure.  --asan is only
implemented for the sitl board, so it is not passed to the
cross-compiled boards.

The examples send their output to /dev/null and most are killed with a
signal rather than exiting, so point the sanitizer at a log file and
fail the step if any example leaves a report behind.
The sanitizer's redzones push loop() to 3040 bytes, past the 2000 byte
limit this example sets for itself, so the example cannot be built with
--asan.
The WMM tables are read with a bilinear interpolation which uses the
next sample along in both latitude and longitude, so the table edges
and out-of-range positions are what need checking.  Sweeping them under
the address sanitizer covers the indexing as well as the results.
The interpolation reads the next sample along in both latitude and
longitude, but the index was not constrained, so at latitude 90 or
longitude 180 the +1 ran off the end of dec_tbl.  Found by running the
examples under the address sanitizer.

Take the corner position back from the constrained index so that the
interpolation still lines up with the table; the index is only
constrained at the very edges, and the position is unchanged everywhere
else.
The examples are built with --asan and run with autotest's --asan, which
is what makes run.examples collect the sanitizer reports.
--asan is implemented for the sitl board alone, so the linux half of the
run was building and running the same tests a second time without any
instrumentation.  The unit-tests job already covers them.
@peterbarker
peterbarker force-pushed the pr-claude/ci-unit-tests-asan branch from 68baabd to b22e283 Compare October 16, 2026 03:32
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: ACCEPT

Reviewed at head b22e2837df.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33976/4.html#pr33976

Re-reviewed at b22e2837df (the head moved from 68baabdb44). The only code change is in Tools/autotest/examples.py. No defects remain open, so the verdict moves from COMMENT to ACCEPT.

Fixed

  • Tools/autotest/examples.py:31: asan_logs() now also matches ERROR: AddressSanitizer and ERROR: LeakSanitizer. A report cut short by the SIGTERM now fails the example. I checked this against fixture logs and real GCC ASAN output. A clean 55-example ASAN run showed no false positives.
  • The earlier fixes are unchanged. They still pass locally with GCC: 75 sitl unit tests and 55 examples.

FYI, unchanged: examples that are SIGKILLed still never reach LSan's exit check. This comes from the existing kill logic.

Not run: full vehicle SITL under ASAN, a macOS link, and a full clang ASAN build. CI at this head was still pending.

@peterbarker
peterbarker marked this pull request as ready for review October 16, 2026 09:36
@peterbarker
peterbarker requested a review from khancyr October 16, 2026 09:36
@peterbarker peterbarker moved this to ReadyForDevCall in Peter's ArduPilot 4.8 Queue Oct 16, 2026
@tridge
tridge merged commit fab2429 into ArduPilot:master Oct 17, 2026
111 checks passed
@github-project-automation github-project-automation Bot moved this from ReadyForDevCall to Done in Peter's ArduPilot 4.8 Queue Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIReview CI EKF github_actions Pull requests that update GitHub Actions code Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants