Skip to content

fix(loader): encode initial pcdata safe value#940

Draft
liuq19 wants to merge 1 commit into
bytedance:mainfrom
liuq19:bugfix/pcdata-safe-entry
Draft

fix(loader): encode initial pcdata safe value#940
liuq19 wants to merge 1 commit into
bytedance:mainfrom
liuq19:bugfix/pcdata-safe-entry

Conversation

@liuq19

@liuq19 liuq19 commented May 13, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

fix

Check the PR title.

  • This PR title match the format: (optional scope):
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. Not required for this loader metadata correctness fix.

(Optional) Translate the PR title into Chinese.

fix(loader): 编码初始 pcdata safe 值

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
This PR fixes a loader pcdata encoder edge case where a single entry whose value equals the runtime pcdata start value (-1) was skipped entirely. In particular, Loader.LoadOne/LoadMany with NoPreempt left as false builds a PcUnsafePoint table containing one PCDATA_UnsafePointSafe entry. Before this change, MarshalBinary encoded that table as only the trailing terminator byte, so the runtime async-preempt path could hit an invalid pc-encoded table.

The fix allows the first emitted pcdata record to carry a zero value delta, which matches the runtime decoder behavior for the first pcdata step. Later zero value deltas are still skipped so the terminator encoding remains unambiguous. The PR also adds regression coverage for the single safe-entry encoding and the LoadOne/LoadMany helper path.

zh(optional):
修复 loader pcdata 编码的边界情况:当首条记录的值等于 runtime pcdata 初始值 -1 时,之前会被 dv == 0 逻辑跳过,导致 NoPreempt=false 生成的 PcUnsafePoint 表只剩 terminator。现在允许首条实际发出的记录使用零 value delta,同时保留后续 dv == 0 的跳过逻辑,避免和 terminator 语义冲突。

Validation:

  • go test ./... (from loader/)
  • go test ./...
  • SONIC_USE_OPTDEC=1 SONIC_USE_FASTMAP=1 SONIC_ENCODER_USE_VM=1 go test ./... -count=1

(Optional) Which issue(s) this PR fixes:

Fixes #939

(optional) The PR that updates user documentation:

Not required.

@codecov-commenter

codecov-commenter commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.28%. Comparing base (59be92f) to head (94d4206).
⚠️ Report is 71 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #940      +/-   ##
==========================================
- Coverage   51.86%   51.28%   -0.59%     
==========================================
  Files         127      172      +45     
  Lines       10893    14147    +3254     
==========================================
+ Hits         5650     7255    +1605     
- Misses       4920     6502    +1582     
- Partials      323      390      +67     
Flag Coverage Δ
arm 42.94% <ø> (?)
macos-latest 44.10% <ø> (?)
ubuntu-24.04-arm 42.94% <ø> (?)
ubuntu-latest 49.74% <ø> (?)
x86 49.74% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

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.

loader: LoadOne/LoadMany with NoPreempt left as false produces empty PcUnsafePoint table, crashes runtime async preempt

2 participants