Skip to content

Conversation

@mauri870
Copy link
Member

@mauri870 mauri870 commented Nov 5, 2023

Currently if morestack on g0 happens the wasm runtime prints
"RuntimeError: memory access out of bounds", which is quite misleading.
By switching to a crash stack we can get better stacktraces
for the error.

There is no way to automate tests for this feature on wasm, since
TestG0StackOverflow relies on spawning a subprocess which is not
supported by the wasm port.

The way I got this tested manually is to comment everything in
TestG0StackOverflow, leaving just runtime.G0StackOverflow().

Then it is a matter of invoking the test:

GOOS=js GOARCH=wasm go test runtime -v -run=TestG0StackOverflow

@gopherbot
Copy link
Contributor

This PR (HEAD: 234dd5a) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@mauri870 mauri870 force-pushed the feature/crash-stack-wasm branch from 234dd5a to 0932b0f Compare November 5, 2023 19:01
@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 0932b0f) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@mauri870 mauri870 force-pushed the feature/crash-stack-wasm branch from 0932b0f to 12e6410 Compare November 6, 2023 20:32
@gopherbot
Copy link
Contributor

This PR (HEAD: 12e6410) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

Currently if morestack on g0 happens the wasm runtime prints
"RuntimeError: memory access out of bounds", which is quite misleading.
By switching to a crash stack we can get better stacktraces
for the error.

There is no way to automate tests for this feature on wasm, since
TestG0StackOverflow relies on spawning a subprocess which is not
supported by the wasm port.

The way I got this tested manually is to comment everything in
TestG0StackOverflow, leaving just runtime.G0StackOverflow().

Then it is a matter of invoking the test:

    GOOS=js GOARCH=wasm go test runtime -v -run=TestG0StackOverflow

Change-Id: If2819938d6424da1b4fd7e8e5dc53e8efeeebb6d
@mauri870 mauri870 force-pushed the feature/crash-stack-wasm branch from 12e6410 to 21c12bc Compare November 6, 2023 22:01
@gopherbot
Copy link
Contributor

This PR (HEAD: 21c12bc) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 5:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 5: Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 5: Commit-Queue+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 5:

Dry run: CV is trying the patch.

Bot data: {"action":"start","triggered_at":"2023-11-18T04:17:55Z","revision":"e30e6b085172a0012799f740eefc31ae0ef0d94e"}


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 5: -Commit-Queue


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 5:

This CL has failed the run. Reason:

Tryjob golang/try/x_tools-gotip-linux-amd64 has failed with summary (view all results):


FAILURE

error: failed to run "test golang.org/x/tools module": exit status 1
failed to run "test golang.org/x/tools/gopls module": exit status 1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 5: LUCI-TryBot-Result-1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

…-wasm

Change-Id: Icb86206377c61384624bfc3d4eb705e36d6dc5b3
@gopherbot
Copy link
Contributor

This PR (HEAD: 0f7d267) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 5:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 5:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 6: Commit-Queue+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 6: -Commit-Queue


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 6:

CV cannot start a Run for mauri870@gmail.com because the user is neither the CL owner nor a committer.


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 6: LUCI-TryBot-Result-1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 6: Code-Review+2 Commit-Queue+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 6:

Dry run: CV is trying the patch.

Bot data: {"action":"start","triggered_at":"2023-11-19T02:27:20Z","revision":"8ccf788a2ec4e83f2a8ffe098b1e0e10e68e6c3a"}


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 6: -Commit-Queue


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go LUCI:

Patch Set 6:

This CL has failed the run. Reason:

Tryjob golang/try/gotip-linux-amd64-newinliner has failed with summary (view all results):


FAILURE

error: shard 4: failed to run "go tool dist test -json": exit status 1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 0d7c396) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/539995.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link
Contributor

Message from Mauri de Souza Meneguzzo:

Patch Set 7:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 7: Code-Review+2 Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7: TryBot-Result+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 7:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from David Chase:

Patch Set 7: Code-Review+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/539995.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Nov 20, 2023
Currently if morestack on g0 happens the wasm runtime prints
"RuntimeError: memory access out of bounds", which is quite misleading.
By switching to a crash stack we can get better stacktraces
for the error.

There is no way to automate tests for this feature on wasm, since
TestG0StackOverflow relies on spawning a subprocess which is not
supported by the wasm port.

The way I got this tested manually is to comment everything in
TestG0StackOverflow, leaving just runtime.G0StackOverflow().

Then it is a matter of invoking the test:

    GOOS=js GOARCH=wasm go test runtime -v -run=TestG0StackOverflow

Change-Id: If450f3ee5209bb32efc1abd0a34b1cc4a29d0c46
GitHub-Last-Rev: 0d7c396
GitHub-Pull-Request: #63956
Reviewed-on: https://go-review.googlesource.com/c/go/+/539995
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/539995 has been merged.

@gopherbot gopherbot closed this Nov 20, 2023
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.

2 participants