Fix clang-tidy and sanitizer CI config blocking score/time_daemon checks by MaciejSalwa543 · Pull Request #216 · eclipse-score/time

MaciejSalwa543 · 2026-09-09T18:32:23Z

Description

Starting on #77, found that QA / Clang-Tidy and QA / Sanitizers show green but aren't
actually checking anything.

Clang-Tidy: under --config=clang-tidy, most targets fail to build before the aspect
runs (clang: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]). MODULE.bazel defines two LLVM toolchains; the
one used here never sets stdlib, unlike the newer llvm_toolchain_coverage one. Fix:
mirror stdlib = {"": "stdc++"} onto it.

Sanitizers: under --config=asan_ubsan_lsan, every test is silently skipped
(Executed 0 out of 22 tests: 22 were skipped). .bazelrc set
--@score_cpp_policies//sanitizers/flags:sanitizer=asan_ubsan_lsan, a flag removed in the
score_cpp_policies commit this repo pins — real API is three bool flags (:asan, :ubsan,
:lsan). Since none were set, the sanitizer run_under wrapper is incompatible and every
test gets skipped. Fix: set the three bool flags.

Both look like they've been broken since #102 (2026-07-03), not a recent regression —
continue-on-error: true on both jobs hid it.

Once the build actually worked, digging further
showed clang-tidy was still only running its own narrow built-in default (clang-diagnostic-*,
clang-analyzer-*) instead of the S-CORE baseline — the baseline lives in
score_cpp_policies, but external/ is a sibling of score/, not an ancestor, so clang-tidy's
own directory-walk config discovery could never reach it. Mirrored the baseline into
//:.clang-tidy and wired it via local_configs. That surfaced 552 real findings in
score/time_daemon/ that had never actually been checked — fixed what's genuinely fixable
(missing includes, redundant/missing default member inits, magic numbers, pass-by-value+move,
const-correctness, trailing return types, a duplicated global turned into a member) and added
specific justification comments for the suppressions that are the right call.

Results

bazel test --config=clang-tidy //score/time_daemon/... → 22/22 pass, 0 clang-tidy findings
bazel test --config=asan_ubsan_lsan --config=x86_64-linux //score/time_daemon/... →
22/22 pass, zero failures.
bazel test //:format.check → 5/5 pass.

Scope note

These two potential bugs are repo-wide (MODULE.bazel/.bazelrc), not time_daemon-specific —
also unblocks #76/#78/#79 and is a prerequisite for #111/#112. Open to splitting into its own
issue/PR if preferred.

Related ticket

closes #77

github-actions · 2026-09-09T18:33:15Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 3693887b-b122-4284-a6d7-eaa65f695b06
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'platforms', the root module requires module version platforms@1.0.0, but got platforms@1.1.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'score_platform', the root module requires module version score_platform@0.7.1, but got score_platform@0.7.2 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_oci', the root module requires module version rules_oci@2.2.7, but got rules_oci@2.3.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 3 packages loaded
WARNING: Target pattern parsing failed.
ERROR: Skipping '//:license-check': no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
ERROR: no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
INFO: Elapsed time: 14.328s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

MaciejSalwa543 · 2026-09-09T18:36:56Z

sanitizers.yml's matrix only runs asan_ubsan_lsan (line 39) — TSan was never added,
despite #102 implementing #43, titled "Add sanitizer CI (ASAN/TSAN/UBSAN)". Doesn't seem
to be intentional — I'd expect TSan to be in the CI job from the title/scope of the mentioned
ticket & PR.

Ran TSan+UBSan manually against score/time_daemon/... out of curiosity (not in CI, not part
of this PR): 2 failures, both ThreadSanitizer: data race.

ptp_control_flow_divider_test — in EventDrivenMachine's stop_token wait path
(event_driven_machine.cpp:28). Self-pipe-wakeup-vs-teardown races are a common real bug
shape here.
job_runner_test — in score_baselibs' Meyer-singleton logging init
(GetSingletonWithFence) — looks more like a known TSan false-positive on fence-based
lock-free init.

Looks like something I want to fix and add TSan to CI, would you agree?

pawelrutkaq · 2026-09-10T06:25:00Z

@lavrovvalera ?

MaciejSalwa543 · 2026-09-10T07:05:09Z

Closed due to missing clang-tidy config file

pawelrutkaq

We need to tue this return type as auto or at least confirm it

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

pawelrutkaq

beside two small thing, now looks good to me

lavrovvalera · 2026-10-02T06:42:44Z

+# Default clang-tidy configuration for S-CORE C++ modules.
+# NOTE: the set of enabled checks is yet subject to be tailored per module.
+Checks: "-*,clang-analyzer-*,cert-*,cppcoreguidelines-*,bugprone-*,misc-*,performance-*,readability-*,modernize-*,-modernize-use-trailing-return-type"


I'm curious if it is the full or not the full list.
where this list shall be taken from

lavrovvalera · 2026-10-02T06:43:45Z

 clang_tidy_aspect = make_clang_tidy_aspect(
    binary = Label("@llvm_toolchain//:clang-tidy"),
-    # No local_configs: use only the S-CORE baseline from score_cpp_policies.
+    local_configs = [Label("//:.clang-tidy")],


why can't we use the score baseline?
or it is intentional too narrow and we want to extend it?

lavrovvalera · 2026-10-02T06:49:09Z

    std::vector<Job> jobs_;
    const std::string name_;
-    Result status_;
+    Result status_{Result::kIdle};


shall it be done in the constructor?

lavrovvalera · 2026-10-02T06:49:55Z

      gptp_machine_{nullptr},
      verification_machine_{nullptr},
      ipc_publisher_{nullptr},
-      ctrl_flow_divider_{nullptr},


why did yuo drop them from the init list?

lavrovvalera · 2026-10-02T06:50:45Z

+    std::shared_ptr<SvtVerificationMachine> verification_machine_;            ///< Handles SVT verification
+    std::shared_ptr<SvtPublisher> ipc_publisher_;                             ///< Publishes SVT data via IPC
+    std::shared_ptr<PtpControlFlowDivider> ctrl_flow_divider_;                ///< Divides PTP control flow
+    TimebaseHandler::Status handler_status_{TimebaseHandler::Status::kIdle};  ///< Current status of the handler


why not to set it in the ctor in init list, like all other members?

lavrovvalera · 2026-10-02T09:36:52Z

 /// \brief General type class to store and pass all necessary data
 struct TimeBaseSnapshot
 {
+    // NOLINTBEGIN(misc-non-private-member-variables-in-classes) — plain data aggregate, like the


but that is nt a class, that is a struct.
where public member s are allowed or?

lavrovvalera · 2026-10-02T09:40:06Z

+    // kGptpIpcName is a char-array constant used as a default arg for a std::string param;
+    // ordinary literal decay, not raw pointer/buffer use.
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay)
    explicit ShmPTPEngine(std::string ipc_name = score::ts::details::kGptpIpcName) noexcept;


can we update it to not char*? can use it as a string or not use it as defualt value?
then we can try to avoid suppression or?

lavrovvalera · 2026-10-02T09:40:31Z

 */
-std::shared_ptr<GPTPShmMachine> CreateGPTPShmMachine(const std::string& name,
-                                                     const std::string& ipc_name = score::ts::details::kGptpIpcName);
+// kGptpIpcName is a char-array constant used as a default arg for a const std::string&; the


the same here

lavrovvalera · 2026-10-02T09:44:59Z

                                       std::chrono::nanoseconds max_time_jump_allowed,
-                                       std::chrono::nanoseconds sync_debounce_threshold,
-                                       std::uint8_t valid_frames_threshold)
+                                       std::uint8_t valid_frames_threshold,


why did you swap them?

lavrovvalera · 2026-10-02T09:45:45Z


+    if (!last_sync_frame_.has_value())
+    {
+        return false;


either return here the is_time_jump_detected
or move this if to the very top

github-project-automation Bot added this to TIM - Time FT Sep 9, 2026

github-project-automation Bot moved this to In Progress in TIM - Time FT Sep 9, 2026

MaciejSalwa543 deployed to workflow-approval September 9, 2026 18:32 — with GitHub Actions Active

MaciejSalwa543 marked this pull request as ready for review September 10, 2026 06:02

pawelrutkaq requested a review from lavrovvalera September 10, 2026 06:24

MaciejSalwa543 closed this Sep 10, 2026

github-project-automation Bot moved this from In Progress to Done in TIM - Time FT Sep 10, 2026

MaciejSalwa543 reopened this Sep 15, 2026

MaciejSalwa543 had a problem deploying to workflow-approval September 15, 2026 06:23 — with GitHub Actions Error

MaciejSalwa543 marked this pull request as draft September 15, 2026 06:40

MaciejSalwa543 had a problem deploying to workflow-approval September 15, 2026 07:13 — with GitHub Actions Error

MaciejSalwa543 force-pushed the fix-sca-sanitizer-ci-config-77 branch from 172e39e to 8926082 Compare September 15, 2026 07:29

MaciejSalwa543 deployed to workflow-approval September 15, 2026 07:29 — with GitHub Actions Active

MaciejSalwa543 marked this pull request as ready for review September 15, 2026 10:29

pawelrutkaq requested changes Sep 16, 2026

View reviewed changes

MaciejSalwa543 had a problem deploying to workflow-approval September 17, 2026 17:39 — with GitHub Actions Error

MaciejSalwa543 added 5 commits September 17, 2026 20:41

Fix clang-tidy and sanitizer CI config blocking time_daemon checks

f75f295

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

Enable S-CORE clang-tidy baseline and fix time_daemon findings

5cd42de

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

fix clang-tidy for yamlfmt

c825f16

Add clang-tidy path to collect only time_deamon findings

73a1f0d

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

Fix swappable params in TimeJumpsValidator ctor

3b97a1c

MaciejSalwa543 added 3 commits September 17, 2026 20:41

Drop unjustified nodiscard on GetName/Deinitialize

9749fc3

Revert modernize-use-trailing-return-type for time_daemon

259719f

Add frozen baseline for clang-tidy findings outside time_daemon

96907e6

MaciejSalwa543 force-pushed the fix-sca-sanitizer-ci-config-77 branch from 873aa0b to 96907e6 Compare September 17, 2026 18:49

MaciejSalwa543 deployed to workflow-approval September 17, 2026 18:49 — with GitHub Actions Active

pawelrutkaq reviewed Sep 18, 2026

View reviewed changes

Comment thread score/time_daemon/src/ptp_machine/stub/details/stub_ptp_engine.h Outdated

Comment thread .github/workflows/clang-tidy.yml Outdated

Remove unneeded comments

f7fbb97

MaciejSalwa543 requested a deployment to workflow-approval September 18, 2026 06:57 — with GitHub Actions Waiting

MaciejSalwa543 mentioned this pull request Sep 24, 2026

Add TSan job to sanitizer CI #233

Open

lavrovvalera reviewed Oct 2, 2026

View reviewed changes

Conversation

MaciejSalwa543 commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Results

Scope note

Related ticket

Uh oh!

github-actions Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

MaciejSalwa543 commented Sep 9, 2026

Uh oh!

pawelrutkaq commented Sep 10, 2026

Uh oh!

MaciejSalwa543 commented Sep 10, 2026

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch is waiting to be deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaciejSalwa543 commented Sep 9, 2026 •

edited

Loading

github-actions Bot commented Sep 9, 2026 •

edited

Loading