FCB Rename causes FDPP crash

[dosemu2]

FCB Rename causes FDPP crash

[stsp]

This should be something very
simple, plus this usually throws a
backtrace into the log.
90% of such cases mean the far
pointer was not found in the cache
at the right moment.
Please provide the log.

[andrewbird]

here's the log
test.zip

[stsp]

Yes, the FAR emulator couldn't evaluate this line:
dta = MK_FAR(Dmatch);.

[stsp]

But not because of the missed FAR cache entry, hmm...

[andrewbird]

I've uploaded the test for this
test-imagedir.tar.gz

bin/dosemu.bin -n -f test-imagedir/dosemu.conf -D+dRW -o test.log --Fimagedir test-imagedir

Then run test_mfs.bat from the C: drive

[andrewbird]

BTW this FCB test is one of the few that actually pass on FreeDOS, so I figure this has to work before I can chase down the others.

[stsp]

I think this should be fixed, although I haven't
tried the test-case itself. FAR emulator was not
able to find the way to pass the object directly
to DOS. I added the intermediate pointer, which
should help him. Of course maybe I should have
fixed the emulator itself, but this is for the later.

[andrewbird]

Still seeing a crash, log here test2.zip

Note: I rebuilt both fdpp and dosemu2

[stsp]

Please test this one:

diff --git a/fdpp/farptr.hpp b/fdpp/farptr.hpp
index a2b8bc6..d65a421 100644
--- a/fdpp/farptr.hpp
+++ b/fdpp/farptr.hpp
@@ -187,7 +187,12 @@ public:
 
     template<typename T0, typename T1 = T,
         typename std::enable_if<ALLOW_CNV(T1, T0) && !_C(T0)>::type* = nullptr>
-    operator FarPtrBase<T0>() {
+    operator FarPtrBase<T0>() const & {
+        return FarPtrBase<T0>(this->seg(), this->off());
+    }
+    template<typename T0, typename T1 = T,
+        typename std::enable_if<ALLOW_CNV(T1, T0) && !_C(T0)>::type* = nullptr>
+    operator FarPtrBase<T0>() && {
         _assert(!obj);
         return FarPtrBase<T0>(this->seg(), this->off());
     }

[andrewbird]

Crash is seemingly the same, except the line reported moved quite a bit - new log test3.zip

[stsp]

OK, trying myself. :)

[andrewbird]

Don't forget to update the fdppkrnl.sys in test-imagedir/dXXXXs/c if you think you need to.

[stsp]

Damn, a small typo in a patch I copy/pasted here. :)
Now it works fine.

[stsp]

Don't forget to update the fdppkrnl.sys in test-imagedir/dXXXXs/c if you think you need to.

This is not needed as the kernel is changed
very rarely. Maybe if I bind it to git hash, it will
became more problematic.

[andrewbird]

Compiling now, but whilst I wait do you still need the patch 7348473 to the kernel source as it would be nice to have as little diff to freedos?

[stsp]

Jfyi, the first patch was not redundant.
Reverting either, makes the crash to re-appear.

[stsp]

You can try reverting it yourself, so far its need.
Maybe later...

[stsp]

In fact I don't even know how it is possible
to avoid that patch. I did a lot of small mods like
this, when doing otherwise is very difficult.
So "lightly modified" kernel only means its
basic structure is the same. There are thousands
of small tweaks all around now, unfortunately.

[andrewbird]

Thanks the crash is gone, test is failing(the DOS reports success, but file isn't renamed), but that's an FCB problem for me to fix!

No worries about backing that previous commit out. Thanks again!

[stsp]

In this particular case the dta is a global DOS
object without any scope, and Dmatch is a local
var which scope is invisible to the FAR emulator
(it is visible only to clang compiler). So far emulator
simply can't deduce the scope of the pointer.
If we hint him with the local pointer, then he happily
parses this place correctly. But there is still a risk
that our hint will mismatch with the scope of Dmatch,
so I placed it right below the Dmatch. Its not a
real compiler, so it really can't do things w/o some
manual help like this.

[andrewbird]

If I want to use gdb on fdpp do I need to use any special dosemu settings? I tried:

gdb bin/dosemu.bin
run -n -f test-imagedir/dosemu.conf -D+dRW -o test.log --Fimagedir test-imagedir -H1
ctrl-c
break FcbRename
cont

bin/dosdebug
g

but I ended up with segv

[andrewbird]

Ahh fullsim

[stsp]

fullsim should be good.
Please fill or fix the crash.

[stsp]

Normally you should use sim, not fullsim.
But w/o DPMI this is the same thing.

[stsp]

you can also use
gdb --args bin/dosemu.bin and_all_args_here

[andrewbird]

ahh --args that's nice

[andrewbird]

So should I be able to see the contents of the extended FCB as per here

Thread 1 "dosemu.bin" hit Breakpoint 1, FcbRename (lpXfcb=...) at fcbfns.cc:516
516	  __FAR(rfcb)lpRenameFcb;
(gdb) l
511	  return result;
512	}
513	
514	UBYTE FcbRename(__FAR(xfcb) lpXfcb)
515	{
516	  __FAR(rfcb)lpRenameFcb;
517	  COUNT FcbDrive;
518	  UBYTE result = FCB_SUCCESS;
519	  __FAR(void)lpOldDta = dta;
520	
(gdb) p lpXfcb
$7 = {<FarPtrBase<_xfcb>> = {ptr = {off = 63968, seg = 32911}}, obj = <error reading variable: Cannot access memory at address 0x19bc0122>, 
  children = std::unordered_set with 2156917768 elements<error reading variable: Cannot access memory at address 0x7>, nonnull = false}

[stsp]

You should, if you use the lowmem_base trick I
showed you the previous time. :)

[andrewbird]

okay thanks but the data looks like zeros

(gdb) p *(struct _xfcb *)  &lowmem_base[(32911<<4) + 63968]
$8 = {xfcb_flag = 0 '\000', xfcb_resvrd = "\000\000\000\000", xfcb_attrib = 0 '\000', xfcb_fcb = {<_fcb> = {fcb_drive = 0 '\000', 
      fcb_fname = {<MembBase<char, &_fcb::off_fcb_fname>> = {<No data fields>}, sym = "\000\000\000\000\000\000\000", static len = 8}, 
      fcb_fext = {<MembBase<char, &_fcb::off_fcb_fext>> = {<No data fields>}, sym = "\000\000", static len = 3}, fcb_cublock = 0, fcb_recsiz = 0, fcb_fsize = 0, fcb_date = 0, fcb_time = 0, 
      fcb_sftno = 0 '\000', fcb_attrib_hi = 0 '\000', fcb_attrib_lo = 0 '\000', fcb_strtclst = 0, fcb_dirclst = 0, fcb_diroff_unused = 0 '\000', fcb_curec = 0 '\000', 
      fcb_rndm = 0}, <MembBase<_fcb, &_xfcb::off_xfcb_fcb>> = {<No data fields>}, <No data fields>}}

[stsp]

lr.AL = FcbRename((xfcb FAR *)FP_DS_DX);
Make sure DS:DX points to the right value.
If so - maybe the Cish typecast got wrongly
parsed (but that would be strange as I think
I've got the type-casts working long ago).

[andrewbird]

I'm sure I'm doing something wrong in gdb as all I get are zeros on the r regs that are copied to lr regs

(gdb) p r
$10 = {<FarPtrBase<_iregss>> = {ptr = {off = 64992, seg = 32911}}, obj = <error reading variable: Cannot access memory at address 0x19bcffea>, 
  children = std::unordered_set with 4426138 elements = {[0] = std::shared_ptr<ObjIf> (empty) = {get() = 0x0}<error reading variable: Cannot access memory at address 0x1f3900>...}, 
  nonnull = 230}
(gdb) p *(struct _iregss *)  &lowmem_base[(32911<<4) + 64992]
$11 = {a = {x = 0, b = {l = 0 '\000', h = 0 '\000'}}, b = {x = 0, b = {l = 0 '\000', h = 0 '\000'}}, c = {x = 0, b = {l = 0 '\000', h = 0 '\000'}}, d = {x = 0, b = {l = 0 '\000', 
      h = 0 '\000'}}, si = 0, di = 0, bp = 0, ds = 0, es = 0, ip = 0, cs = 0, flags = 0}

And we are entering FcbRename, so lr.AL must have been 0x17

[stsp]

See p lr

[stsp]

I think this code is full of problems though.
All those nonnull = 230 etc - its just a load of shit. :(
I'll see about fixing it in general (not this particular bug)

[andrewbird]

(gdb) p lr
$23 = <optimised out>
(gdb) l
596	    case 0x16:
597	      lr.AL = FcbOpen((__FAR(xfcb))FP_DS_DX, O_FCB | O_LEGACY | O_CREAT | O_TRUNC | O_RDWR);
598	      break;
599	
600	    case 0x17:
601	      lr.AL = FcbRename((__FAR(xfcb))FP_DS_DX);
602	      break;
603	
604	    default:
605	#ifdef DEBUG

[stsp]

Try EXTRA_DEBUG = 1.
But as I said, this code is severely sick.

[andrewbird]

Okay will try that tomorrow, thanks.

[stsp]

I wasn't able to reproduce that problem,
gdb was showing everything correctly for
me, both before and after the patch that
I've now applied. Hope it will fix your case.

[andrewbird]

So using EXTRA_DEBUG=1 and the latest commit enables lr to be resolved
without:

(gdb) p lr
$4 = <optimised out>

with:

(gdb) p lr
$1 = {a = {x = 5888, b = {l = 0 '\000', h = 23 '\027'}}, b = {x = 0, b = {l = 0 '\000', h = 0 '\000'}}, c = {x = 255, b = {l = 255 '\377', h = 0 '\000'}}, d = {x = 286, b = {l = 30 '\036', 
      h = 1 '\001'}}, si = 256, di = 65534, ds = 6588, es = 6588}

I can also do p lr.DS and p lr.DX

It's different with r I have to use the lowmem_base trick but the values seem the same as the copied lr so I think we are seeing correct data

(gdb) p r
$2 = {<FarPtrBase<_iregss>> = {ptr = {off = 65510, seg = 6588}}, obj = std::shared_ptr<ObjIf> (empty) = {get() = 0x0}, children = std::unordered_set with 0 elements, nonnull = false}

(gdb) p *(struct _iregss *) &lowmem_base[(6588<<4)+65510]
$3 = {a = {x = 5888, b = {l = 0 '\000', h = 23 '\027'}}, b = {x = 0, b = {l = 0 '\000', h = 0 '\000'}}, c = {x = 255, b = {l = 255 '\377', h = 0 '\000'}}, d = {x = 286, b = {l = 30 '\036', 
      h = 1 '\001'}}, si = 256, di = 65534, bp = 2334, ds = 6588, es = 6588, ip = 266, cs = 6588, flags = 12802}

I don't seem to be able to print individual fields directly

(gdb) p r.DX
There is no member or method named d.
(gdb) p r.d.x
There is no member or method named d.
(gdb) p r.dx
There is no member or method named dx.

but I can do this

(gdb) p ((struct _iregss *) &lowmem_base[(6588<<4)+65510])->d->x
$14 = 286

[stsp]

Yes, that looks correct. And no more
garbage in the objects.

[stsp]

Use p /x for a hex output.

[stsp]

Would be nice to see if the garbage was
fixed by a patch or by debug.

[andrewbird]

Thanks, maybe I try turning off EXTRA_DEBUG later, but I'm on the track of the FCB problem now.

[andrewbird]

this works quite nicely

(gdb) macro define xfcb(v) *(struct _xfcb*) &lowmem_base[(v.ptr.seg<<4)+v.ptr.off]
(gdb) p xfcb(lpExtFcb)
$24 = {xfcb_flag = 0 '\000', xfcb_resvrd = "testa", xfcb_attrib = 32 ' ', xfcb_fcb = {<_fcb> = {fcb_drive = 32 ' ', 
      fcb_fname = {<MembBase<char, &_fcb::off_fcb_fname>> = {<No data fields>}, sym = " bat\000\000\000", static len = 8}, 
      fcb_fext = {<MembBase<char, &_fcb::off_fcb_fext>> = {<No data fields>}, sym = "\000te", static len = 3}, fcb_cublock = 29811, fcb_recsiz = 8290, fcb_fsize = 1633820704, 
      fcb_date = 108, fcb_time = 0, fcb_sftno = 0 '\000', fcb_attrib_hi = 0 '\000', fcb_attrib_lo = 0 '\000', fcb_strtclst = 0, fcb_dirclst = 0, fcb_diroff_unused = 0 '\000', 
      fcb_curec = 0 '\000', fcb_rndm = 0}, <MembBase<_fcb, &_xfcb::off_xfcb_fcb>> = {<No data fields>}, <No data fields>}}

Same pointer printed as an rfcb (which it is)

(gdb) macro define rfcb(v) *(struct _rfcb*) &lowmem_base[(v.ptr.seg<<4)+v.ptr.off]
(gdb) p rfcb(lpExtFcb)
$28 = {renDriveID = 0 '\000', renOldName = "testa   ", renOldExtent = "bat", renReserved1 = "\000\000\000\000", renNewName = {<MembBase<char, &_rfcb::off_renNewName>> = {<No data fields>}, 
    sym = "testb   ", static len = 8}, renNewExtent = "bal", renReserved2 = "\000\000\000\000\000\000\000\000"}

[andrewbird]

The extra stuff on the end of the gdb print line was fixed by the commit
The lack of real data when printing was fixed by setting EXTRA_DEBUG=1

The second one surprised me

macro define tdos(t, v) (struct _##t *) &lowmem_base[(v.ptr.seg<<4)+v.ptr.off]

# with EXTRA_DEBUG=0
(gdb) p *tdos(xfcb, lpXfcb)
$2 = {xfcb_flag = 0 '\000', xfcb_resvrd = "\000\000\000\000", xfcb_attrib = 0 '\000', xfcb_fcb = {<_fcb> = {fcb_drive = 0 '\000', 
      fcb_fname = {<MembBase<char, &_fcb::off_fcb_fname>> = {<No data fields>}, sym = "\000\000\000\000\000\000\000", static len = 8}, 
      fcb_fext = {<MembBase<char, &_fcb::off_fcb_fext>> = {<No data fields>}, sym = "\000\000", static len = 3}, fcb_cublock = 0, fcb_recsiz = 0, fcb_fsize = 0, fcb_date = 0, fcb_time = 0, 
      fcb_sftno = 0 '\000', fcb_attrib_hi = 0 '\000', fcb_attrib_lo = 0 '\000', fcb_strtclst = 0, fcb_dirclst = 0, fcb_diroff_unused = 0 '\000', fcb_curec = 0 '\000', 
      fcb_rndm = 0}, <MembBase<_fcb, &_xfcb::off_xfcb_fcb>> = {<No data fields>}, <No data fields>}}
(gdb) p *tdos(rfcb, lpXfcb)
$3 = {renDriveID = 0 '\000', renOldName = "\000\000\000\000\000\000\000", renOldExtent = "\000\000", renReserved1 = "\000\000\000\000", 
  renNewName = {<MembBase<char, &_rfcb::off_renNewName>> = {<No data fields>}, sym = "\000\000\000\000\000\000\000", static len = 8}, renNewExtent = "\000\000", 
  renReserved2 = "\000\000\000\000\000\000\000\000"}

# with EXTRA_DEBUG=1
(gdb) p *tdos(xfcb, lpXfcb)
$1 = {xfcb_flag = 0 '\000', xfcb_resvrd = "testa", xfcb_attrib = 32 ' ', xfcb_fcb = {<_fcb> = {fcb_drive = 32 ' ', fcb_fname = {<MembBase<char, &_fcb::off_fcb_fname>> = {<No data fields>}, 
        sym = " bat\000\000\000", static len = 8}, fcb_fext = {<MembBase<char, &_fcb::off_fcb_fext>> = {<No data fields>}, sym = "\000te", static len = 3}, fcb_cublock = 29811, 
      fcb_recsiz = 8290, fcb_fsize = 1633820704, fcb_date = 108, fcb_time = 0, fcb_sftno = 0 '\000', fcb_attrib_hi = 0 '\000', fcb_attrib_lo = 0 '\000', fcb_strtclst = 0, fcb_dirclst = 0, 
      fcb_diroff_unused = 0 '\000', fcb_curec = 0 '\000', fcb_rndm = 0}, <MembBase<_fcb, &_xfcb::off_xfcb_fcb>> = {<No data fields>}, <No data fields>}}
(gdb) p *tdos(rfcb, lpXfcb)
$2 = {renDriveID = 0 '\000', renOldName = "testa   ", renOldExtent = "bat", renReserved1 = "\000\000\000\000", renNewName = {<MembBase<char, &_rfcb::off_renNewName>> = {<No data fields>}, 
    sym = "testb   ", static len = 8}, renNewExtent = "bal", renReserved2 = "\000\000\000\000\000\000\000\000"}

[stsp]

Yes, absolutely.
See if the addresses differ.
I hope DOS addresses are in general stable
across boots.

[stsp]

Or maybe the code works properly
whereas gdb showing wrong data?
If you print the data by fdprintf etc, is
it correct then?