Topic: FB Alpha 0.2.97.43 Bug Reports

[barbudreadmon]

I hope not -- that would make it 16 bits. Again, check using std::numeric_limits if you're not sure.

My bad, i mean an int.

[barbudreadmon]

The cross-platform issue with sh2 savestates is that opbase in sh2ext is 4 byte on 32 bits and 8 byte on 64 bits. In libretro-fba 39 someone commented the

Code: [Select]

SCAN_VAR (Sh2Ext[i].opbase) in sh2.cpp. It seems commenting it again solve the cross-platform issue, but i suppose it is there for a reason.

After reading jan's posts, i tried replacing the type by uint8_t, the size would still differ.

So i tried replacing the declaration/code by this :

Code: [Select]

diff --git a/src/cpu/sh2/sh2.cpp b/src/cpu/sh2/sh2.cpp
index a257404..1649fa2 100644
--- a/src/cpu/sh2/sh2.cpp
+++ b/src/cpu/sh2/sh2.cpp
@@ -179,7 +179,7 @@ typedef struct
  pSh2ReadLongHandler ReadLong[SH2_MAXHANDLER];
  pSh2WriteLongHandler WriteLong[SH2_MAXHANDLER];
 
- unsigned char * opbase;
+ ALIGN_VAR(8) unsigned char * opbase;
  int suspend;
 } SH2EXT;
 
@@ -3508,7 +3508,7 @@ int Sh2Scan(int nAction)
  Sh2Ext[i].sh2.irq_callback = irq_callback;
 
  SCAN_VAR (Sh2Ext[i].suspend);
- SCAN_VAR (Sh2Ext[i].opbase);
+ ScanVar(& (Sh2Ext[i].opbase), 8, "Sh2Ext[i].opbase" );
 
 #if FAST_OP_FETCH
  // Sh2Ext[i].opbaseIt seems to work but i've got a feeling my code is horrible. Any suggestion ?

[jan_klaassen]

That's because Sh2Ext.opbase's type is not uint8_t, but uint8_t*.

Pointers have no business being in any savestate. In fact at first glance doesn't look like opbase needs to be included at all.

More generally, in cases where you do need to include pointers, which will be rare, you will need to find a way to convert them to something that's not a pointer, even if that's by using pointer arithmetic and forced alignment.

[dink]

Just remove/comment this line:  SCAN_VAR (Sh2Ext.opbase);
it's recalculated on state load, and doesn't need to be scanned in the first place.

best regards,
- dink

[jan_klaassen]

In order to detect that sort of problem, here's a bit of code that replaces the SCAN_VAR macro; insert it into state.h replacing the #define SCAN_VAR line:

Code: [Select]

#if !defined(__cplusplus) || ((_MSC_VER < 1900) && (__cplusplus <= 199711L))

#define SCAN_VAR(x) ScanVar(&x, sizeof(x), #x)

#else

class scan_exception
{
const char* const _what_arg;

public:

const char* const variable;

scan_exception(const char* const what_arg, const char* const variable) : _what_arg(what_arg)
, variable(variable)
{
return;
}

const char* const what()
{
return _what_arg;
}
 };

#define SCAN_VAR(x) { if (std::is_pointer<decltype(x)>::value) throw scan_exception("variable is a pointer!", #x); else ScanVar(&x, sizeof(x), #x); }

#endif
Also insert #include <type_traits> in burnint.h:

Code: [Select]

#if defined(__cplusplus) && !((_MSC_VER < 1900) && (__cplusplus <= 199711L))

#define _USE_MATH_DEFINES
#include <type_traits>

#endif
If you use this, trying to include a pointer in a savestate will cause an exception to be thrown. Note that it's only compiled if your compiler has C++11 support enabled, does not work for code that's compiled as plain C, and your compiler may complain about exceptions being thrown where it doesn't expect them.

Other than that, see screenshot.

[jan_klaassen]

I make a nicer version. The new one uses static asserts and sfinae to detect problems at compile time and do the right thing in 32/64 bit versions. I also built in support for pointers into some part of the game's memory or an array (only neo geo uses those atm).

Direct support for ptrdiff_t and (u)intptr_t to come.

Compiling threw up errors in two drivers that dump pointers into savestates:

d_powerins.cpp:
SCAN_VAR(RamCurPal);

d_galpanic.cpp:
SCAN_VAR(RamCurPal);
SCAN_VAR(RamCTB64k);

They should not use SCAN_VAR, since they need to save the contents of RAM and not a pointer.

[dink]

d_powerins.cpp:
SCAN_VAR(RamCurPal);

d_galpanic.cpp:
SCAN_VAR(RamCurPal);
SCAN_VAR(RamCTB64k);

Thanks for the heads-up.

best regards,
- dink

[jan_klaassen]

Final update on area/variable scanning:

• included a macro and c++ functions for scanning memory, much like the one for variables
• better const-correctness, full const-correctness will require changes to all drivers, but as a start...
• burnArea struct has constructors easily allowing use of const char* const etc, wich requires...
• all of burn is compiled as c++, which allows...
• no more c linkage except for the burn <-> burner interface and external libraries
• both c++97 and c++11 versions of scanning functions
• c++11: separate header file
• c++11: type-safe support for pointers pointing inside a driver's data (e.g. memory banking, used by neo geo)
• c++11: compile time test for incompatible types (except in structs which is impossible to detect in the current standard)
• c++11: 64bit builds have fixups for some types (pointers, ptrdiff_t, size_t, (u)intptr_t)
• c++11: 64bit builds have runtime checks that throw exceptions (indicating driver bugs)

Tested and working with both visual studio and gcc (except 64bit, so until that code is tested it fails with a static assert).

[dink]

Nice, use this to find more bugs for us, and leave our burn / drivers alone! edit: please.

[jan_klaassen]

Thanks, Dink. Christmas spirit in action, I guess.

Is this how you treat everyone who contributes, or am I I just special?

[vbt]

Thanks, Dink. Christmas spirit in action, I guess.

Is this how you treat everyone who contributes, or am I I just special?

Just curious Jan, you got your own version of FBA or it's another emulator based on FBA?

[jan_klaassen]

Just curious Jan, you got your own version of FBA or it's another emulator based on FBA?

It's FBA, though it has a lot of new things for the Windows UI and is slowly diverging otherwise. It's also a work in progress. That wasn't my choice, but as you can see this situation is probably going to remain the status quo.

[vbt]

It's FBA, though it has a lot of new things for the Windows UI and is slowly diverging otherwise. It's also a work in progress. That wasn't my choice, but as you can see this situation is probably going to remain the status quo.

try to make a jan's WIP topic, it will be easier to follow

[dink]

Yes, It would be nice to stay on topic --> "FB Alpha 0.2.97.42 Bug Reports"

best regards,
- dink

[barbudreadmon]

I'm not sure it should be called a bug, but there doesn't seem to be multitap support in fbalpha megadrive.
Also, perhaps it is worth mentioning there were several models of multitap, with different compatibilities (see Team Player and 4 Way Play)