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

codegen: remove UB from uninitialized bitstypes in new #52169

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 14, 2023

In the time since the creation of issue #26764, there is now 'a way to say to llvm "I don't care what this value is, but it always has to be the same"' using the freeze instruction, so we can use that to instruct LLVM to not give us undefined behavior when users are using uninitialized memory. There should not be an impact if users were already avoiding this paradigm and are fully initializing their structs.

Fixes #26764

In the time since the creation of issue #26764, there _is_ now 'a way to
say to llvm "I don't care what this value is, but it always has to be
the same"', so we can use that to instruct LLVM to not give us undefined
behavior when users are using uninitialized memory. There should not be
an impact if users were already avoiding this paradigm and are fully
initializing their structs.

Fixes #26764
@vtjnash vtjnash added the compiler:codegen Generation of LLVM IR and native code label Nov 14, 2023
@vtjnash vtjnash requested a review from gbaraldi November 14, 2023 18:37
@vtjnash
Copy link
Member Author

vtjnash commented Nov 14, 2023

Note that this is already consistent with the way that inference treats these bitstype allocations as being non-pure:

julia> Base.infer_effects(Bad{Int}, ())
(!c,+e,+n,+t,+s,+m,+u)

@vtjnash
Copy link
Member Author

vtjnash commented Nov 16, 2023

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

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

@vtjnash
Copy link
Member Author

vtjnash commented Nov 17, 2023

@nanosoldier runbenchmarks("misc" || "inference" || "rem_pio2", vs=":master")

@nanosoldier
Copy link
Collaborator

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

@vtjnash vtjnash merged commit 81afdbc into master Nov 17, 2023
3 checks passed
@vtjnash vtjnash deleted the jn/codegen-init-noub branch November 17, 2023 23:44
@aviatesk
Copy link
Member

aviatesk commented Nov 18, 2023

Note that this is already consistent with the way that inference treats these bitstype allocations as being non-pure:

julia> Base.infer_effects(Bad{Int}, ())
(!c,+e,+n,+t,+s,+m,+u)

I investigated into this and found this is a bug that we don't propagate :noub information to post-opt analysis

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
# input). Ideally that'd be handled at insertion time (TODO), but for
# the time being just do that here.
ir.stmts[idx][:flag] |= IR_FLAG_NOUB
end

(i.e. :noub is actually tainted during abstract interpretation time but it is refined by post-opt analysis).

I think now we don't need to taint :noub during abstract interpretation time too.

aviatesk added a commit that referenced this pull request Nov 18, 2023
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.
@MasonProtter
Copy link
Contributor

MasonProtter commented Nov 18, 2023

There should not be an impact if users were already avoiding this paradigm and are fully initializing their structs.

Just for clarity, will there be any impact on users who create uninitialized structs? i.e. are there things such as performance implications to this, or does this simply eliminate a family of bugs where LLVM was previously free to swap out bits

Keno pushed a commit that referenced this pull request Nov 19, 2023
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.
@vtjnash
Copy link
Member Author

vtjnash commented Nov 20, 2023

There are performance implications for people who were using uninitialized memory, though that was previously discouraged anyways. Some positive and some negative effects.

@vtjnash vtjnash mentioned this pull request Apr 16, 2024
aviatesk added a commit that referenced this pull request Apr 18, 2024
After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is consistent
always.
aviatesk added a commit that referenced this pull request Apr 18, 2024
After #52169, there is no need to taint `:consistent`-cy on allocations
with undefined `isbitstype` field since the value of the fields is
consistent always.
aviatesk added a commit that referenced this pull request Apr 19, 2024
After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is consistent
always.
aviatesk added a commit that referenced this pull request Apr 19, 2024
After #52169, there is no need to taint `:consistent`-cy on allocations
with undefined `isbitstype` field since the value of the fields is
consistent always.
aviatesk added a commit that referenced this pull request Apr 29, 2024
After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is consistent
always.
aviatesk added a commit that referenced this pull request Apr 29, 2024
After #52169, there is no need to taint `:consistent`-cy on allocations
with undefined `isbitstype` field since the value of the fields is
consistent always.
aviatesk added a commit that referenced this pull request Apr 29, 2024
After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is consistent
always.
aviatesk added a commit that referenced this pull request Apr 29, 2024
After #52169, there is no need to taint `:consistent`-cy on allocations
with undefined `isbitstype` field since the value of the fields is
consistent always.
aviatesk added a commit that referenced this pull request Apr 30, 2024
…-fields (#54136)

After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is freezed
and thus never transmute across multiple uses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantics of Expr(:new) underspecified
5 participants