Exception#set_backtrace accept arrays of Backtrace::Location

[casperisfine]

[Feature #13557]

Setting the backtrace with an array of strings is lossy. The resulting exception will return nil on #backtrace_locations.

By accepting an array of Backtrace::Location instance, we can rebuild a Backtrace instance and have a fully functioning Exception.

NOTE: Exception's instance variables id_bt and id_bt_locations are a bit inconsistent right now and would deserve a cleanup.

[KJTsanaktsidis]

Looks reasonable to me.

NOTE: Exception's instance variables id_bt and id_bt_locations are a bit inconsistent right now and would deserve a cleanup.

Agree this could use some love. Why do we even have both given that exc_backtrace seems happy enough for either strings or Thread::Backtrace::Locations to be in id_bt?

[KJTsanaktsidis]

I think it's unnecessary to guard this if you're returning it since btobj obviously must either be in the stack or register state for the entirety of this frame since it's being returned?

[casperisfine]

Yeah, I started to be a bit liberal with GC_GUARD...

[KJTsanaktsidis]

The more I learn about how compilers work the more scared I am that on day some compiler will do something really clever like store a pointer to the middle of a struct in a register, rather than the actual VALUE, and break everything :/

I wonder if anybody has tried compiling Ruby with LTO, maybe aggressive enough cross-TU inlining could make this happen…

[casperisfine]

Why do we even have both given that exc_backtrace seems happy enough for either strings or Thread::Backtrace::Locations to be in id_bt?

Yeah, that surprised me as well, I'd need to do some spelunking to see when it was introduced.

[casperisfine]

NOTE: Exception's instance variables id_bt and id_bt_locations are a bit inconsistent right now and would deserve a cleanup.

So I dug a bit more into this since various Exceptions and Marshal related tests were failing.

I used the following test script:

begin
  raise "foo"
rescue => exc
end
p [:orig, exc.backtrace, :bt, exc.bt, :bt_locations, exc.bt_locations]

begin
  raise StandardError, "second", ["second_line"]
rescue => exc2
end
p [:raise, exc2.backtrace, :bt, exc2.bt, :bt_locations, exc2.bt_locations]

exc.set_backtrace("foo")
p [:set_backtrace, exc.backtrace, :bt, exc.bt, :bt_locations, exc.bt_locations]

[:orig, ["./test.rb:2:in '<main>'"], :bt, #<Thread::Backtrace:0x00000001037321d0>, :bt_locations, #<Thread::Backtrace:0x00000001037321d0>]
[:raise, ["second_line"], :bt, ["second_line"], :bt_locations, nil]
[:set_backtrace, ["foo"], :bt, ["foo"], :bt_locations, #<Thread::Backtrace:0x00000001037321d0>]

So it's a bit weird, but bt is either the original Backtrace or an array of strings, and bt_locations is either nil if the exception never had a real backtrace in the first place, or it's original Backtrace.

This would be hard to cleanup because a bunch of test asserts on the Marshal format of Exception, so changing the type of either ivar would break backward compatibility. And in the end leaning into that format, really cut the PR size, so...

[github-actions]

Try on Playground: https://ruby.github.io/play-ruby?run=8278867369
This is an automated comment by pr-playground.yml workflow.

[ko1]

def foo
  raise
end

begin
  foo
rescue => e
  e.set_backtrace(caller_locations + [""])
end

=begin
-e:8:in 'Exception#set_backtrace': wrong argument type String (expected frame_info) (TypeError)

e.set_backtrace(caller_locations + [""])
^^^^^^^^^^^^^^^^^^^^^^^
from -e:8:in '<main>'
from -e:5:in '<main>'
-e:2:in 'Object#foo': unhandled exception
from -e:6:in '<main>'
=end

----

def foo
  raise
end

begin
  foo
rescue => e
  e.set_backtrace([""] + caller_locations)
end

=begin
-e:8:in 'Exception#set_backtrace': backtrace must be an Array of String or an Array of Thread::Backtrace::Location (TypeError)

e.set_backtrace([""] + caller_locations)
^^^^^^^^^^^^^^^^^^^^^^^
from -e:8:in '<main>'
from -e:5:in '<main>'
-e:2:in 'Object#foo': unhandled exception
from -e:6:in '<main>'
=end

can we make them same error?

'Exception#set_backtrace': backtrace must be an Array of String or an Array of Thread::Backtrace::Location (TypeError)

[casperisfine]

can we make them same error?

I think so, I'll try.

[casperisfine]

Alright, the error message is now consistent. I'll merge when CI pass.

[ko1]

Could you modify docs and maybe NEWS?

[casperisfine]

Could you modify docs and maybe NEWS?

Of course: #10255

Let me know what you think.