wasm: fix "thread_in_wasm flag was not set" crash in debug builds. by PiotrSikora · Pull Request #16132 · envoyproxy/envoy

PiotrSikora · 2021-04-22T23:07:02Z

Signed-off-by: Piotr Sikora piotrsikora@google.com

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

repokitteh-read-only · 2021-04-22T23:07:07Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16132 was opened by PiotrSikora.

see: more, trace.

PiotrSikora · 2021-04-23T08:36:58Z

/retest

repokitteh-read-only · 2021-04-23T08:37:02Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16132 (comment) was created by @PiotrSikora.

see: more, trace.

howardjohn

Its definitely possible I built it wrong as I had some difficulty, but I tested this and it seemed to have the same issue.

PiotrSikora · 2021-04-23T15:50:59Z

Its definitely possible I built it wrong as I had some difficulty, but I tested this and it seemed to have the same issue.

Thanks for testing! It's definitely possible that this PR doesn't fix the issue (I didn't replicate it locally yet), it's a cherry-pick of a fix that looked relevant, but perhaps there are even more issues around that code. I'll try to dig into it next week.

PiotrSikora · 2021-04-29T09:13:56Z

OK, this indeed doesn't work. The failure can be easily replicated using:

bazel test -c dbg //test/extensions/common/wasm:wasm_test

It passes on the CI, because it only uses dbg in tests with sanitizers, and trap handlers are disabled with sanitizers.

I think it might be a false-positive, but I filled the issue with V8 team:
https://bugs.chromium.org/p/v8/issues/detail?id=11713

…rapfix

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-05-04T05:41:57Z

@howardjohn if it's not too much trouble, could verify the updated version? It fixes the issue for me.

howardjohn · 2021-05-13T00:56:36Z

@PiotrSikora I was able to test this and confirmed it addressed the issue

…rapfix Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-05-13T02:05:53Z

@moderation I know that you're not a big fan of carrying patches, but debug builds are completely broken right now. Those patches will be removed once we update to V8 v9.2, but it won't happen until the end of next week when v9.2 is branched.

moderation · 2021-05-13T03:25:14Z

Just like I'm not a fan of carrying patches, you are not a fan of putting a TODO(PiotrSikora) in the patch, with a date / target for resolution, so we know who we can go to when the patch blocks a dependency upgrade 😄

…rapfix

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-05-14T09:06:20Z

Just like I'm not a fan of carrying patches, you are not a fan of putting a TODO(PiotrSikora) in the patch, with a date / target for resolution, so we know who we can go to when the patch blocks a dependency upgrade 😄

I've added TODO without a date or resolution, since there is nothing to do until the next version is branched and we update to it. I'm not a fan of adding TODO in this case, since patches are already clearly commented with the source of the cherry-picks, and adding it to the existing PRs triggers another multi-hour CI cycle, which in turn delays approval process and merge by another few days, while providing virtually zero value (historically speaking, I'm going be doing the update anyway).

PiotrSikora · 2021-05-14T10:59:21Z

/retest

repokitteh-read-only · 2021-05-14T10:59:24Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16132 (comment) was created by @PiotrSikora.

see: more, trace.

PiotrSikora · 2021-05-15T08:11:07Z

@moderation PTAL.

moderation · 2021-05-15T16:52:43Z

/lgtm deps

antoniovicente · 2021-05-20T05:49:36Z

bazel/external/wee8.patch

 # 2. Increase VSZ limit to 64 TiB (allows us to start up to 6,553 VMs).
 # 3. Fix building and linking with MSAN.
+# TODO(PiotrSikora): remove when not needed anymore (most likely in v9.2 branch):
+# 4. Fix "thread_in_wasm flag was not set" crash in debug builds (https://chromium-review.googlesource.com/c/v8/v8/+/2817598, https://chromium-review.googlesource.com/c/v8/v8/+/2867468).


The changes listed are part of the diff below. LGTM!

antoniovicente

Anything to add @lizan ?

These changes LGTM.

@envoyproxy/senior-maintainers: would one of you want to take a look or should we merge this fix?

lizan · 2021-05-21T21:29:25Z

I merged v8 9.2, do we still need this?

PiotrSikora · 2021-05-21T23:18:03Z

I merged v8 9.2, do we still need this?

Nope. Thanks!

wasm: fix "thread_in_wasm flag was not set" crash in debug builds.

b8fbad3

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 22, 2021

PiotrSikora requested a review from howardjohn April 23, 2021 08:44

lizan requested a review from mathetake April 23, 2021 08:49

howardjohn reviewed Apr 23, 2021

View reviewed changes

ggreenway assigned lizan May 3, 2021

PiotrSikora added 2 commits May 3, 2021 20:08

Merge remote-tracking branch 'origin/main' into PiotrSikora/v8_v9.0-t…

9fac42d

…rapfix

review: cherry-pick another fix.

308267c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora mentioned this pull request May 13, 2021

WASM test failure with debug image istio/istio#30645

Closed

Merge remote-tracking branch 'origin/main' into PiotrSikora/v8_v9.0-t…

92eb104

…rapfix Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora marked this pull request as ready for review May 13, 2021 02:03

PiotrSikora requested review from lizan and moderation May 13, 2021 02:03

PiotrSikora added 2 commits May 14, 2021 08:44

Merge remote-tracking branch 'origin/main' into PiotrSikora/v8_v9.0-t…

7ad0df3

…rapfix

review: add TODO.

f17cd45

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mathetake approved these changes May 15, 2021

View reviewed changes

PiotrSikora mentioned this pull request May 15, 2021

build: Avoid use of fastbuild when building with gcc to work around command line limit #16492

Closed

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 15, 2021

moderation approved these changes May 15, 2021

View reviewed changes

antoniovicente reviewed May 20, 2021

View reviewed changes

antoniovicente assigned mathetake May 20, 2021

antoniovicente approved these changes May 20, 2021

View reviewed changes

antoniovicente self-assigned this May 20, 2021

PiotrSikora closed this May 21, 2021

Conversation

PiotrSikora commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 22, 2021

Uh oh!

PiotrSikora commented Apr 23, 2021

Uh oh!

repokitteh-read-only bot commented Apr 23, 2021

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Apr 23, 2021

Uh oh!

PiotrSikora commented Apr 29, 2021

Uh oh!

PiotrSikora commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

howardjohn commented May 13, 2021

Uh oh!

PiotrSikora commented May 13, 2021

Uh oh!

moderation commented May 13, 2021

Uh oh!

PiotrSikora commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrSikora commented May 14, 2021

Uh oh!

repokitteh-read-only bot commented May 14, 2021

Uh oh!

PiotrSikora commented May 15, 2021

Uh oh!

moderation commented May 15, 2021

Uh oh!

antoniovicente May 20, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented May 21, 2021

Uh oh!

PiotrSikora commented May 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PiotrSikora commented Apr 22, 2021 •

edited

Loading

PiotrSikora commented May 4, 2021 •

edited

Loading

PiotrSikora commented May 14, 2021 •

edited

Loading