Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix bug in impGetStructAddr.#24126

Merged
sandreenko merged 1 commit into
dotnet:masterfrom
sandreenko:fixStmtBug
Apr 20, 2019
Merged

Fix bug in impGetStructAddr.#24126
sandreenko merged 1 commit into
dotnet:masterfrom
sandreenko:fixStmtBug

Conversation

@sandreenko
Copy link
Copy Markdown

@sandreenko sandreenko commented Apr 19, 2019

Fixes https://github.com/dotnet/coreclr/issues/24114.

Thanks @Kaliumhexacyanoferrat and @jkotas for reporting/analyzing this.

The bug was introduced by my change b2f0301. I have not found any similar places where we can hit it.

When I merge this PR I will drop the repro test, wait for several hours and merge it as a separate PR to avoid breaks in other active PRs due to bug in ADO testing that use different head commits for "build" and "test" jobs.

Comment thread src/jit/importer.cpp
// stmt - a statement to insert;
// stmtBefore - an insertion point to insert "stmt" before.
//
inline void Compiler::impInsertStmtBefore(GenTreeStmt* stmt, GenTreeStmt* stmtBefore)
Copy link
Copy Markdown
Author

@sandreenko sandreenko Apr 19, 2019

Choose a reason for hiding this comment

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

This function and impInsertTreeBefore have only one use each. I think they will go away soon so that is why they do not need more checks/cleaning right now.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I trust you on that - though this is a bit confusing and I would have asked for a few more comments otherwise ;-)

@sandreenko sandreenko requested a review from CarolEidt April 19, 2019 23:40
@sandreenko
Copy link
Copy Markdown
Author

PTAL @dotnet/jit-contrib

Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread src/jit/importer.cpp
// stmt - a statement to insert;
// stmtBefore - an insertion point to insert "stmt" before.
//
inline void Compiler::impInsertStmtBefore(GenTreeStmt* stmt, GenTreeStmt* stmtBefore)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I trust you on that - though this is a bit confusing and I would have asked for a few more comments otherwise ;-)

@sandreenko
Copy link
Copy Markdown
Author

Thanks @CarolEidt.

The PR was updated to exclude the test.

@sandreenko sandreenko merged commit 924d4fc into dotnet:master Apr 20, 2019
@sandreenko sandreenko deleted the fixStmtBug branch April 20, 2019 01:06
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AccessViolationException in .NET Core 3.0 Preview 4

2 participants