Call mem callbacks with addr, value

[brian-armstrong]

No description provided.

[Asnivor]

i saw some IRC chat regarding this, but can we get some more background on this? Purpose, benefits, etc?

[vadosnaprimer]

Does it support callback type too (read/white/execute)?

[brian-armstrong]

My intended use case for this is that I'd like my external tool to be able to get a full trace of all the memory addresses being written to. I am doing some memory exploration on a game and I want to make an interactive tool that can build a set of all the addresses that the game writes to during some State A, and then start capturing a new trace and play the game to state B. That way I can find which addresses might be associated with the transition from A to B by subtracting the first set from the second.

I could also get this by scanning the full memory every frame but it's a little clumsy. In practice it looks like games don't write to memory super often and so this isn't impractical to attach to with a tool.

zeromus suggested widening the whole thing to support address and value so that's what I did here.

I like the idea of adding the callback type in - I assume you mean telling the callback which type of memory op occurred?

[vadosnaprimer]

EmuHawk stores memory callbacks as 3 types (read, write, or execute), but there's no way to tell what it is if you just look into them, this info isn't kept internally. So if the callback triggers something, that something can't know why it was triggered. I found this out when checking if debugger can be made to work. Whenever an event occurs, there's no way for debugger to know which memory action it is attached to. I tried adding this by turning Action into Action<something>, but never finished. It should be easy to add while you're at it already.

[brian-armstrong]

So just to clarify, should I go ahead and add the type to this PR?

[vadosnaprimer]

Sure!

[adelikat]

I don't want to merge this PR without type added.

[brian-armstrong]

Sure, I haven't been in front of my Windows computer recently so haven't had time for this, but I can try to close this out soon.

[zeromus]

Yes, this is a great idea. Please convey the size of the operation while touching all this stuff.

The naive way to do this is probably with an integer saying the size.

Actually, I think that's good--it will give us the ability to mask other information into there later if we need to. For instance, indicate which CPU provoked the access. In fact, I mention this because, maybe it would be better if we went ahead and "dirtied" all of these parameters so that nobody could even use them without masking out the size. For instance you could send 32 as the size, but actually send 32|0x100 so that nobody would be able to use it without masking the size by 0xFF. People would just have to know to do that. (The reason for going ahead and doing this now is that it would let us add flags later without breaking anything, since everyone would already be doing the masking)

And in this case, maybe it shouldn't be called size but maybe flags (which happens to include the size).

How problematic it is it to do this masking in callbacks in lua? I tried to make it easier in my proposal above by not turning the sizes into bits but rather leaving them as integers in the low byte.

Pinging @nattthebear since he may care

[vadosnaprimer]

I love the bit-field idea!

[vadosnaprimer]

@brian-armstrong hi, how is it going? We're looking forward to your update!

[brian-armstrong]

Hello, I could swing back around and take this, though I don't think I understand what @zeromus is suggesting. I'd like to make sure we agree on the spec before I start so that I can make sure I get mergeable code next time.

[vadosnaprimer]

I think he wants 3 uints to come with every Action. One would be address, one would be value, and one would be flags. It's a very neat solution for all the possible info we might want to provide/obtain. Here's how it looks in practice, in GPGX where I recently sent this functionality:
ekeeke/Genesis-Plus-GX@d1e7cd6
The difference is that in my PR, size/width is its own variable, but it can't be unreasonably big, so we can just shove it into flags instead. Then all we have to do is defining which flags mean what in the core. So cores could be sending masked bits of info, and client would mask them out to get the info based on the convention. The best part is that you don't have to invent this convention. You only need the variable that will be used for that later.

tl;dr: Action<uint, uint, uint>

[brian-armstrong]

A few questions from the top of my head:

How wide is the widest size we care about? I assume these generally can't be wider than a few bytes, but maybe 32 bits of size just to be safe, which leaves 32 bits for flags, assuming a 64-bit size argument.

What do you think about using R/W/Exec as our first two bit flags? Could do something like 00/01/10 respectively.

How much do we care about lua backwards compat, going on what @zeromus mentioned? We could send lua fewer args to not break anything here, I think?

[vadosnaprimer]

How wide is the widest size we care about? I assume these generally can't be wider than a few bytes, but maybe 32 bits of size just to be safe, which leaves 32 bits for flags, assuming a 64-bit size argument.

You don't need to match 32 bits of the size with 32 bits of the uint you're passing. If width is 64 bits, all you have to pass is the number "64", which occupies only 7 bits. With 8 bits being used, you can pass the width of 255 bits. I don't even know if any system we emulate uses this much.

What do you think about using R/W/Exec as our first two bit flags? Could do something like 00/01/10 respectively.

That's the intent I guess, but I think coming up with this convention is a future goal. There could be things like "enabled" and "forbidden", some of which might make sense to go before type.

How much do we care about lua backwards compat, going on what @zeromus mentioned? We could send lua fewer args to not break anything here, I think?

Lua is aware which callback type triggers it, so I don't know what could theoretically break in that regard. It's not like passing type would lead to removing the r/w/x arrays and slapping all callbacks together. If it is going to happen, it'll be a future decision anyway.

[zeromus]

32bits of size is overkill. we may as well bow to the 64bit overlords and make everything ulongs.
I wouldn't worry about breaking compatibility here, as it would be easy to fix any affected script.

I dont know if anyone else thinks it's valuable to keep the size as an integer rather than an enum. It wastes bits as an integer, but not enough to matter.

As for the flags... we have some kind of a precedent for this. It would be prudent to point it out:

http://tasvideos.org/Bizhawk/CodeDataLogger.html

The CDLogger needs and the lua callbacks needs should be similar. My main point is that when you're truly supporting/representing everything, there's way too much special case stuff to come up with a general solution.

That said, I think we could go ahead and define the obvious common fields. Let's suppose the "common fields" are provided as a best effort for general use and other bits may be used later as extended enumeration values for more detailed inquiries. So.. 2 bits for R/W/X is a fine start (any core that can provide more information will still do its best to categorize it as R/W/X). But I'd say let's go ahead and make that 4 bits, just in case. Make 4 bits for the CPU number, too, and 8 bits for the memory domain. Even though you might register these callbacks on a cpu/memdomain scope, I think it would be nice to identify the event through all the bits. That way one callback can be used to handle multiple event types, if the user wishes, and the event type being understood by inspecting the flags. Rather than being obliged to have separate callbacks for every purpose. This could have added value in the future by giving us some mechanism to experiment with event masking inside the core, so that the script could have finer control over what it got sent, possibly bypassing the memorydomains completely. Only time would tell if this is a better way for setting up callbacks.

So to be clear, the script might say
mask = CPU_MASK | ACCESS_MASK | WIDTH_MASK
test = CPU_0 | ACCESS_RW | WIDTH_16

and that would get reads&writes on any memdomain from cpu 0

So, I would say, [ x...x | XXXXXXXX XXXXWWWW AAAACCCC MMMMMMMM ]
Where x = undefined, W is width, A is access-type, C is cpu number, and M is memdomain number. Later when someone needs to we can allocate undefined bits for the real special case stuff like "first or second operand byte"

But for now... we just want to make sure we have the plumbing to support this, while we're revising it.

[vadosnaprimer]

While it's obviously fine to suggest the convention right now, I believe it shouldn't be included in this PR. Having it separated would allow to discuss in details what we want and why, without delaying this PR and without scattering the info between different tickets.

[brian-armstrong]

Agree that 32 bits for length would be overkill! So then we could pass a 64-bit int here and reserve something 40-48 bits for flags, or pass a 32-bit int and reserve maybe 24 bits for flags?

Which flags do we feel would be essential for merging this? I was lucky in that the initial pass with just addr, value did not require a lot of knowledge about individual cores to implement, but the new flags and size likely will. I'd like to limit feature creep and get to something I could reasonably finish in the next week or so. Though I understand that this isn't super useful to consumers that will care about the flags until it has them.

Regarding masking, I'm of the opinion that, at least for SNES games, having the consumer attach to every single memory access was actually not terribly burdensome CPU wise, and a consumer could probably feasibly do its own masking. I suppose this would be different for newer systems but attempting to implement a complex masking system in BizHawk seems like the kind of thing that makes everyone unhappy :)

[zeromus]

You dont have to understand it all, nor implement it on all the cores. I propose you return this from the cores:

[31:24] 00000000
[23:16] 00000000 (byte) / 00000001 (word) / 00000002 (long)
[15:8] FFFF0000 (AAAACCCC -> A=$F=invalid currently; C=$0=cpu 0 currently)
[7:0] FFFFFFFF (memdomain = $FF=invalid currently)

In other words, set the flags to one of
0x00000000:0000F0FF //byte
0x00000000:0001F0FF //word
0x00000000:0002F0FF //long

the rationale being, users cant get any information without masking it, so once the new system is in place we can fiddle with providing more information without breaking anyone

[brian-armstrong]

Ah, I see, so size is just an enum then. That will certainly save some bytes. I expected there were DMA commands or weird rep-like instructions that would move arbitrary numbers of bytes, but this is much easier.

I think I see what needs to be done. I will try to implement the passing in BizHawk itself and something approximately right on the cores. Maybe we could also use xFFFF to denote an invalid size for now, since I suspect knowing the size on each core requires some knowledge of addressing modes.

[zeromus]

dma commands and rep-like instructions appear to the memory subsystem as bytes, words, or longs. this does not present a problem.

I find it hard to believe you won't know the right size while hooking these up, but in any case where you don't know, yeah, use $F on the width to denote invalid.

I would like @nattthebear to chime in an OK or at least a "meh, whatever". I'm sure I haven't thought through everything we'd need in the future, but I want to make sure I didn't make a dumb mistake.

[nattthebear]

For something that's used in many places and part of a defined API, could we make our own delegate type for this instead of Action<...>? This would let us name the parameters and provide xmldocs that intellisense could pick up on, which makes things easier to grok.

[nattthebear]

I think this is all going in a useful direction. I personally don't give one shit about old lua script compatibility, but others might not share that view.

[vadosnaprimer]

As I said, Lua will only get affected if the current system gets revamped not to keep callbacks separated by type as it does now. Such a revamp is clearly outside the scope of this PR.

[brian-armstrong]

I'm making good progress on this but I've noticed a couple things that could use clarification.

• Each core calls CallReads or equivalent for the type of memory access it is doing currently. Should we just replace all of these with a single CallCallbacks since the memory access type will now be encoded in the flags? I think the client side of this might not change for now since BizHawk can just record the type of callback the client wants and then inspect the flag, but we could also go ahead and change the client side to pass a flag mask as was mentioned here. I think that wouldn't be a ton of work to implement but might break existing consumers unless we keep the old functions.

• 0x00 is seeming to me to be a better value for unknown/unspecified, rather than 0xFF, since it means generators of callbacks can simply start by sending 0 for their callbacks, and in general it's less surprising. If we add another flag later, callback creators will automatically generate "unknown" values correctly until they can be updated to encode the right info.

[zeromus]

1. If you're touching that code anyway to change the arguments to CallReads and CallWrites you may as well change it to CallCallbacks(readmode and CallCallbacks(writemode --if that's straightforward. The whole exercise here is to do whatever's simple while youre touching everything.
2. 0xFF was chosen specially to prevent people from making equality checks for 8,16,32 but instead alerting them that it was intended as bitfields. However with the latest approach I gave, with 0,1,2 being byte,word,long, inside the middle of the value, nobody's going to assume things like that anyway, so go ahead and use 0x00 for undefined.

[brian-armstrong]

Re: first point, I think I can add those correctly, though I think there might be subtle distinctions on when a write is also a read? Or does a read/modify/write instruction first generate a read and then a write? And is an Exec also always a read?

[zeromus]

a RMW always generates a read and then a write.
An exec is not always a read. You should keep track of things as exec/fetches as long as you can (especially in the c++ sides), until you get to a point where the framework doesnt understand exec/fetch; then, turn it into a read so that existing things keep working

[brian-armstrong]

I'm feeling pretty good about this latest commit although apparently it broke in AppVeyor - it built locally. I would like to test it a bit more but I think this is at a good place for review, and I'll get those last AppVeyor errors fixed (appears I missed a couple of cores).

Edit: All fixed! Looks like I needed to sync with master.

[brian-armstrong]

What's left to get this merged? This patchset does use Enum HasFlag in 3 places, I can switch those to basic bitwise logic if that's the only remaining blocker. I'd prefer to keep utility functions out of this PR, let's get it merged :)

[brian-armstrong]

Gentle reminder for above comment, let me know if there's a better way to chat about the PR. I have some time tomorrow so I'd like to try to finish up whatever's left, or get as close as I can anyway :)

[zeromus]

i’m not blocking it for hasflags or utility functions. i don’t like blocking PR at all. even if it’s bugged, commit it and fix it.

[vadosnaprimer]

@brian-armstrong might be worth it to discuss the details in #bizhawk on freenode.