MemoryWidget: Add negative offset search support #10079
Conversation
sepalani
changed the title
|
I ran into this the other day. Thought it was really stupid that we couldn't do negative offset. +1 |
| @@ -446,7 +446,7 @@ void MemoryWidget::OnSearchAddress() | |||
| bool good_addr, good_offset; | |||
| // Returns 0 if conversion fails | |||
| const u32 addr = m_search_address->text().toUInt(&good_addr, 16); | |||
| const u32 offset = m_search_offset->text().toUInt(&good_offset, 16); | |||
| const s32 offset = m_search_offset->text().toInt(&good_offset, 16); | |||
There was a problem hiding this comment.
note that without casts below, this introduces an implicit sign/unsigned mismatch in terms of arithmetic. Ditto for the other cases as well
|
@lioncash |
| @@ -651,22 +647,53 @@ std::vector<u8> MemoryWidget::GetValueData() const | |||
| return search_for; | |||
| } | |||
|
|
|||
| u32 MemoryWidget::GetTargetAddress(bool* good_address, bool* good_offset) const | |||
There was a problem hiding this comment.
| u32 MemoryWidget::GetTargetAddress(bool* good_address, bool* good_offset) const | |
| u32 MemoryWidget::GetTargetAddress(bool* const good_address, bool* const good_offset) const |
| *good_address |= m_search_address->text().isEmpty(); | ||
| const s32 offset = m_search_offset->text().toInt(good_offset, 16); | ||
| *good_offset |= m_search_offset->text().isEmpty(); | ||
| *good_offset &= offset >= 0 || u32(-offset) <= addr; |
There was a problem hiding this comment.
A few edge cases:
- This wraps addresses from the top of the address space to the bottom but not vice versa.
- I think -offset is undefined behavior if offset == -INT_MIN. It works anyway on my computer, but nasal demons.
- Offsets can't be more than half the size of the address space.
Would it make sense to parse everything as s64 then manually validate the ranges? That would allow everything to have the full range of values that make sense while avoiding signed/unsigned conversion issues.
sepalani
force-pushed
the
neg-offset
branch
from
375d4f3
to
4befa86
Compare
|
@Dentomologist Done. I addressed the offset checks by doing it both way and using |
|
Making the offsets s16 fixes the UB but limits them to +- 32k; I don't know how often people need offsets larger than that, but I would guess that it's not so rare as to not be worth supporting. |
| @@ -57,6 +57,7 @@ class MemoryWidget : public QDockWidget | |||
| void OnDumpFakeVMEM(); | |||
|
|
|||
| std::vector<u8> GetValueData() const; | |||
| u32 GetTargetAddress(bool* good_address, bool* good_offset) const; | |||
There was a problem hiding this comment.
Any reason this doesn't return a struct like:
struct SomeName
{
u32 address = 0;
bool is_good_address = false;
bool is_good_offset = false;
};?
This would get rid of the out parameters and allow structured bindings.
It also gets rid of the uninitialized reads that are present in the existing code from forgetting to initialize the bools before calling the function (since |= is used in the function)
There was a problem hiding this comment.
The struct sounds good to me.
Pedantic: good_address and good_offset are initialized by QString's toUInt and toShort functions before the |=s, but the fact that isn't obvious is a point in favor of the struct.
sepalani
force-pushed
the
neg-offset
branch
from
4befa86
to
6401b8c
Compare
|
@lioncash |
|
This is still limited to +/- 32k offsets. |
sepalani
force-pushed
the
neg-offset
branch
4 times, most recently
from
92cea60
to
45ae9fc
Compare
|
@AdmiralCurtiss |
This PR allows to search/replace value from the Memory widget using a negative offset.
Ready to be reviewed & merged.