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

Offline barbudreadmon

  • Expert
  • *****
  • Posts: 273
  • Karma: +7/-0
  • lr-fbalpha developer
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #885 on: December 22, 2017, 11:28:41 am »
barbudreadmon, oh my... INT32 isn't a long!  you have your define for this type set wrongly, or perhaps, need to define it differently on those platforms.  it should be an unsigned int / 32bit / 4 bytes :)  Obviously, the 32 after INT means something,  right? :)
read here:
http://en.cppreference.com/w/cpp/language/types (thanks jan for the link)

burn.h:
typedef unsigned int                  UINT32;
typedef signed int                     INT32;

okidoki?

bye for now,
- dink
You are right, i assumed int32 was a long but it's a short.
Well, hopefully i'm using the vanilla defines, but this is a fact that adding the alignment macro for those 2 int32 ended up solving the cross platform savestates issue for neogeo/cps, so there definitely seems to be something different with "signed short" on 32/64 bits. Since this is really puzzling, and could be an important information for writing cross platform code in the future, i'll write a little program to test size/alignment of those on both 32 and 64 bits system.

Edit : i confirm size/alignment of INT32 don't differ between 32 and 64 bits system (at least on gcc linux), i overlooked this one but there is also an ALIGN_VAR(8) for the void in ay8910.h, so it is probably this one which solved the issue for those drivers.
« Last Edit: December 22, 2017, 11:44:05 am by barbudreadmon »

Offline jan_klaassen

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #886 on: December 22, 2017, 12:22:35 pm »
Quote
i assumed  int32 was a long but it's a short.
I hope not -- that would make it 16 bits. Again, check using std::numeric_limits if you're not sure.

Offline barbudreadmon

  • Expert
  • *****
  • Posts: 273
  • Karma: +7/-0
  • lr-fbalpha developer
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #887 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

  • Expert
  • *****
  • Posts: 273
  • Karma: +7/-0
  • lr-fbalpha developer
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #888 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #889 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: 2556
  • Karma: +216/-0
  • summer is here!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #890 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #891 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #892 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: 2556
  • Karma: +216/-0
  • summer is here!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #893 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #894 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: 2556
  • Karma: +216/-0
  • summer is here!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #895 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #896 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

  • Member
  • ***
  • Posts: 193
  • Karma: +9005/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #897 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

  • FBA Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #898 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

  • Member
  • ***
  • Posts: 193
  • Karma: +9005/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #899 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 :)