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

feat(gstd): get panic message without panic_info_message #3527

Merged
merged 46 commits into from
Jan 16, 2024

Conversation

StackOverflowExcept1on
Copy link
Member

@StackOverflowExcept1on StackOverflowExcept1on commented Nov 27, 2023

Resolves #3524

We are getting closer to cargo build +stable -p gstd

@gear-tech/dev

@StackOverflowExcept1on StackOverflowExcept1on added A0-pleasereview PR is ready to be reviewed by the team D3-gstd Gear Standard Library labels Nov 27, 2023
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Still considering best approach here

Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

So what have we decided to do with possibility to throw whole display for not(feature(debug))? @StackOverflowExcept1on
And how is it working wout oom-handler and allocation error on stable panic message? It would be good to have some tests maybe

gstd/src/common/mod.rs Outdated Show resolved Hide resolved
@StackOverflowExcept1on
Copy link
Member Author

@breathx

So what have we decided to do with possibility to throw whole display for not(feature(debug))?

I think it's quite expensive even for the nightly version. core::fmt::write uses about 2.5 KiB, and extra 2.5 KiB is used for impl Display for u32 (it's quite large). Thus, contracts will increase by 5 KiB on the stable version and by 2,5 KiB on nightly.

How is it working wout oom-handler and allocation error on stable panic message?

We've already discussed this. On the nightly version, gr_oom_panic syscall is called. On the stable version, gr_panic syscall is called with message like this: 'memory allocation of 65536 bytes failed', library/alloc/src/alloc.rs:411:13. This creates bit difference in behavior, but in general it is unlikely that anyone will rely on it in tests.

Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Got your point. I'm good with the solution. Awesome work
Please review @ark0f

@ark0f
Copy link
Member

ark0f commented Dec 11, 2023

I'd just print PanicInfo without any parsing. Do we have cons with this approach?

@StackOverflowExcept1on
Copy link
Member Author

@ark0f there are several reasons:

  1. As user of a smart contract, it would be convenient for me to see an error like 'You are not admin', /path/to/lib.rs:1:2 instead of panicked at /path/to/lib.rs:1:2:\nYou are not admin. The user will see an error at the beginning of the line instead of long path to source at the beginning.
  2. Different versions of rust have different default panic message formats. If you rely on this format in tests, then you will have to change the tests. This also applies to our pallet tests.

gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/Cargo.toml Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/mod.rs Outdated Show resolved Hide resolved
gstd/src/lib.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
examples/waiter/tests/utils.rs Show resolved Hide resolved
gstd/Cargo.toml Show resolved Hide resolved
gstd/Cargo.toml Show resolved Hide resolved
gstd/src/common/handlers.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/Cargo.toml Show resolved Hide resolved
gstd/src/common/handlers.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
gstd/src/common/handlers.rs Outdated Show resolved Hide resolved
@breathx breathx merged commit d9de34e into master Jan 16, 2024
11 checks passed
@breathx breathx deleted the av/gstd-panic-msg branch January 16, 2024 09:49
@breathx
Copy link
Member

breathx commented Jan 16, 2024

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team A2-mergeoncegreen PR is ready to merge after CI passes D3-gstd Gear Standard Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: get rid of nightly features in gstd
4 participants