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

[1.10] Function returns wrong value instead of throwing an error #53062

Closed
vchuravy opened this issue Jan 26, 2024 · 7 comments
Closed

[1.10] Function returns wrong value instead of throwing an error #53062

vchuravy opened this issue Jan 26, 2024 · 7 comments
Assignees
Labels
correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing

Comments

@vchuravy
Copy link
Member

julia> function f1(cond)
           x = Ref{Int}(0)
           if cond
               x[] = x
           else
               return -1
           end
       end
f1 (generic function with 1 method)

julia> @code_typed f1(true)
CodeInfo(
1 ─ %1 = %new(Base.RefValue{Int64}, 0)::Base.RefValue{Int64}
└──      goto #3 if not cond
2 ─      invoke Base.setindex!(%1::Base.RefValue{Int64}, %1::Base.RefValue{Int64})::Union{}
└──      unreachable
3 ─      return -1
) => Int64

julia> f1(true)
-1

julia> f1(false)
-1

julia> @code_llvm optimize=false f1(true)
;  @ REPL[1]:1 within `f1`
define i64 @julia_f1_305(i8 zeroext %0) #0 {
top:
  %1 = call {}*** @julia.get_pgcstack()
  %2 = bitcast {}*** %1 to {}**
  %current_task = getelementptr inbounds {}*, {}** %2, i64 -14
  %3 = bitcast {}** %current_task to i64*
  %world_age = getelementptr inbounds i64, i64* %3, i64 15
  ret i64 -1
}

julia> Base.code_ircode(f1, (Bool,))
1-element Vector{Any}:
2 1 ─ %1 = %new(Base.RefValue{Int64}, 0)::Base.RefValue{Int64}        │╻╷ Ref
3 └──      goto #3 if not _2                                          │  
4 2 ─      invoke Base.setindex!(%1::Base.RefValue{Int64}, %1::Base.RefValue{Int64})::Union{}
  └──      unreachable                                                │  
6 3 ─      return -1                                                  │  
   => Int64                                                   

This seems fixed on nightly and on 1.9, but not on 1.10

@aviatesk maybe you can take a look? I am very confused by the fact that code_ircode seems to give the right answer, but by the time we end up at LLVM someone replaced it all with a constant.

@aviatesk
Copy link
Member

Yeah, I'll look into this today.

@aviatesk aviatesk self-assigned this Jan 26, 2024
@oscardssmith
Copy link
Member

is this #52856?

@vchuravy
Copy link
Member Author

It looks different enough... and there is no embedded value, the value is created through new

@nsajko nsajko added the correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing label Jan 26, 2024
@aviatesk
Copy link
Member

It appears to be a bug in the LLVM optimization level. The the Julia level IR seems fine. The code gets incorrectly optimized on LLVM level:

julia> code_typed(f1, (Bool,))
1-element Vector{Any}:
 CodeInfo(
1%1 = %new(Base.RefValue{Int64}, 0)::Base.RefValue{Int64}
└──      goto #3 if not cond
2 ─      invoke Base.setindex!(%1::Base.RefValue{Int64}, %1::Base.RefValue{Int64})::Union{}
└──      unreachable
3return -1
) => Int64

julia> code_llvm(f1, (Bool,))
;  @ REPL[2]:1 within `f1`
define i64 @julia_f1_758(i8 zeroext %0) #0 {
top:
  ret i64 -1
}

@vchuravy
Copy link
Member Author

@aviatesk it's before LLVM

julia> @code_llvm optimize=false f1(true)
;  @ REPL[1]:1 within `f1`
define i64 @julia_f1_305(i8 zeroext %0) #0 {
top:
  %1 = call {}*** @julia.get_pgcstack()
  %2 = bitcast {}*** %1 to {}**
  %current_task = getelementptr inbounds {}*, {}** %2, i64 -14
  %3 = bitcast {}** %current_task to i64*
  %world_age = getelementptr inbounds i64, i64* %3, i64 15
  ret i64 -1
}

@aviatesk
Copy link
Member

Ah, I see. This seems to be another effects analysis bug:

julia> Base.infer_effects((Bool,)) do cond
           x = Ref{Int}(0)
           if cond
               x[] = x
           else
               return -1
           end
       end

aviatesk added a commit that referenced this issue Jan 27, 2024
This particular fix was part of #50805, but it wasn't included in
version 1.10, leading to situations where an incorrect `:nothrow` could
occur in 1.10 (#53062). This commit implements a minimal correction in
1.10 and also added some test cases.

Fixes #53062.
@aviatesk
Copy link
Member

Will be fixed by #53076.

aviatesk added a commit that referenced this issue Jan 27, 2024
The fix for #53062 was included in #50805, but it lacked explicit test
cases added. This commit cherry-picks the test cases from #53076 to
prevent future regression.
aviatesk added a commit that referenced this issue Jan 27, 2024
The fix for #53062 was included in #50805, but it lacked explicit test
cases added. This commit cherry-picks the test cases from #53076 to
prevent future regression.
aviatesk added a commit that referenced this issue Jan 29, 2024
The fix for #53062 was included in #50805, but it lacked explicit test
cases added. This commit cherry-picks the test cases from #53076 to
prevent future regression.
aviatesk added a commit that referenced this issue Jan 29, 2024
This particular fix was part of #50805, but it wasn't included in
version 1.10, leading to situations where an incorrect `:nothrow` could
occur in 1.10 (#53062). This commit implements a minimal correction in
1.10 and also added some test cases.

Fixes #53062.
Drvi pushed a commit to RelationalAI/julia that referenced this issue Jun 7, 2024
This particular fix was part of JuliaLang#50805, but it wasn't included in
version 1.10, leading to situations where an incorrect `:nothrow` could
occur in 1.10 (JuliaLang#53062). This commit implements a minimal correction in
1.10 and also added some test cases.

Fixes JuliaLang#53062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing
Projects
None yet
Development

No branches or pull requests

4 participants