Skip to content

Revert - The disabling of enable_stack_frame_gaps in bpf_account_data_direct_mapping#7056

Merged
Lichtso merged 2 commits intoanza-xyz:masterfrom
Lichtso:revert/enable_stack_frame_gaps_in_bpf_account_data_direct_mapping
Jul 23, 2025
Merged

Revert - The disabling of enable_stack_frame_gaps in bpf_account_data_direct_mapping#7056
Lichtso merged 2 commits intoanza-xyz:masterfrom
Lichtso:revert/enable_stack_frame_gaps_in_bpf_account_data_direct_mapping

Conversation

@Lichtso
Copy link
Copy Markdown

@Lichtso Lichtso commented Jul 21, 2025

Problem

In the original direct mapping implementation it was necessary to disable stack frame gaps to avoid issues in memory accesses spanning over multiple regions. In the new direct mapping implementation memory accesses are restricted to a single region, thus we can revert this part of the feature gate back to the state it is when the feature is disabled. This way the stack behavior will only change when a program opts into SBPFv1.

Summary of Changes

Re-enables enable_stack_frame_gaps in the SBPF VM.

@mergify
Copy link
Copy Markdown

mergify Bot commented Jul 21, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (82d3802) to head (28cc4d8).
Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7056     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      374799   374799             
=========================================
- Hits       311897   311892      -5     
- Misses      62902    62907      +5     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Lichtso Lichtso marked this pull request as ready for review July 22, 2025 12:00
@Lichtso Lichtso requested a review from a team as a code owner July 22, 2025 12:00
Comment thread programs/sbf/rust/invoke/src/lib.rs Outdated
Comment on lines +1363 to +1364
for i in 10..MAX_CALL_DEPTH / 2 {
let stack = &mut stack[i * STACK_FRAME_SIZE * 2..][..STACK_FRAME_SIZE];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The maximum call depth shouldn't have changed.

stack_frame_size: compute_budget.stack_frame_size,
enable_address_translation: true,
enable_stack_frame_gaps: !feature_set.bpf_account_data_direct_mapping,
enable_stack_frame_gaps: true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps we can clean up this setting then.

@Lichtso Lichtso force-pushed the revert/enable_stack_frame_gaps_in_bpf_account_data_direct_mapping branch from df4fce0 to 28cc4d8 Compare July 23, 2025 10:11
@Lichtso Lichtso requested a review from LucasSte July 23, 2025 15:27
@Lichtso Lichtso merged commit 151cf20 into anza-xyz:master Jul 23, 2025
41 of 42 checks passed
@Lichtso Lichtso deleted the revert/enable_stack_frame_gaps_in_bpf_account_data_direct_mapping branch July 23, 2025 17:11
puhtaytow pushed a commit to puhtaytow/agave that referenced this pull request Jul 24, 2025
…ata_direct_mapping` (anza-xyz#7056)

* Reverts the disabling of enable_stack_frame_gaps in bpf_account_data_direct_mapping.

* Adjusts test_stack_heap_zeroed.
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