CodeViewWidget: Fix undefined behavior when centered around address 0

[Pokechu22]

Scrolling defaults to being centered around address 0x00000000, which means addresses before the start are visible (e.g. 0xffffffa8 - 0x00000050). However, std::clamp has undefined behavior if the low value is greater than the high value (in MSVC, this results in a failed assertion in debug mode, and returning the max value in release mode (I think).) This essentially means that using a debug build with the code view widget enabled results in a crash every time.

The new behavior should work correctly, though I'm not 100% sure it gives correct results for a branch from, say, address 0x10 to address 0xfffffff0. That's not something that happens often in practice, so I'm not too worried about it.

[AdmiralCurtiss]

This is still broken for the case first_visible_addr > last_visible_addr. Try eg. a branch from 0x8 to 0x228 (for testing: m_branches.emplace_back(CodeViewBranch{0x8, 0x228, 0, false});). This block will generate nonsense because first_visible_addr is larger than arrow_first_visible_addr/arrow_last_visible_addr:

    const u32 arrow_first_visible_row = (arrow_first_visible_addr - first_visible_addr) / 4;
    const u32 arrow_last_visible_row = (arrow_last_visible_addr - first_visible_addr) / 4;

Which will then result in an out-of-bounds vector access in arrow_space_used.

Maybe just clamp the first/last visible rows too...

---

The wrap-around behavior is questionable but since this didn't work before either and is extremely unlikely to happen in practice it's probably fine to generate bad arrows.

[Pokechu22]

Fixed. Here's the result with this:

  m_branches.emplace_back(CodeViewBranch{0xffffffe0, 0xffffffe4, 0, false});
  m_branches.emplace_back(CodeViewBranch{0xffffffec, 0xffffffe8, 0, false});
  m_branches.emplace_back(CodeViewBranch{0x10, 0xfffffff0, 0, false});
  m_branches.emplace_back(CodeViewBranch{0xfffffff4, 0x0c, 0, false});
  m_branches.emplace_back(CodeViewBranch{0x14, 0x18, 0, false});
  m_branches.emplace_back(CodeViewBranch{0x20, 0x1c, 0, false});

[AdmiralCurtiss]

I'd replace this with is_visible = std::max(arrow_addr_lower, first_addr) <= std::min(arrow_addr_upper, last_addr) -- basically just a range overlap check. That way we properly handle cases where neither arrow end is inside the view.

[Pokechu22]

Changed, but note that the way m_branches is currently calculated means that this doesn't come up in practice (it only contains branches where the source is visible; it doesn't contain ones where the destination is visible but not the source, and it doesn't contain ones where neither of them are visible). That might be worth changing in the future, but the current implementation doesn't handle it.

[AdmiralCurtiss]

True! But if someone were to change this in the future so, say, all branches of the currently visible function(s) were checked, it'll be nice that this works already.

[AdmiralCurtiss]

Otherwise this seems good now.