libretro / vice-libretro

Versatile Commodore 8-bit Emulator
GNU General Public License v2.0
40 stars 70 forks source link

Crash loading any .d64 #379

Closed snoofly closed 3 years ago

snoofly commented 3 years ago

Ever since the 3.5 sync update, I can’t load a d64 file on Switch or Vita, Retroarch crashes. 3.3 version builds work fine with the same d64 files. Any other type file, tap, pro, p00 etc work ok on 3.5 Thanks

sonninnos commented 3 years ago

Not able to reproduce on Windows and Linux, so without logs there is too much guessing involved..

snoofly commented 3 years ago

Let me know what files may help debug the issue. I will upload a Vita psp2dmp file tomorrow, anything else?

Also, maybe coincidence, but this issue is strangely similar to ZX Spectrum emulator issue I hit on the Vita

https://github.com/libretro/fuse-libretro/issues/95

after the Fuse libretro port was upgraded

rsn8887 commented 3 years ago

I can reproduce this on my Switch.

Logfile: https://pastebin.com/jWSxcz4z

rsn8887 commented 3 years ago

Another log file: https://pastebin.com/23QWLnFj

rsn8887 commented 3 years ago

Another two logs after updating retroarch_switch.nro as well: https://pastebin.com/q8vB2FTF https://pastebin.com/PRxURQQU

rsn8887 commented 3 years ago

I suspect you are trying to open .d64 with some non-supported fopen flags, e.g. "read-only" or some such. For example on Switch, I couldn't open files read-only (r), but read-write worked (rw). I don't remember exactly.

Here is another log (error-only) after deleting all configs, and trying to open a .d64 that is not in .zip: https://pastebin.com/qwzqgkJ0

rsn8887 commented 3 years ago

Hmm log file looks as if you are trying to open .d64 twice. First time works, second time fails.

rsn8887 commented 3 years ago
[libretro INFO] AUTOSTART: Autodetecting image type of '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'.
[libretro INFO] AUTOSTART: Attached file '/retroarch/cores/savefiles/TEMP/ABOULDER.D64' as a disk image.
[libretro INFO] AUTOSTART: mounted image is type: 1541, not changing drive.
[libretro ERROR] Filesystem Image: Cannot open file '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'.
[libretro ERROR] Tape: Cannot open file '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'
[libretro INFO] CART: '/retroarch/cores/savefiles/TEMP/ABOULDER.D64' is not a valid CRT file.
[libretro ERROR] AUTOSTART: Cannot open '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'.
[libretro ERROR] AUTOSTART: '/retroarch/cores/savefiles/TEMP/ABOULDER.D64' is not a valid file.
[libretro ERROR] Failed to autostart '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'

Here it seems it works at first then says not a valid file.

sonninnos commented 3 years ago

So perhaps that new disk image type autodetect isn't closing the file.. I wish I could test/confirm it somehow.

PUAE had similar issue on that platform, where it tried to do stat on already opened file.

rsn8887 commented 3 years ago

It seems to mount it, yet then AUTOSTART again opens it?!?!

rsn8887 commented 3 years ago

I thought that autodetect was broken, did they actually fix it in 3.5? There was this commented/unused code in Vice for a long time, always broken I think.

sonninnos commented 3 years ago

Yes, it was changed and enabled now in 3.5, and we had the old one enabled as well all this time.

rsn8887 commented 3 years ago

I cannot debug either, newest SDK is incompatible with Retroarch code.

The first error is the problem I think: [libretro ERROR] Filesystem Image: Cannot open file '/retroarch/cores/savefiles/TEMP/ABOULDER.D64'.

This happens here in fsimage.c I think. Somehow zfile_open fails.

        log_error(fsimage_log, "Cannot open file `%s'.", fsimage->name);
        return -1;
rsn8887 commented 3 years ago

It opens the file twice, without closing in-between.

rsn8887 commented 3 years ago

Something is wrong with autostart, it shouldn't try to open it twice.

rsn8887 commented 3 years ago

If I disable that disk drive autodetect, it works on Switch!!!

diff --git a/vice/src/autostart.c b/vice/src/autostart.c
index 745fd4944..185c17990 100644
--- a/vice/src/autostart.c
+++ b/vice/src/autostart.c
@@ -1514,14 +1514,14 @@ int autostart_disk(int unit, int drive, const char *file_name, const char *progr
     if (name) {
         autostart_disk_cook_name(&name);
         if (!(file_system_attach_disk(unit, drive, file_name) < 0)) {
-#if 1
+#if 0
             vdrive_t *vdrive;
             struct disk_image_s *diskimg;
 #endif

             log_message(autostart_log,
                         "Attached file `%s' as a disk image.", file_name);
-#if 1
+#if 0
             /*
              * Simple attempt at implementing setting the current drive type
              * based on the image type as per feature request #319.
rsn8887 commented 3 years ago

The double open can be easily debugged on any machine by adding some logging around the zfile_fopen calls in fsimage_open in fsimage.c. I cannot seem to find another way around it though. Even if I detach the image after drive autodetect, it doesn't work! The only solution seems to be to disable the drive type autodetect code.

Why was code used in 3.5 that is labeled

/* shitty code, we really need to extend the drive API to
* get at these sorts for things without breaking into core code
*/
sonninnos commented 3 years ago

It isn't that different from the 3.3 one, so we could simply use the old version instead. It is also mostly replicated in libretro-core.c as autodetect_drivetype() and is used when disks are changed via Disc Control. So that should still work..?

That same comment was there in 3.3 too btw.

rsn8887 commented 3 years ago

You might want to check on your platform that the second open is not in read only mode. It tried to do that on Switch, as a fallback, but fails. I think this could pose a potential problem with saving to d64.

thesnable commented 3 years ago

Hi! On the ARMHF / NEON Platform exactly the same problem. PRG works. D64 fails (3.3 core) 3.0 works fine

The d64 load fine when inserted later.

thesnable commented 3 years ago

-> i cross compiled for armhf on linux. This works. The buildbot on rtetroarch needs an update.

rsn8887 commented 3 years ago

I think I found the problem. It is a real problem in the code logic.

file_system_attach_disk(unit, drive, file_name) is called twice without a file_system_detach_disk(unit, drive) inbetween, resulting in double fopen().

rsn8887 commented 3 years ago

@sonninnos please see my PR. The logic makes sense to me and it fixes the problem for me.

sonninnos commented 3 years ago

Yep, that seems reasonable.

I also tried with RPi2 just now and no problem launching D64s without that fix..

snoofly commented 3 years ago

Thanks guys. That's some great work there.