Bail out if we get an invalid wrapper to avoid an inifinite loop here.

[AlexEne]

If you have code like the one below and set a break-point on the first println! line with code lldb you end up getting your whole editor stuck due to the loop i just changed.

This is also related to also the problem described here: llvm/llvm-project#196812
After this PR it will show the wrong strong / weak counts for the Rc (both 0), but at least doesn't freeze the debug session.

#[test]
fn test_lldb_fail() {
    struct Holder {
        rc1: std::rc::Rc<u32>,
        rc2: std::rc::Rc<u32>,
        rc3: std::rc::Rc<u32>,
    }
    let cell: std::cell::Cell<u64> = std::cell::Cell::new(0);
    let holder = Holder {
        rc1: std::rc::Rc::new(1),
        rc2: std::rc::Rc::new(2),
        rc3: std::rc::Rc::new(3),
    };
    println!("breakpoint here: {} {}", cell.get(), holder.rc1);
    println!("Second one");
}

I reproduced it in Zed, on arch, but I suspect many editors work the same. codelldb attempts a cancellation for stuck responses, but it only cancels one so it doesn't help for the example above.

Further on, I will follow up with a codelldb PR as well to cancel checks inside the struct / field summaries, instead just the top-level variables (to further harden these cases).

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @Mark-Simulacrum

[Mark-Simulacrum]

@Walnut356, do you have an opinion on whether a test is worth it for this?

[Walnut356]

Hmm, tbh I think unwrap_scalar_wrappers is still a bit flimsy. What it's trying to do is a bit roundabout considering we're working with integer types. I'm also not a huge fan of returning plausible (but incorrect) information without any kind of warning. SBValue can be thought of as a pointer to the debugee memory, so we should be able to just cast the weak and strong valobjs to unsigned long long rather than unwrapping. Then if those end up invalid, we can output a dummy value and print a warning.

Rc is barely tested at all atm anyway (for good reason, atm it often reads OoB memory if the allocator doesn't 0 the portion after any u8/i8 data), so we can probably skip a test for now. The plan is to have a single, much more robust test for each type we have a visualizer for. Once I get the tests running on windows and linux in CI (which shouldn't be too much longer), and wrap up gsoc stuff, it's next on the agenda.

[Walnut356]

Suggested change

	def __init__(self, valobj: SBValue, _dict: LLDBOpaque, is_atomic: bool = False):
	self.valobj = valobj
	self.ptr = unwrap_unique_or_non_null(self.valobj.GetChildMemberWithName("ptr"))
	self.value = self.ptr.GetChildMemberWithName("data" if is_atomic else "value")
	# infallibly gets an unsigned integer type of at least 64 bits. We don't need to worry about
	# whether or not `usize` is actually smaller than that since we don't ever display the
	# underlying type to the user anyway
	usize_type = valobj.GetTarget().GetBasicType(eBasicTypeUnsignedLongLong)
	self.strong = self.ptr.GetChildMemberWithName("strong").Cast(usize_type)
	self.weak = self.ptr.GetChildMemberWithName("weak").Cast(
	usize_type
	)
	def num_children(self) -> int:
	# Actually there are 3 children, but only the `value` should be shown as a child
	return 1
	def get_child_index(self, name: str) -> int:
	if name == "value":
	return 0
	if name == "strong":
	return 1
	if name == "weak":
	return 2
	return -1
	def get_child_at_index(self, index: int) -> Optional[SBValue]:
	if index == 0:
	return self.value
	if index == 1:
	return self.strong
	if index == 2:
	return self.weak
	return None
	def update(self):
	pass

Along the lines of this, where we only operate on SBValue objects

View changes since the review

[Walnut356]

Suggested change

	strong = valobj.GetChildMemberWithName("strong")
	weak = valobj.GetChildMemberWithName("weak")
	if not (strong.IsValid() and weak.IsValid()):
	strong = "?"
	weak = "?"
	print("Unable to read ref count, see llvm/llvm-project#196812")
	else:
	strong = strong.GetValueAsUnsigned()
	weak = weak.GetValueAsUnsigned()
	return "strong={}, weak={}".format(strong, weak)

And then return some kind of dummy value and error message.

Also we can subtract 1 from weak here before printing it to the user if that's desirable. The existing Rc synthetic modifies the child itself, but that's not super necessary since it also hides that child from the user.

It's maybe not entirely necessary though because the weak count does start at 1, but iunno.

View changes since the review

[AlexEne]

Should we merge this first before changing more of it? I would bias towards some minimal changes to fix the infinite loop and then can iterate on the behavior? What do you think?

[Walnut356]

Fundamentally, i think this sort of looping behavior is always going to be finicky and unreliable. Whether we patch the infinite loop then delete the loop, or just delete the loop immediately isnt a huge deal to me.

I do think we need some kind of warning and dummy value for sure though. A debugger outputting incorrect dara is worse than one that outputs nothing at all.

[AlexEne]

It's not that debuggers don't output things but they get stuck making it impossible to even debug.

I will close this as it seems there is a different direction / cleanup you may want to take it and I don't have bandwidth to test and iterate on suggestions. So, I'll focus on getting the codelldb PR vadimcn/codelldb#1413 over the line instead as it's more valuable if it can defend against these type of infinite loop bugs in visualizers.