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.

Sign up for GitHub

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

Jump to bottom

effects: fix correctness issues of :consistent-cy analysis #46111

Merged
merged 2 commits into from
Jul 25, 2022
Merged

effects: fix correctness issues of :consistent-cy analysis #46111

merged 2 commits into from
Jul 25, 2022

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Jul 20, 2022 •
edited
Loading

Today @vtjnash and I found two correctness issues for :consistent-cy:

  • :the_exception should taint :consistent-cy as much like an allocation
  • access to (potentially) undefined field should also taint :consistent-cy
    (in a way that even a return type information can't later refine it)

@aviatesk aviatesk requested a review from Keno July 20, 2022 03:04
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Jul 20, 2022
@Keno
Copy link
Member

Keno commented Jul 20, 2022

Seems like the uninitialized field access should only be an issue for us it's fields? Otherwise you gen an undef ref error, which is fine from a consistency perspective.

@aviatesk
Copy link
Sponsor Member Author

Seems like the uninitialized field access should only be an issue for us it's fields?

You mean isbitstype field, right?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 20, 2022

Yes, but we also just usually hope people don't leave undef fields lying around, so that it won't matter and doesn't need to complicate this

@KristofferC KristofferC mentioned this pull request Jul 20, 2022
Merged
32 tasks
@aviatesk
Copy link
Sponsor Member Author

Okay, let's not bother with it for now but just leave a comment why we want to ignore it.

@aviatesk aviatesk force-pushed the avi/correct-consistent branch 3 times, most recently from 79236ce to 1140fc2 Compare July 20, 2022 15:41
@Keno
Copy link
Member

Keno commented Jul 20, 2022

I don't think we can really afford to ignore that complication. undef isbitstype fields are pretty strongly undef (#26764), so this could definitely lead to crashes if we don't taint consistency there.

@aviatesk aviatesk mentioned this pull request Jul 20, 2022
Merged
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 20, 2022

That should be the point of this PR (to taint everything)?

aviatesk added a commit that referenced this pull request Jul 20, 2022
@aviatesk
inference: guard error of fieldtype in :new expression handling
c185f44 
Separated from #46111.
This really doesn't matter usually though as the frontend doesn't form
`:new` expression anyway.
@aviatesk aviatesk mentioned this pull request Jul 20, 2022
Merged
@aviatesk aviatesk changed the base branch from master to avi/guard-fieldtype July 20, 2022 20:20
aviatesk added a commit that referenced this pull request Jul 20, 2022
@aviatesk
inference: simplify error-case handling within abstract_eval_statement
0ad5816 
Separated from #46111.
aviatesk added a commit that referenced this pull request Jul 20, 2022
@aviatesk
inference: simplify error-case handling within abstract_eval_statement
87bd4d3 
Separated from #46111.
aviatesk added a commit that referenced this pull request Jul 21, 2022
@aviatesk
inference: simplify error-case handling within abstract_eval_statement
4d84e6c 
Separated from #46111.
Base automatically changed from avi/guard-fieldtype to master July 21, 2022 14:49
@aviatesk aviatesk force-pushed the avi/correct-consistent branch 3 times, most recently from 9494922 to 43a05c0 Compare July 21, 2022 16:46
Keno
Keno reviewed Jul 22, 2022
View reviewed changes
base/compiler/tfuncs.jl Outdated
function getfield_notundefined(@nospecialize(obj), @nospecialize(name))
if !isa(name, Const)
if !hasintersect(widenconst(name), Union{Int,Symbol})
# always throw doesn't account for undefined
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I think we don't need to taint :consistent-cy when a getfield call is determined to throw. If the thrown object (with a potentially undefined field) is caught later, we will taint it anyway when we see :the_exception expression.

Copy link
Member

Choose a reason for hiding this comment

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

That is correct

Keno
Keno reviewed Jul 22, 2022
View reviewed changes
base/compiler/tfuncs.jl Outdated
@@ -1817,6 +1855,10 @@ function builtin_effects(f::Builtin, argtypes::Vector{Any}, @nospecialize(rt))
end
s = s::DataType
consistent = !ismutabletype(s) ? ALWAYS_TRUE : ALWAYS_FALSE
# access to undefined field leads to undefined behavior and should taint `:consistent`-cy
Copy link
Member

Choose a reason for hiding this comment

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

Make a note that undefined access of non-isbitstype would be ok, but we model it conservatively for simplicitly?

Keno
Keno approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

LGTM, but there are some test failures that look related.

@aviatesk aviatesk force-pushed the avi/correct-consistent branch 3 times, most recently from 1e1fe6c to e9b560e Compare July 23, 2022 04:14
@aviatesk aviatesk merged commit 8a8e3bf into master Jul 25, 2022
@aviatesk aviatesk deleted the avi/correct-consistent branch July 25, 2022 14:20
aviatesk added a commit that referenced this pull request Jul 26, 2022
@aviatesk
Merge pull request #46111 from JuliaLang/avi/correct-consistent
1645d1d 
effects: fix correctness issues of `:consistent`-cy analysis
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aviatesk @Keno @vtjnash @KristofferC