Author Topic: FB Alpha 0.2.97.43 Bug Reports  (Read 479390 times)

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #885 on: December 22, 2017, 01:17:50 PM »
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.

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #886 on: December 23, 2017, 05:01:04 AM »
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].opbase
It seems to work but i've got a feeling my code is horrible. Any suggestion ?
« Last Edit: December 23, 2017, 05:15:13 AM by barbudreadmon »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #887 on: December 23, 2017, 06:53:42 AM »
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.

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #888 on: December 23, 2017, 09:05:05 AM »
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

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #889 on: December 23, 2017, 09:17:44 AM »
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.

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #890 on: December 24, 2017, 12:45:03 PM »
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.

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #891 on: December 24, 2017, 08:08:14 PM »
d_powerins.cpp:
SCAN_VAR(RamCurPal);

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

Thanks for the heads-up.

best regards,
- dink

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #892 on: December 26, 2017, 08:08:02 AM »
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).

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #893 on: December 26, 2017, 08:41:45 AM »
Nice, use this to find more bugs for us, and leave our burn / drivers alone! edit: please.
« Last Edit: December 26, 2017, 09:05:55 AM by dink »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #894 on: December 26, 2017, 08:53:01 AM »
Thanks, Dink. Christmas spirit in action, I guess.

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

Offline vbt

  • FBNeo Contributor
  • *****
  • Posts: 205
  • Karma: +9005/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #895 on: December 26, 2017, 11:39:05 AM »
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?

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #896 on: December 26, 2017, 03:12:15 PM »
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.

Offline vbt

  • FBNeo Contributor
  • *****
  • Posts: 205
  • Karma: +9005/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #897 on: December 28, 2017, 06:36:02 PM »
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 :)

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #898 on: December 28, 2017, 06:40:10 PM »
Yes, It would be nice to stay on topic --> "FB Alpha 0.2.97.42 Bug Reports"

best regards,
- dink

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #899 on: February 13, 2018, 05:45:11 PM »
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)