RIP-Comm / clementine

Gameboy Advance emulator.
MIT License
49 stars 13 forks source link

Small fixes in thumb/bx #138

Closed AlessioC31 closed 1 year ago

AlessioC31 commented 1 year ago
codecov-commenter commented 1 year ago

Codecov Report

Base: 48.20% // Head: 46.88% // Decreases project coverage by -1.31% :warning:

Coverage data is based on head (a30654a) compared to base (91cf3bc). Patch coverage: 1.81% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #138 +/- ## ========================================== - Coverage 48.20% 46.88% -1.32% ========================================== Files 30 30 Lines 1670 1717 +47 ========================================== Hits 805 805 - Misses 865 912 +47 ``` | [Impacted Files](https://codecov.io/gh/RIP-Comm/clementine/pull/138?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [emu/src/arm/arm7tdmi.rs](https://codecov.io/gh/RIP-Comm/clementine/pull/138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZW11L3NyYy9hcm0vYXJtN3RkbWkucnM=) | `60.45% <0.00%> (-1.63%)` | :arrow_down: | | [emu/src/arm/data\_processing.rs](https://codecov.io/gh/RIP-Comm/clementine/pull/138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZW11L3NyYy9hcm0vZGF0YV9wcm9jZXNzaW5nLnJz) | `72.35% <ø> (ø)` | | | [emu/src/arm/instruction.rs](https://codecov.io/gh/RIP-Comm/clementine/pull/138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZW11L3NyYy9hcm0vaW5zdHJ1Y3Rpb24ucnM=) | `40.00% <0.00%> (-33.92%)` | :arrow_down: | | [emu/src/arm/opcode.rs](https://codecov.io/gh/RIP-Comm/clementine/pull/138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZW11L3NyYy9hcm0vb3Bjb2RlLnJz) | `52.00% <0.00%> (ø)` | | | [emu/src/arm/psr.rs](https://codecov.io/gh/RIP-Comm/clementine/pull/138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZW11L3NyYy9hcm0vcHNyLnJz) | `75.00% <25.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

guerinoni commented 1 year ago

Mostly LGTM, I don't understand if for reading PC we should read 32 bit with first (and in case second) to 0 or if we should read only other 31 (or in case 30)

AlessioC31 commented 1 year ago

Mostly LGTM, I don't understand if for reading PC we should read 32 bit with first (and in case second) to 0 or if we should read only other 31 (or in case 30)

That's a good question, I don't know :)

But I think we should read 32 bits and have the last/latest two to 0. This is because this is from the fact that instructions are word/halfword aligned (arm/thumb), so for example when we access an ARM instruction we will always have the latest two bits to 0. If we take the bits 2-31 as a PC to access the memory, we would still have the problem that this address may have the latest two bits != 0b00, so it would be possible for it to read instructions not word-aligned. So I think we should use the full 32 bits as the address of the instruction, but always setting the latest two bits/one bit to 0.

guerinoni commented 1 year ago

Make sense also to me :)