Skip to content

Commit

Permalink
effects: don't taint :noub for :new allocations (#52222)
Browse files Browse the repository at this point in the history
After #52169, the UB previously associated with allocations with
uninitialized fields has been addressed, so there's no longer a need to
taint `:noub` for `:new` allocations during abstract interpretation.

I believe, even without #52169, uninitialized field does not inherently
leads to UB, but just causes inconsistency of the program, since what
actually causes UB is `getfield` that accesses into whatever object, but
not the allocation itself.
  • Loading branch information
aviatesk authored Nov 19, 2023
1 parent c07893d commit f5d189f
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 10 deletions.
20 changes: 11 additions & 9 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2352,24 +2352,22 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
elseif ehead === :new
t, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], vtypes, sv), true)
ut = unwrap_unionall(t)
consistent = noub = ALWAYS_FALSE
nothrow = false
if isa(ut, DataType) && !isabstracttype(ut)
ismutable = ismutabletype(ut)
fcount = datatype_fieldcount(ut)
nargs = length(e.args) - 1
if (fcount === nothing || (fcount > nargs && (let t = t
has_any_uninitialized = (fcount === nothing || (fcount > nargs && (let t = t
any(i::Int -> !is_undefref_fieldtype(fieldtype(t, i)), (nargs+1):fcount)
end)))
# allocation with undefined field leads to undefined behavior and should taint `:noub`
if has_any_uninitialized
# allocation with undefined field is inconsistent always
consistent = ALWAYS_FALSE
elseif ismutable
# mutable object isn't `:consistent`, but we still have a chance that
# mutable allocation isn't `:consistent`, but we still have a chance that
# return type information later refines the `:consistent`-cy of the method
consistent = CONSISTENT_IF_NOTRETURNED
noub = ALWAYS_TRUE
else
consistent = ALWAYS_TRUE
noub = ALWAYS_TRUE
consistent = ALWAYS_TRUE # immutable allocation is consistent
end
if isconcretedispatch(t)
nothrow = true
Expand Down Expand Up @@ -2411,9 +2409,13 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
end
else
t = refine_partial_type(t)
nothrow = false
end
else
consistent = ALWAYS_FALSE
nothrow = false
end
effects = Effects(EFFECTS_TOTAL; consistent, nothrow, noub)
effects = Effects(EFFECTS_TOTAL; consistent, nothrow)
elseif ehead === :splatnew
t, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], vtypes, sv), true)
nothrow = false # TODO: More precision
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,7 @@ function check_effect_free!(ir::IRCode, idx::Int, @nospecialize(stmt), @nospecia
elseif nothrow
ir.stmts[idx][:flag] |= IR_FLAG_NOTHROW
end
if !isexpr(stmt, :call) && !isexpr(stmt, :invoke)
if !(isexpr(stmt, :call) || isexpr(stmt, :invoke))
# There is a bit of a subtle point here, which is that some non-call
# statements (e.g. PiNode) can be UB:, however, we consider it
# illegal to introduce such statements that actually cause UB (for any
Expand Down

5 comments on commit f5d189f

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

looks like some big regressions in sumcartesian, but I don't have a guess of why

["array", "index", ("sumcartesian", "Matrix{Int32}")]	15.09 (50%) ❌	1.00 (1%)

Please sign in to comment.