Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add panicking::panic_count to mc-sgx-panic #31

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Conversation

nick-mobilecoin
Copy link
Collaborator

No description provided.

@github-actions github-actions bot requested review from awygle and NotGyro January 10, 2023 22:54
@github-actions github-actions bot added the size/L Large PRs label Jan 10, 2023
@nick-mobilecoin nick-mobilecoin force-pushed the nick/common-panic branch 2 times, most recently from cf501b9 to b4249a1 Compare January 10, 2023 22:59
@nick-mobilecoin nick-mobilecoin marked this pull request as draft January 10, 2023 23:08
@nick-mobilecoin
Copy link
Collaborator Author

Moved back to a draft, with the failures on, https://github.com/mobilecoinfoundation/sgx-std/actions/runs/3887979469/jobs/6634849726, it seems pretty clear that the value is not being thread local

---- panicking::test::decrementing_one_at_a_time stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `5`,
 right: `4`', panic/src/panicking.rs:100:9

---- panicking::test::incrementing_one_at_a_time stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `6`,
 right: `1`', panic/src/panicking.rs:88:9

---- panicking::test::initialized_to_0 stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `6`,
 right: `0`', panic/src/panicking.rs:68:9

@nick-mobilecoin
Copy link
Collaborator Author

Doing some debugging have the side by side of what rust std does and what is being attempted in this PR https://godbolt.org/z/r3c37Tqxh
I also removed no_std and tried to do what rust std does and got the same failure...

@nick-mobilecoin nick-mobilecoin marked this pull request as ready for review January 11, 2023 18:06
@github-actions github-actions bot requested a review from jcape January 11, 2023 18:06
@nick-mobilecoin
Copy link
Collaborator Author

moved out of draft, found problem that was causing llvm-cov to fail, added explanation in code at https://github.com/mobilecoinfoundation/sgx-std/pull/31/files#diff-0ce129c061b49b04073e4988bbaca5999d4b7148812aff7489623ab6f53edad8R66

        /// By default each test runs in a separate thread thus having their own
        /// thread local copy of `LOCAL_PANIC_COUNT`. However per
        /// https://github.com/rust-lang/rust/issues/58907 when running single
        /// threaded the thread is re-used for each test execution so the value
        /// should be reset to a known value on each test. This also means that
        /// there isn't a reliable way to test that 0 is the true initial value.

Copy link
Contributor

@awygle awygle left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

@nick-mobilecoin
Copy link
Collaborator Author

Note: intel/linux-sgx#283

I looked through the current doc, https://download.01.org/intel-sgx/sgx-linux/2.18/docs/Intel_SGX_Developer_Reference_Linux_2.18_Open_Source.pdf trying to get a feel of how the threading worked, but wasn't able to piece it together.
If I'm reading the issue correctly, for panics, it will only be an issue if threads are bound to the untrusted thread, and the untrusted side assumes each ecall is a fresh state

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Merging #31 (c93e6bf) into main (63c237b) will increase coverage by 3.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   88.09%   91.71%   +3.61%     
==========================================
  Files           5        6       +1     
  Lines         126      181      +55     
==========================================
+ Hits          111      166      +55     
  Misses         15       15              
Impacted Files Coverage Δ
panic/src/lib.rs 100.00% <ø> (ø)
panic/src/panicking.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Base automatically changed from nick/common-panic to main January 13, 2023 22:24
Copy link
Contributor

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Jan 25, 2023

✅ This pull request merged successfully as part of a Graphite job
Stack job ID: 174FRuRmqv3ul7Wrh6cz.
See details on graphite.dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update github actions size/L Large PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants