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

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #855 on: December 10, 2017, 03:52:27 AM »
barbudreadmon, Just a guess, but this might be the problem:
src/cpu/m68k/m68kops.c:8228:16: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

code: uint mask = 1 << (OPER_I_8() & 0x1f);

uint is defined as "unsigned int" in m68kcpu.h.  that should be ok.  1<<31 or 1<<0x1f is the top bit in a 32bit int, so that should also be ok.  But if it can't set the top bit when it needs to - there's going to be a problem.
try to get rid of that error.  Some things I can think of... uint mask = ((uint) 1 << (OPER_I_8() & 0x1f)); .. maybe?  Does it work if you disable optimization for the 68k cpu?  if not, try disabling optimization for everything that neogeo requires (68k, z80, snd/*2610*, fm.c, drv/neogeo/*) and see if that helps. 

bye for now,
- dink
It removed the ubsan message, but it didn't solve the issue. I already tried disabling all optimization. I also made sure differences in codebase between lr-fbalpha and fbalpha were not the root of this, and there was no difference apart from the "s1945pmode" hotfix and a change to "if (nNeoSMARNGAddress[nNeoActiveSlot][0] > 0 || nNeoSMARNGAddress[nNeoActiveSlot][1] > 0) {" in "NeoSMABankswitch", which i think is part of your current svn version (tried reverting to original .42 code, didn't help), there were no change in 68k, z80, 2610 and fm code.
At this point, 2 things comes to mind :
- An issue in the libretro frontend part of the code, not finding similar issues in other games is weird though
- An issue in the codebase for non standard x86_32 platform, for cross-platform compatibility reasons lr-fbalpha is built without fastcall (works only on x86_32), x86_asm (same ?), mmx (x86 only), and c++11 (no compatible compiler for ios, ps3, ...)

I'll try building fbalpha standalone without speedhack, fastcall, x86_asm, mmx and c++11 to see if i can reproduce this issue.
« Last Edit: December 10, 2017, 03:55:20 AM by barbudreadmon »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #856 on: December 10, 2017, 05:41:24 AM »
FWIW, my older builds have the same problem in karnovr with a different Z80 core. But blazstar's intro music is only missing in the new ones, just as the regular fba release.

So it looks like there are several problems. My builds have the same neo geo code as the older ones. The timers haven't changed, but the Z80 and YM2610 cores have, as has the Z80 interface code. So, karnovr's problem is probably in neo_run.cpp somewhere. but blazstar's isn't, at least not directly.

c++11 (no compatible compiler for ios, ps3, ...)
Say what? IOS has had a current version version of CLang for years, included in XCode. PS3 is another matter entirely,  though unofficial devkits definitely support C++11, and much of C++14.

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #857 on: December 10, 2017, 06:40:53 AM »
Say what? IOS has had a current version version of CLang for years, included in XCode. PS3 is another matter entirely,  though unofficial devkits definitely support C++11, and much of C++14.
Well, i am kinda sure being asked to remove c++11 dependency for ios compatibility, it was probably 1 or 2 years ago though, perhaps it was for older device (support for newer ios version was dropped for iphone 4 and below a few years ago if i remember well).
Considering my experience as a web developper with apple devices (safari doesn't support a lot of standard w3c stuff other browser have been supporting for 5+ years), and the fact opengl 4.2+ support (2011) was still missing a few months ago (and perhaps is still missing) on mac os x, i never thought it was strange.
There are probably other platforms not compatible with c++11, iirc lr-fbalpha is also used on psvita, 3ds, wiiu, ...

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #858 on: December 10, 2017, 08:31:01 AM »
barbudreadmon, if there is some way you could make this problem happen in the standalone version, I could help better. 
p,s, also add xbox to the list of those that don't do current c++. 

best regards,
- dink

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #859 on: December 10, 2017, 09:56:34 AM »
Well, i am kinda sure being asked to remove c++11 dependency for ios compatibility, it was probably 1 or 2 years ago though, perhaps it was for older device (support for newer ios version was dropped for iphone 4 and below a few years ago if i remember well).
Yeah, apple is fairly ruthless where backward compatibility is concerned (c++11 support arrived before that cutoff, though).

Quote
Considering my experience as a web developper with apple devices (safari doesn't support a lot of standard w3c stuff other browser have been supporting for 5+ years), and the fact opengl 4.2+ support (2011) was still missing a few months ago (and perhaps is still missing) on mac os x, i never thought it was strange.
Don't get me talking about that... I do mostly back-end web stuff, but still sometimes the refusal of the browser makers (not just Apple) to even acknowledge big parts of the standards makes me want to throw stuff.

Quote
There are probably other platforms not compatible with c++11, iirc lr-fbalpha is also used on psvita, 3ds, wiiu, ...
Right, I'm not expecting platforms that were discontinued and unsupported by 2011 to ever support c++11. My approach is to not use c++11 in burn, but some stuff I write for other parts of fba relies on it extensively.

Anyway, good luck with the sound problem.

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #860 on: December 10, 2017, 11:37:40 AM »
I couldn't reproduce it on standalone, but i noticed something while testing : on my video, when i reach the title screen by "inserting coin", the sound from inserting this first coin is weird, i hear it twice (i pressed coin button only once to reach the title screen). On standalone, i hear it once. I tried a couple of other neogeo games on lr-fbalpha and didn't notice anything similar, so i was wondering if it's not a hint to the issue ?
I was wondering if it couldn't be related to something in the code defining audio segment length and audio buffer in my libretro port.

My audio buffer is declared like this :
Code: [Select]
static int16_t *g_audio_buf;(that's the type the libretro api audio callback is asking for)

I have a macro defined like this :
Code: [Select]
#define AUDIO_SAMPLERATE 48000
Before running BurnDrvInit i do this :
Code: [Select]
nAudSegLen = (AUDIO_SAMPLERATE * 100 + (6000 >> 1)) / 6000;
g_audio_buf = (int16_t*)malloc(nAudSegLen<<2 * sizeof(int16_t));
pBurnSoundOut = g_audio_buf;
nBurnSoundRate = AUDIO_SAMPLERATE;
nBurnSoundLen = nAudSegLen;

After running it i do this :
Code: [Select]
nAudSegLen = (AUDIO_SAMPLERATE * 100 + (nBurnFPS >> 1)) / nBurnFPS;
g_audio_buf = (int16_t*)malloc(nAudSegLen<<2 * sizeof(int16_t));
nBurnSoundRate = AUDIO_SAMPLERATE;
nBurnSoundLen = nAudSegLen;
(redefining nBurnSoundRate was probably not necessary)

Anything in this is likely to cause an issue ?
« Last Edit: December 10, 2017, 11:39:49 AM by barbudreadmon »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #861 on: December 10, 2017, 11:54:04 AM »
Your calculation for the number of samples in  a frame is fine (identical to mine).

In your case, I would see if the coin sound is triggered twice because it's getting the coin insert inputs twice, the 68k is sending the sound command twice, and so on.

Offline radius

  • New Member
  • *
  • Posts: 2
  • Karma: +0/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #862 on: December 19, 2017, 07:21:06 PM »
Hi,

I'm part of libretro, on 0.2.97.39 I did a lot of research with aliaspider to fix savestates between popular platforms for some popular machines (because netplay).
Turns out several states included pointers and we added some padding which in turn fixed the issue.

barbudreadmon backported 0.2.97.40 and now everything seems broken again:
- CPSx states get fudged between 64-bit and 32-bit platforms (music glitches mostly)
- NeoGeo states are different size again

Do you have any ideas regarding these?
I can try to fix them again but I wonder if we can avoid these in the future.

If at all possible, do you think we can pull from SVN directly? is the SVN public? (at least for reading?)

Thanks in advance for your responses

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #863 on: December 19, 2017, 09:13:01 PM »
radius,
I think we did some padding using a compiler macro to prevent different sized states between 32/64bit systems.  I only have a 32bit system, so I can't really test it right now.  I'd better let barbudreadmon respond here, as he knows more about the libretro port :)  Our svn is private right now, but that may change in the future.

best regards,
- dink

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #864 on: December 20, 2017, 04:32:05 AM »
I'm not sure it is related to the bump to .40 release, as i mentioned in https://github.com/libretro/fbalpha/issues/166, it is more likely an issue with changing int to intptr_t in m68kcpu.c's m68k_context_size_no_pointers function in https://github.com/libretro/fbalpha/commit/0d21221fe9030ca8c482ca9042f92babf0a52dde .
From my understanding of "intptr_t", it doesn't return the same in 32/64 bits, and i kinda remember there was a warning related to this function and dink explicitly told me not to mess with it if i didn't want to break savestates, but someone else did and i overlooked this.

Edit : it would also be worth checking what was done on http://neo-source.com/index.php?topic=3162.0 , jan's special version mention being fully cross-platform for savestates. I don't think i'll have time to deal with bugs this week (wait for next week, i took leave), so please be patient, or try reverting the 2 changes i mentioned in https://github.com/libretro/fbalpha/issues/166 (the one with intptr_t and the one with ACB_FULLSCAN) and tell me if it fixes this issue.
« Last Edit: December 20, 2017, 04:58:19 AM by barbudreadmon »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #865 on: December 20, 2017, 07:54:29 AM »
Correct -- intptr_t returns an int the size of a pointer on the current platform.

Unfortunately what I meant by cross-platform there is that the code in burner is cross-platform. It still gets the same blobs of data from burn so that old savestates can be loaded or converted. Any 32/64bit or endianness issues that remain in burn need to be fixed there, which may break savestates (input recordings need the endianness converted on BE systems but should otherwise work).

The new fileformat code itself can deal with endianness and has string conversions (all strings in the files are utf8), so that all files can be read and understood on any platform. It also uses the boost filesystem library (now part of the C++ standard, since C++17) to deal with file system in a cross-platform manner. It also has C++14 template stuff. The fixes burn needs are easy enough to do with the same C++14 template stuff, but portability...

Offline dink

  • Administrator
  • *****
  • Posts: 5014
  • Karma: +449/-1
  • pie? I nearly bought one!
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #866 on: December 20, 2017, 08:02:09 AM »
I'd like to help, but I don't have a 64bit system handy for testing.
Also, got some bad news: the next version has lots of updated cores, and with updates come more critical variable scanning.  This means .42 states won't be compatible with .43.  I had a dream about a new savestate format that would overcome problems like this, but we would probably have to do some collaboration to make it happen. 
I'm guessing that right now everything is saved in a binary blob, and if something is added or missing in-between, everything below it goes a-skew.
The idea is, state data could be saved in a format like:
var_name_token,size_of_data,data
if the size doesn't match the current size, it would be skipped.  Also, if a new variable is added but the data in the savestate doesn't exist, it would get skipped. 
Of course, Jan's special version could do this in the latest incarnation of C++, with a ton of add-on libraries, but, that would leave a lot of people out.  We need a global solution that will work with archaic compilers alike.  C++99 would be the limit.   
Jan, I'm really surprized you can't think of a solution that doesn't require the latest C++ and a bunch of libraries, I'm not even going to debate this with you, but.. give me a break!

best regards,
- dink
« Last Edit: December 20, 2017, 08:06:08 AM by dink »

Offline jan_klaassen

  • FBNeo Dev
  • ******
  • Posts: 315
  • Karma: +10/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #867 on: December 20, 2017, 08:39:20 AM »
Actually, most of what you describe is already how the savestates work. And using templates to store some types in a cross-platform manner requires zero add-on libraries, just a few small templates, and the use of  ptrdiff_t and inptr_t/uinptr_t for pointer arithmetic. That also assumes you (or at least the compiler) know the types involved, so dumping structs with these types in them into a single blob won't work (but templates can still help).

You can do that in plain C, but you'd need to explicitly call special functions for each affected type, which is error-prone and goes against the way fba has handled savestates since forever.

Also, just for the record, my builds have exactly 2 external dependencies that yours doesn't: libjpeg and boost. Go be a luddite and keep being stuck in the past, I really don't care. I'm just completely uninterested in joining you there. Modern C++ and Python help me do stuff that make fba better, so I use them. Go figure.

Offline radius

  • New Member
  • *
  • Posts: 2
  • Karma: +0/-0
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #868 on: December 20, 2017, 10:28:11 PM »
I'm not sure it is related to the bump to .40 release, as i mentioned in https://github.com/libretro/fbalpha/issues/166, it is more likely an issue with changing int to intptr_t in m68kcpu.c's m68k_context_size_no_pointers function in https://github.com/libretro/fbalpha/commit/0d21221fe9030ca8c482ca9042f92babf0a52dde .
From my understanding of "intptr_t", it doesn't return the same in 32/64 bits, and i kinda remember there was a warning related to this function and dink explicitly told me not to mess with it if i didn't want to break savestates, but someone else did and i overlooked this.

Edit : it would also be worth checking what was done on http://neo-source.com/index.php?topic=3162.0 , jan's special version mention being fully cross-platform for savestates. I don't think i'll have time to deal with bugs this week (wait for next week, i took leave), so please be patient, or try reverting the 2 changes i mentioned in https://github.com/libretro/fbalpha/issues/166 (the one with intptr_t and the one with ACB_FULLSCAN) and tell me if it fixes this issue.

It's not the fullscan thing (that one helped me with other issues like the coin counter not being saved properly in neogeo games) and I did that waaaay later.
I tried reverting that change and it didn't help though.

It's definitely the bump to .40 but that is one HUGE COMMIT so it will be difficult to narrow it down.

Offline barbudreadmon

  • Administrator
  • *****
  • Posts: 1091
  • Karma: +59/-1
  • Helper
Re: FB Alpha 0.2.97.42 Bug Reports
« Reply #869 on: December 21, 2017, 05:40:38 AM »
Ok, i think this is my fault, when i told dink about the changes for padding the savestates, it seems i overlooked some of them, so they weren't included in .40 release, here is the patch for the things i overlooked : https://github.com/libretro/fbalpha/commit/9722254c6b3789b0936e883d5b63a27a7c20b81a.patch
I also reverted intptr_t to int in m68kcpu.c .

There is just one last thing in sh2.cpp, in libretro-fbalpha .39 we did this for savestates cross-platform compatibility : https://pastebin.com/0cmkDAAi
But the current code is slightly different :
Code: [Select]
ScanVar(& ( Sh2Ext[i].sh2 ), sizeof(SH2) - 4, szText);was replaced by
Code: [Select]
ScanVar(& ( Sh2Ext[i].sh2 ), sizeof(SH2), szText);
Sh2Ext[i].sh2.irq_callback = irq_callback;"
So i'm not sure how to apply the changes from https://pastebin.com/0cmkDAAi to the current version (honestly i don't even know why they were necessary for savestates cross-platform compatibility). Any suggestion would be welcome.