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

Fix a segfault in code produced by Visual Studio 2019 #4825

Merged
merged 6 commits into from
Jul 29, 2021
Merged

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Jul 23, 2021

The segfault was discovered by additional tests enabled in #4706.

It looks to have been introduced by #4178 to fix another MSVC issue in debug mode. Instead of reverting that change which would bring up the old debug mode issue, this changes the code to not rely on the mpark::visit(overload pattern with an auto case as this seems to be a combination of code that shows a bug in MSVC 2019 and/or mpark.

I have observed that there are other ways to "fix" the issue, such as removing the lifetime extension idea and adding an explicit case for GlobalKey:

    auto get = overload{
        [this](InternString str) -> PrimaryKey {
            return get_string(str);
        },
        [](const GlobalKey& key) -> PrimaryKey {
            return key;
        },
        [](auto otherwise) -> PrimaryKey {
            return otherwise;
        },
    };
    return mpark::visit(get, key);

But the changes in this PR seem less magical and less prone to similar issues in the future.
This should also fix #4624

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)

@ironage ironage requested review from simonask, jbreams and jedelbo July 23, 2021 21:36
@ironage ironage self-assigned this Jul 23, 2021
@nirinchev
Copy link
Member

Should this revert #4749 and fix it in a similar way?

@ironage
Copy link
Contributor Author

ironage commented Jul 23, 2021

@nirinchev yes good point, I would rather have verbose code like this and reenable the runtime checks in debug mode - in case there is another bug lurking there as well. However, I will wait for one reviewer to approve the initial fix before propagating it elsewhere.

Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

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

I like the more explicit approach.

@ironage ironage requested a review from jedelbo July 27, 2021 02:02
@ironage
Copy link
Contributor Author

ironage commented Jul 27, 2021

I removed the auto&& cases from the visitor pattern which VS 2019 doesn't seem to like. This has a couple of other benefits as well: the compiler will produce an error if we forget to handle a type instead of getting swallowed by the auto case, and also gives us slightly more specific error messages.

},
[&](Obj&, ColKey) {
bad_transaction_log("Invalid path for ArrayInsert (obj, col)");
}};

resolve_path(instr, "ArrayInsert", callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use std::move(callback) here.

},
};
resolve_path(instr, "ArrayErase", callback);
resolve_path(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the same pattern everywhere. Any particular reason not to have a callback variable here?

};

resolve_path(instr, "Clear", callback);
resolve_path(instr, "Clear",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only case where VS 2019 actually didn't work with std::move, and I've added a comment about it and changed the rest to be consistent.

},
[&](Obj&, ColKey) {
bad_transaction_log("Invalid path for SetErase (object, column)");
}};

resolve_path(instr, "SetErase", callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

std::move?

@ironage ironage requested a review from jedelbo July 28, 2021 00:03
@ironage ironage merged commit 11c0051 into master Jul 29, 2021
@ironage ironage deleted the js/msvc2019-fix branch July 29, 2021 15:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack corrupted in Changeset::get_key
3 participants