Updates/MSVC 2017 compilation

[albertofustinoni]

This is incomplete. Opening in the hopes someone can help me finish it.

• Integrated new filestream with VFS support, however VFS is left inactive because of call to path_mkdir which is not VFS aware and current VFS doesn't support the functionality. Truth be told, the core works even when VFS is on and that call fails. Would like to figure out options/workarounds.
• Updated build script for MSVC2017 (Desktop+UWP), however UWP doesn't build due to CHD issues and GCC build fails due to an error I haven't been able to figure out yet. If someone can help me with that it would be really appreciated

[inactive123]

Maybe I am missing something, but is there a point to this if filestream right now is not being used anyway yet it still works? Or is the intention to have it make use of that eventually?

Or are you using file_stream_transforms.h perhaps? In that case I understand.

[albertofustinoni]

The intention is to have it use filestream with the VFS eventually. In any case, it updates the core to use the new versions of libretro.h and filestream - which is good for consistency.

When I say "it works" I mean if I enable the VFS for the core when running against my front I can play games just fine: path_mkdir is only being used to persist EEPROM state between core runs.

We may just do without the EEPROM for the time being, if you think the trade off is acceptable - even if not, it's probably better if my fork of the core only differs by two lines commented out.

[inactive123]

Well, promise me this - don't add this VFS stuff to cores that use need_fullpath = false or that are not using the filestream interface in any capacity. We don't want to add it to cores that have no need for filestream interfaces at all, just for the cores that do.

In the case of FB Alpha an argument can be made that there is probably a point to doing this. For something like snes9x or VBA-M, though, I don't want it.

[albertofustinoni]

Question for you then: what about cores that have need_fullpath = false but do file IO on the system/save directory?

Also, FBAlpha does require fullpath - in general I do not like to make more work for myself

[barbudreadmon]

I'm not merging this anytime soon, i want to fix the current bugs before introducing new potential sources of bugs, because i'm still not 100% sure the introduction of filestream is not partially responsible of https://github.com/libretro/RetroArch/issues/5924, and i don't have that much time at the moment to work on libretro-fbalpha.

[inactive123]

@albertofustinoni Cores that set need_fullpath = false do not need filestream-specific I/O bits for saving savestates or SRAM in most cases. Reason being that when need_fullpath is false, the info struct's path member will be NULL, only data and size will be filled in.

If you know of cores that buck this trend, please point them out as this is not standard procedure.

[albertofustinoni]

@twinaphex That is good to know - less work and less reason to mess with the codebase :)

[barbudreadmon]

I was right, file_stream_transform is causing the buffer overflow in hiscores.

[albertofustinoni]

@barbudreadmon @twinaphex In that case, I would highly recommend that we fix the issue starting from this PR, since the whole file_stream has been rewritten (part of this PR is updating file_stream to the current version). This means that:

• The memory leak may have been fixed as part of the rewrite
• If the leak is still there, we would definitely want to fix it in the current version of file_stream, which is used by the other libretro cores

[barbudreadmon]

Your feof drop-in replacement is not working properly, see lines 525-530 of https://github.com/libretro/fbalpha/blob/master/src/burn/hiscore.cpp ? Without your file_stream_transform, nSize ends up at 180 for the dkong rom, with your code, feof probably always return 0, so nSize stays 0.

No, i won't push any new code as long as you don't find a fix for the current one. For the sake of less than 1% of windows phone users, we are corrupting data for all users there, i'm really displeased with that, and don't intend to keep the whole file_stream_transform thing for long if a fix for the current one is not found.

[inactive123]

@Alcaro @albertofustinoni Found another issue with the filestream/VFS code - just wanted to give a heads-up to everybody -

https://github.com/libretro/Genesis-Plus-GX/commit/69c2412b8ea0c1ba4e9be49bd6c3cd8864eced5f

Looks like returning int64_t instead of int32_t is causing a lot of issues on 32bit systems. For Genesis Plus GX I managed to fix the issues for now by just casting back to int32_t - otherwise size in this example would be 0 for PS3 and other 32-bit OSes/systems I presume.

[inactive123]

Improved upon it and just made them use 64bit integer values for size instead -

https://github.com/libretro/Genesis-Plus-GX/commit/9588749d454a077fd3200ba7adfe303f8c1ddc3d

Works on PS3.

[inactive123]

@barbudreadmon

Just to clarify - the issue with filestream_eof was due to older filestream code.

@albertofustinoni will submit a new PR for the filestreams/VFS code - this will be newer updated code which should fix the filestream_eof issues. They were already not an issue for quite some time; this code has just not been updated recently. I hope we can fast-track through at least his separate PR since it's relatively safe.

[albertofustinoni]

@barbudreadmon As you correctly identified, the issue lies with the old implementation of feof, which was broken. This has been fixed in newer versions of filestream. Relevant changes below.

Since this is a priority issue as it breaks the core, I have reverted the makefile to the old one so that we can merge it quicker. The only other UWP/VS specific change I left are the ones in libretro.cpp (from line 761), which are #ifdefined away for anything but Visual Studio compiling for UWP.

feof fix:

From

int filestream_eof(RFILE *stream)
{
   size_t current_position = filestream_tell(stream);
   size_t end_position     = filestream_seek(stream, 0, SEEK_END);

   filestream_seek(stream, current_position, SEEK_SET);

   if (current_position >= end_position)
      return 1;
   return 0;
}

Error was in setting end_position = filestream_seek

To (the version in the current PR - moved to vfs_implementation.c)

int filestream_eof(RFILE *stream)
{
   int64_t current_position = filestream_tell(stream);
   int64_t end_position     = filestream_get_size(stream);

   if (current_position >= end_position)
      return 1;
   return 0;
}

Which has worked so far in other cores

[barbudreadmon]

Merged your pr locally, now instead of corrupting data for dkong, it crashes...

[barbudreadmon]

Ended up fixing #162 the simple way, make running dkong a part of your tests and come back when you have a working solution.

[barbudreadmon]

I don't intend to remove the -O3 optimization level, and this is still crashing

[barbudreadmon]

Also, the old issue was indeed with feof, but the new one seems to be with rfopen (crashing at line 410 of hiscore.cpp now, long before the feof is called)

[albertofustinoni]

OK, hopefully this fixes the issue. I have:

• Included @Alcaro 's fix for feof
• Modified fropen as below, to be more stdio like and fix the "r" flag issue

RFILE* rfopen(const char *path, const char *mode)
{
   unsigned int retro_mode = RETRO_VFS_FILE_ACCESS_READ;
   bool position_to_end = false;

   if (strstr(mode, "w"))
   {
       retro_mode = RETRO_VFS_FILE_ACCESS_WRITE;
   }

   if (strstr(mode, "a"))
   {
       retro_mode = RETRO_VFS_FILE_ACCESS_READ_WRITE & RETRO_VFS_FILE_ACCESS_UPDATE_EXISTING;
       position_to_end = true;
   }

   if (strstr(mode, "+"))
   {
       retro_mode = RETRO_VFS_FILE_ACCESS_READ_WRITE;
       if (strstr(mode, "r"))
       {
           retro_mode = retro_mode & RETRO_VFS_FILE_ACCESS_UPDATE_EXISTING;
       }
   }

   RFILE* output = filestream_open(path, retro_mode, RETRO_VFS_FILE_ACCESS_HINT_NONE);
   if (output != NULL && position_to_end)
   {
       filestream_seek(output, 0, RETRO_VFS_SEEK_POSITION_END);
   }

   return output;
}

Does it look sensible enough?

Also restored O3 compilation under GCC

[barbudreadmon]

I added the changes related to libretro-common in my last commit.

You are still not telling me why you want to change the optimization level, i won't do that without a good explanation, it will most likely end up with the compilers aligning data differently, so potentially breaking netplay because the savestates will be different.

[albertofustinoni]

@barbudreadmon The Visual Studio compiler doesn't support -O3, this is why I made the change to begin with.

In my last commit, I have changed the makefile to use -O3 again unless the core is being compiled in Visual Studio.

Relevant lines below:

ifeq ($(DEBUG), 1)
   CFLAGS += -O0 -g -DFBA_DEBUG
   CXXFLAGS += -O0 -g -DFBA_DEBUG
else
   ifeq (,$(findstring msvc,$(platform)))
      CFLAGS += -O3 -DNDEBUG
      CXXFLAGS += -O3 -DNDEBUG
   else
      CFLAGS += -O2 -DNDEBUG
      CXXFLAGS += -O2 -DNDEBUG
   endif
endif

Hopefully this works for you?

[barbudreadmon]

Well, as i explained, it could break savestates compatibility and netplay between your platform and other platforms if we do this, since they heavily rely on data alignment.

What do you mean by -O3 is not supported by msvc ? This is pretty worrying.

Could you send me some neogeo / cps2 / cps3 savestates from your platform so i could check if their size are the same as mine and if they load fine ?

[albertofustinoni]

By -O3 not being supported I mean this: note -O1 and -O2 but no -O3.

Just to make sure, the save states you want are the result of calling retro_serialize, right?

[barbudreadmon]

Just to make sure, the save states you want are the result of calling retro_serialize, right?

I think so, with retroarch they are generated by using "save state" in ui's "quick menu".

[ghost]

@albertofustinoni Just FYI, I recently made path_mkdir Unicode friendly in RA's -common folder. I asked @twinaphex about adding it to VFS and he mentioned there was some disagreement on that, so I have not added it... but if he says yes, you're more than welcome to (or I can add it, either way).

[inactive123]

You can go ahead and add it to the VFS yes, as long as you keep it within file_path.c as well.

[Alcaro]

retro_mode = RETRO_VFS_FILE_ACCESS_READ_WRITE & RETRO_VFS_FILE_ACCESS_UPDATE_EXISTING;

That equals zero. Do you mean |?

it will most likely end up with the compilers aligning data differently, so potentially breaking netplay because the savestates will be different.

I've never ever heard of a C or C++ compiler aligning data differently depending on compiler flags. They're all designed to allow mixing object files with different compilation flags. I haven't even seen struct layout vary between different compilers for the same platform.

By -O3 not being supported I mean this: note -O1 and -O2 but no -O3.

Even if -O2 exists on both, it does different things. The "Uses maximum optimization" flag is -Ox, -O2 just enables a few of them. -O0 and -g don't exist on MSVC, either. The flags are so different between GCC and MSVC I recommend not even trying to put them in the same Makefile.

I also recommend splitting MSVC and VFS to separate PRs, they don't require each other and it's kinda messy to have them together like this.

[barbudreadmon]

I've never ever heard of a C or C++ compiler aligning data differently depending on compiler flags

Spent 3 weeks fixing misaligned data crashes when building with -O3, from my understanding it means -O3 (and particularly the tree vectorizers) handle data alignment kinda differently, i don't know how or if it will affect savestates though, that's why i'm asking for the savestates so i can check.

[frranck]

@barbudreadmon maybe you should try to add flags included into -O3 one by one to figure out which one fails. I would bet on the aliasing.

[barbudreadmon]

@frranck as i mentioned, the flags were the tree vectorizers (-ftree-loop-vectorize and -ftree-slp-vectorize), they were throwing exception about misaligned data (which, from my understanding, means the compiler is working kinda differently with data alignment since -O2 wouldn't throw those exceptions), all of this was fixed in .39 or before . Again, i'm not sure if or how it will affect savestates, i just want some peace of mind.

[albertofustinoni]

Closing since it’s getting messy