-
Notifications
You must be signed in to change notification settings - Fork 399
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
gnovm(bug): time.Time rendering causes Machine Panic #1588
Comments
full trace result and txtar test: https://gist.github.com/thehowl/f0ca7715705998116c2c88a6811a692f This seems to be related to realm storage of time.Time values which have a Location value of the timezone. Needs some more investigation. Specifically, the machine info seems to point to the error happening somewhere here: gno/gnovm/stdlibs/time/time.gno Lines 467 to 468 in b619351
|
You're spot on with the line in question @thehowl. It's resolving the |
…#1606) <!-- please provide a detailed description of the changes made in this pull request. --> Addresses #1588. The following explanation refers to the txtar test linked to in the issue. [Here it is](https://gist.github.com/thehowl/f0ca7715705998116c2c88a6811a692f) for convenience. ### Why the panic? This line in the txtar test's `Read()` function caused the panic: `out += post.CreatedAt.Format(time.RFC3339)`. The `Format` method results in invoking the `Time.locabs` method, and this is the line inside that method that caused the panic: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468 The panic occurs when trying to resolve the value of `l.cacheZone` as a part of the selector operation. This is a panic caused by an invalid type assertion. The invalid type assertion happens on the line that was modified for this PR: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468 In the case of this particular txtar test, the target value's base value is a struct value, not an array value. We can look at how the original `cacheZone` value was persisted to understand why the base type is different from what we'd expect. ### Original value persistence The original time value is built as a result of this line in the txtar test's `Store()` function: `parsedTime, err = time.Parse(time.RFC3339, date)`. The `cacheZone` value that is persisted either has a struct base value or is nil for the majority of this operation. However, the time value's `Location` field gets updated near the end of the `parse` operation and assumes the value of the result of the `FixedZone` function: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/format.gno#L1306 Taking a look at what `FixedZone` actually does, we can see that the location's `cacheZone` is now a reference to a zone that exists as a part of an array -- a struct with a base type of an array value: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/timezoneinfo.gno#L114 This explains why the value we were seeing when trying to resolve the `cacheZone` selector had an array base. ### The fix It doesn't matter if the source value's parent is an array or struct; we need the underlying value's type and value. That being said, I don't think the array type assertion needs to happen since the type value is accessible via the `Elem()` method defined as part of the `Type` interface. Now the value is obtained without causing a panic. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
…gnolang#1606) <!-- please provide a detailed description of the changes made in this pull request. --> Addresses gnolang#1588. The following explanation refers to the txtar test linked to in the issue. [Here it is](https://gist.github.com/thehowl/f0ca7715705998116c2c88a6811a692f) for convenience. ### Why the panic? This line in the txtar test's `Read()` function caused the panic: `out += post.CreatedAt.Format(time.RFC3339)`. The `Format` method results in invoking the `Time.locabs` method, and this is the line inside that method that caused the panic: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468 The panic occurs when trying to resolve the value of `l.cacheZone` as a part of the selector operation. This is a panic caused by an invalid type assertion. The invalid type assertion happens on the line that was modified for this PR: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468 In the case of this particular txtar test, the target value's base value is a struct value, not an array value. We can look at how the original `cacheZone` value was persisted to understand why the base type is different from what we'd expect. ### Original value persistence The original time value is built as a result of this line in the txtar test's `Store()` function: `parsedTime, err = time.Parse(time.RFC3339, date)`. The `cacheZone` value that is persisted either has a struct base value or is nil for the majority of this operation. However, the time value's `Location` field gets updated near the end of the `parse` operation and assumes the value of the result of the `FixedZone` function: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/format.gno#L1306 Taking a look at what `FixedZone` actually does, we can see that the location's `cacheZone` is now a reference to a zone that exists as a part of an array -- a struct with a base type of an array value: https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/timezoneinfo.gno#L114 This explains why the value we were seeing when trying to resolve the `cacheZone` selector had an array base. ### The fix It doesn't matter if the source value's parent is an array or struct; we need the underlying value's type and value. That being said, I don't think the array type assertion needs to happen since the type value is accessible via the `Elem()` method defined as part of the `Type` interface. Now the value is obtained without causing a panic. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
While working on the rendering of the blog realm, I came upon a possible Gno bug. Specifically, the bug happens when trying to return the value of a
time.Time
, formatted to theRFC3339
format using.Format()
.Take a look at the following example realm.
Calling the following commands:
Here is a gist of the error.
I am sure the formats passed in as strings are valid in Go: https://go.dev/play/p/IFMutN8aWGV
Running a
vm/qeval
returns a 500 internal server error:What might be causing this?
cc @thehowl
The text was updated successfully, but these errors were encountered: