MelonDS System Bus

[SuuperW]

This PR allows access to 16-bit and 32-bit reads/writes on the system bus domains for melonDS, resolving #3121. This is done in 2 parts:

1. Create a new type of waterbox memory domain. It is similar to the function hook domain, except this one exposes 6 function hooks for various sizes of read and write operations.
2. Change the system bus domains to the new type.

The behavior of bulk byte operations remains unchanged compared to 2.11.

This bit left in from the original version of this PR:
Regarding hex editor, I don't think there are any great solutions. With this PR, access size in hex editor can be controlled by changing the viewed data size. This is not intuitive. Using sized system bus domains would require the user to change domains, and also leads to the question of what should be done if the user writes a single byte in 1-byte data size mode, to a 32-bit domain? I guess this would have to first read the relevant 4 bytes, change the one byte, then write 4 bytes again. Fortunately, none of this matters for the vast majority of addresses so it's probably not very important as long as it is possible to perform sized access.

Check if completed:

• I have run any relevant test suites
• I, the commit author, have read the licensing terms for contributors (last updated 2024-06-22) and am compliant

[CasualPokePlayer]

This does not give a way to specify to waterbox function hook domains the write size to use. MelonDS will now infer it.

This is very much the wrong way to do it. If I'm specifying a bulk read of multiple bytes (e.g. memory.read_bytes_as_array), the behavior shouldn't change just because I decided to read a different amount or depending on where I decided to start the read. Byte at address X returns what address X has when you peek that byte with PeekByte.

Lua users cannot obviously specify access size

They can, there are a ton of lua functions specifically for this.

C# users cannot obviously specify write size (as MemoryDomain has no such functions)

They can, MemoryDomain does have such functions (that's the entire point of the different poke functions).

Another option would be to provide new function hook domains for 16 and 32 bit access.

You don't need new domains, the function hook just needs to be modified to specify access size and that access size must be sent over for the core to understand how to read/write things.

Ideally different widths shouldn't be changing results in the first place, but the System Bus is inherently quirky to the console (since it is more or less native bus reads), so if different widths change results those are arguably just system quirks which are inherent to the System Bus domain, so under that logic it's acceptable for System Bus domains.

[SuuperW]

the behavior shouldn't change just because I decided to read a different amount or depending on where I decided to start the read.

Just to be clear - for the vast majority of addresses, the behavior does not change depending on alignment or number of bytes read.

They can, there are a ton of lua functions specifically for this.

With the current implementation memory.read_u32_le(0x02000001) will internally do 4 single-byte reads while memory.read_u32_le(0x02000000) will do 1 4-byte read. Similar for read_bytes_as_array and for the C# API. This doesn't matter for most addresses. Except, if we do a 4-byte read with read_u32_le then reading from address 0x02000001 would ALWAYS return the same value as reading from address 0x02000000 since the system bus aligns multi-byte reads and writes. Not a bad thing, but surprising to most people.

You don't need new domains, the function hook just needs to be modified to specify access size and that access size must be sent over for the core to understand how to read/write things.

I didn't want to try modifying every other core that uses function hooks. Although I suppose it could instead be a second kind of function hook. I could do that.

And if we do it this way, the Lua bulk read functions could similarly have a parameter for access size. And corresponding C# API functions...? There's already BulkPeekUshort and BulkPeekUint but no multi-byte bulk poke functions. Maybe Lua should follow suit and have multiple functions instead of adding a parameter to the existing one. That probably makes more sense actually, a new read_u32_le_array would return a table with 32-bit numbers instead of separating each byte.

those are arguably just system quirks which are inherent to the System Bus domain, so under that logic it's acceptable for System Bus domains

Yes, that is my view. IMO there should also be a way to perform system bus reads that have side effects. Maybe this could be accomplished with a new API method that enables read side effects. Making read side effects opt-in would protect users from accidentally causing desyncs.

[YoshiRulz]

Can I veto any new peek or poke APIs until #3472 and/or #3738? At least in ApiHawk; I guess if there's an asymmetry in the Lua API that should be filled.

[SuuperW]

The first one I will look at later. I don't see the relevance of the second, nor would I wish to attempt to implement it in the near future.

[SuuperW]

While reviewing Yoshi's PR, I found this in BaseImplementations/MemoryDomain.cs lines 115-116:

			if ((start & 1) != 0 || (end & 1) != 0)
				throw new InvalidOperationException("The API contract doesn't define what to do for unaligned reads and writes!");

I didn't know this was here, and it doesn't make sense. Why would the bulk read option require aligned access when individual reads do not? And this is for generic memory domains too, where we don't even know if (a) the emulated system cares about memory alignment or (b) 16/32-bit values are always or even usually aligned.

The commit message indicates it's related to Hex Editor ("the only consumer of the API"), which I assume can never perform bulk operations un-aligned because there just isn't a UI option to do that. So this code probably belongs in Hex Editor logic, and not in memory domain logic.

[YoshiRulz]

Someone probably noticed that one of the cores' implementations choked on unaligned access and, rather than add the necessary guards and logic there, forced alignment on everyone.

[CasualPokePlayer]

Unaligned access is complete UB on many platforms. That includes even dereferencing with like *(int*) natively. In practice on modern machines unaligned access usually "just works" with the most UB involved just being on the language constructs (although this typically comes with a minor speed penalty and of course actual raw I/O probably won't like unaligned access). For the machines we emulate many different behaviors could occur (especially relevant for the System Bus): you could just have the same result as if you read 1 byte at a time ("just works"), you could have force alignment (low address bits masked off), sometimes you get some rotated result, sometimes you get garbage, sometimes the machine would just crash / throw a processor exception.

[SuuperW]

Force pushed with a new approach.

[SuuperW]

Rebased, split into two PRs (#4626), and fixed a couple minor issues pointed out by @CasualPokePlayer.

[SuuperW]

Somehow those minor fixes didn't make it into the last force push, so force push again.

[SuuperW]

Any objections to merging this?

[YoshiRulz]

Follow the project code style.

[SuuperW]

Can we get a consensus about your code style issue? (#4463)
IMO, the way I've done it is easier to read.

[YoshiRulz]

Missing braces is only one of the problems. There's also missing spaces after keywords and casts, arguments split to multiple lines but multiple on one line (this might be undocumented as well), and incomplete ArgumentException creation. I'd also like to see explicit argument names and blank lines between member declarations. You can use primary constructor syntax for StubResolver to keep the line count down.

[SuuperW]

Where are these style rules? None of those things are mentioned in the short style segment of contributing.md. If they're not written down or checked (by the checks run on GitHub), they should be considered to not exist imo.

Skimming the changed files, I do not see "arguments split to multiple lines but multiple on one line" (not that I think this is necessarily bad) or any incomplete ArgumentExceptions.

And spaces between a cast typename and the thing being casted is ugly. Idk why you'd want that. I could add spaces after fixed though.

For explicit argument names, is this something you want everywhere? Or are there specific places you think would benefit?

And why blank lines between member declarations? I can see wanting to separate unrelated members, but for example all of the MemoryDomainSizedAccessStubs in WaterboxMemoryDomainSizedFuncs I think should stay compact.

[YoshiRulz]

Spaces after keywords:
(csharp_space_after_keywords_in_control_flow_statements is unset so defaults to true)

Spaces after casts:

BizHawk/.editorconfig

Line 14 in 848eb78

csharp_space_after_cast = true

Comma-separated expressions:

BizHawk/.global.editorconfig.ini

Lines 707 to 718 in 848eb78

	# Opening parenthesis or bracket should be on declaration line
	dotnet_diagnostic.SA1110.severity = silent
	# Closing parenthesis should be on line of last parameter
	dotnet_diagnostic.SA1111.severity = silent
	# Parameter list should follow declaration
	dotnet_diagnostic.SA1114.severity = silent
	# Split parameters should start on line after declaration
	dotnet_diagnostic.SA1116.severity = silent
	# Parameters should be on same line or separate lines
	dotnet_diagnostic.SA1117.severity = silent
	# Parameter should not span multiple lines
	dotnet_diagnostic.SA1118.severity = silent

They are disabled and I think miss this edge case, but the style is widely followed.

ArgumentException:

BizHawk/.global.editorconfig.ini

Lines 236 to 237 in 848eb78

	# Specify the parameter name in ArgumentException
	dotnet_diagnostic.MA0015.severity = error

(+ CA2208 which is unset so defaults to suggestion-level)

Explicit argument names:
No rule enabled, but generally if you're able to swap two arguments and have it compile, then one or both should have an explicit name.

Lines between declarations:

BizHawk/.global.editorconfig.ini

Lines 835 to 838 in 848eb78

	# Closing brace should be followed by blank line
	dotnet_diagnostic.SA1513.severity = silent
	# Element documentation header should be preceded by blank line
	dotnet_diagnostic.SA1514.severity = warning

BizHawk/.global.editorconfig.ini

Lines 841 to 842 in 848eb78

	# Elements should be separated by blank line
	dotnet_diagnostic.SA1516.severity = silent

I think I've had them muted from the start because of the high number of warnings it would add, and because our code style was (and still is) vertically sparse already.

I can see wanting to separate unrelated members, but for example all of the MemoryDomainSizedAccessStubs in WaterboxMemoryDomainSizedFuncs I think should stay compact.

Sure, you can make an exception for those cases.

[YoshiRulz]

Suggested change

	_read8.Access((IntPtr)(&ret), addr, 1);
	_read8.Access(buffer: (IntPtr) &ret, address: addr, count: 1);

[SuuperW]

csharp_space_after_cast = true

...but why. Code is much easier to visually parse when casts don't have a space after. The commit message you made says the why is "Stop IDEs suggesting to remove the space between cast type and operand", and in that case it should be a thing you do locally, not something you push to everyone else. If there's a rule you can set severity for in a .editorconfig, you can set it to silent (and you could even do that in an untracked file).
And if this is just a personal preference of yours that you committed without consulting other devs, I'm not particularly inclined to follow it.

Explicit argument names:
No rule enabled, but generally if you're able to swap two arguments and have it compile, then one or both should have an explicit name.

I disagree. When the names of the variables being given (or other context) make it clear what's what, readability is harmed by adding explicit parameter names, not helped. You might say it's not for readability, it's to help with writing it. To which I would say, that isn't a reason to change code that's already correct.

Comma-separated expressions ... edge case ... the style is widely followed

I'm pretty sure I've seen things like what I did in other places, but can't remember any off the top of my head. This is one thing I wouldn't really mind changing though.

vertically sparse

I don't like how vertically sparse a lot of code is, too much scrolling. That's also why I often like to omit braces when omitting them doesn't hurt readability.

Looking at code changes, I also see I have put some || operators at the ends of lines. IIRC you prefer them at the start and I don't really care. Should I change those too? (static_asserts in BizDebugging.cpp)

[YoshiRulz]

I did enable the rule for operators like || to appear after the line break in C#, but we don't have linting set up for C++ and I never touch it so idrc either.

This is not the place to bikeshed over which particular style is the most readable; I want uniformity because that allows it to be automated away, reducing the workload on devs. And I'm sick of reminding you in every PR.

[SuuperW]

Readability is more important than uniformity. And, importantly, not at all the same thing. And uniformity is not a realistic goal. There will always be edge cases and little inconsistencies, and pushing too hard will just irritate people.

What workload are you trying to reduce? If uniformity is a goal because of the potential for automation, then lack of uniformity was not a thing placing a workload on devs.

I'm sick of it too, by the way.

[SuuperW]

Made some of the requested style changes. (just not the ones I've argued against, which at least are consistent with the style in the rest of the file)