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

V8 v12.3 patch #15

Merged
merged 1 commit into from
Apr 16, 2024
Merged

V8 v12.3 patch #15

merged 1 commit into from
Apr 16, 2024

Conversation

StefanStojanovic
Copy link

This patches V8 v12.3 for Windows, by fixing multiple compilation errors caused by V8 being a Clang-oriented project. There are various types of errors fixed by this going from changing using directives and renaming to overcoming the differences in which Clang and MSVC see templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they shouldn't be pushed upstream.

Refs: #13
Refs: #14

@StefanStojanovic
Copy link
Author

@targos this patch doesn't land cleanly on v12.4. I'm working on it, but until it is ready, let's have this one as a backup. As a Node.js collaborator, can you run this through the CI with the request-ci label from here? If it cannot be done, I'll start the run manually.

@targos
Copy link
Owner

targos commented Apr 15, 2024

As a Node.js collaborator, can you run this through the CI with the request-ci label from here? If it cannot be done, I'll start the run manually.

I can't

This patches V8 v12.3 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: targos#13
Refs: targos#14
@targos
Copy link
Owner

targos commented Apr 16, 2024

Could you post a link to the most recent CI run?

@StefanStojanovic
Copy link
Author

Could you post a link to the most recent CI run?

Yes, here it is https://ci.nodejs.org/job/node-test-commit/69900/. It is a resumed build from https://ci.nodejs.org/job/node-test-commit/69886/.

From what I see, there are failures with test-worker-arraybuffer-zerofill which we've already seen in the V8 v12.2 update. There was nodejs@f188183 to address this, but it is not a part of the v12.3 update.

In addition, as seen in one of those 2 builds, but I also saw it previously when testing only Windows, test-error-serdes is showing signs of flakiness as well. We could mark that one as flaky too, if we're gonna mark the test-worker-arraybuffer-zerofill again.

Other than that I see no issues. I had some problems compiling on non-Windows platforms with my fixes, but I resolved that before running those 2 builds.

@targos
Copy link
Owner

targos commented Apr 16, 2024

There was nodejs@f188183 to address this, but it is not a part of the v12.3 update.

The change was in the test directory. It's still here: https://github.com/targos/node/blob/v8-123/test/parallel/parallel.status#L12-L13

@targos
Copy link
Owner

targos commented Apr 16, 2024

Ok, then let's merge this, thank you!

Comment on lines -1352 to +1356
CountLeadingSignBits(k2, rep_w) > k1) {
if (matcher.Get(left).saturated_use_count.IsZero()) {
return __ Comparison(
x, __ WordConstant(base::bits::Unsigned(k2) << k1, rep_w), kind,
rep_w);
} else if constexpr (reducer_list_contains<
ReducerList, ValueNumberingReducer>::value) {
// If the shift has uses, we only apply the transformation if the
// result would be GVNed away.
OpIndex rhs =
__ WordConstant(base::bits::Unsigned(k2) << k1, rep_w);
static_assert(ComparisonOp::input_count == 2);
static_assert(sizeof(ComparisonOp) == 8);
base::SmallVector<OperationStorageSlot, 32> storage;
ComparisonOp* cmp =
CreateOperation<ComparisonOp>(storage, x, rhs, kind, rep_w);
if (__ WillGVNOp(*cmp)) {
return __ Comparison(x, rhs, kind, rep_w);
}
}
CountLeadingSignBits(k2, rep_w) > k1 &&
matcher.Get(left).saturated_use_count.IsZero()) {
return __ Comparison(
x, __ WordConstant(base::bits::Unsigned(k2) << k1, rep_w), kind,
rep_w);
Copy link
Owner

Choose a reason for hiding this comment

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

This change seems non-trivial. What's the effect of removing the else if branch?

Copy link
Author

Choose a reason for hiding this comment

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

That is a revert of this commit. It was also a part of v12.2 fix before.

@targos targos merged commit 4574dfc into targos:v8-123 Apr 16, 2024
1 check failed
targos pushed a commit that referenced this pull request Apr 17, 2024
This patches V8 v12.3 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: #13
Refs: #14
Refs: #15
targos pushed a commit that referenced this pull request Apr 18, 2024
This patches V8 v12.3 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: #13
Refs: #14
Refs: #15
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Apr 19, 2024
This patches V8 v12.3 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: targos#13
Refs: targos#14
Refs: targos#15
PR-URL: #52293
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Apr 19, 2024
This patches V8 v12.3 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: targos#13
Refs: targos#14
Refs: targos#15
PR-URL: #52293
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Apr 19, 2024
This patches V8 v12.4 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: targos#13
Refs: targos#14
Refs: targos#15
StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Apr 19, 2024
This patches V8 v12.4 for Windows, by fixing multiple compilation
errors caused by V8 being a Clang-oriented project. There are various
types of errors fixed by this going from changing `using` directives
and renaming to overcoming the differences in which Clang and MSVC see
templates and metaprogramming.

The changes introduced here are strictly meant as a patch only, so they
shouldn't be pushed upstream.

Refs: targos#13
Refs: targos#14
Refs: targos#15
@StefanStojanovic StefanStojanovic mentioned this pull request Apr 21, 2024
@StefanStojanovic StefanStojanovic deleted the v8-update branch April 24, 2024 07:21
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