Vamiga: Miss-matching override types

[emoon]

When trying to compile the code with gcc it will complain about this

Emulator/FileSystems/FSBlock.h:101:25: note:   overriding ‘virtual const char* FSBlock::getName()’
     virtual const char *getName() { return nullptr; }

and then

Emulator/FileSystems/FSFileHeaderBlock.h:56:11: error: invalid covariant return type for ‘virtual char* FSFileHeaderBlock::getName()’ [-Werror]
     char *getName() override { return name.name; }

return type doesn't match here const char* vs char* it seems that clang doesn't emit this warning. This above can result in that the overridden method won't be called and only the base one will.

[emoon]

Some more warnings/errors

/home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.cpp: In member function ‘virtual bool EXEFile::readFromBuffer(const u8*, size_t)’:
/home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.cpp:82:64: error: use of deleted function ‘OFSVolume::OFSVolume(OFSVolume&&)’
     OFSVolume volume = OFSVolume("Disk", hd ? 4 * 880 : 2 * 880);
                                                                ^
In file included from /home/emoon/code/projects/vAmiga/Emulator/Files/ADFFile.h:14:0,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.cpp:10:
Emulator/FileSystems/FSVolume.h:183:7: note: ‘OFSVolume::OFSVolume(OFSVolume&&)’ is implicitly deleted because the default definition would be ill-formed:
 class OFSVolume : public FSVolume {
       ^~~~~~~~~
Emulator/FileSystems/FSVolume.h:183:7: error: use of deleted function ‘FSVolume::FSVolume(const FSVolume&)’
Emulator/FileSystems/FSVolume.h:29:7: note: ‘FSVolume::FSVolume(const FSVolume&)’ is implicitly deleted because the default definition would be ill-formed:
 class FSVolume : AmigaObject {
       ^~~~~~~~
Emulator/FileSystems/FSVolume.h:29:7: error: use of deleted function ‘AmigaObject::AmigaObject(const AmigaObject&)’
In file included from /home/emoon/code/projects/vAmiga/Emulator/Files/AmigaFile.h:13:0,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/DiskFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/ADFFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.cpp:10:
Emulator/Foundation/AmigaObject.h:33:7: note: ‘AmigaObject::AmigaObject(const AmigaObject&)’ is implicitly deleted because the default definition would be ill-formed:
 class AmigaObject {
       ^~~~~~~~~~~
Emulator/Foundation/AmigaObject.h:33:7: error: use of deleted function ‘std::recursive_mutex::recursive_mutex(const std::recursive_mutex&)’
In file included from Emulator/Foundation/AmigaObject.h:17:0,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/AmigaFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/DiskFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/ADFFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.h:13,
                 from /home/emoon/code/projects/vAmiga/Emulator/Files/EXEFile.cpp:10:
/usr/include/c++/7/mutex:101:5: note: declared here
     recursive_mutex(const recursive_mutex&) = delete;

[dirkwhoffmann]

Thanks for reporting these!

Did you compile the project under Linux or macOS? I'm asking because it would be easiest if I compile the project locally on my machine with gcc. But I don't have a Makefile for doing this and I don't know if I can tell Xcode to use gcc instead of clang.

[emoon]

I compiled this under Linux with my own "makefile" (not actually using make but another build system, but same idea)

[emoon]

Also these days on macOS I think that gcc is "forwarded" to clang unless you install a specific version

edit: This seems to be the case

gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.21)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[dirkwhoffmann]

I think that gcc is "forwarded" to clang

Yes, it does. I'm going to install Linux in a VM, pull the repo from Github and compile it with gcc... (I think it's best to keep my Mac as close as possible to the "factory settings". I've already screwed up my Python installation when I tried to get xdftools to work under Catalina)

[emoon]

Sounds like a good idea :) What I have been trying to do is to compile everything under Emulator I ran into errors with the VA_ENUM what I ended up doing with that was something like this

#ifdef VAMIGA_MACOS
// Replacement for the VA_ENUM macro which is only available on macOS
#ifndef VA_ENUM
#define VA_ENUM(_type, _name) \
typedef enum __attribute__((enum_extensibility(open))) _name : _type _name; \
enum _name : _type
#endif

#else
#ifndef VA_ENUM
#define VA_ENUM(_type, _name) \
enum _name : _type; \
enum _name : _type
#endif

And then in all places where VA_ENUM is being used I removed the typedef in front. Not sure if this will cause some issues, but I wanted to let you know.

Also if you want the code under Emulator to support cross-compilation it might be a good idea to setup Github Actions to always compile the code when you submit something, that way you will know if something gets broken.

[dirkwhoffmann]

Linux Mint is up and running...

@mithrendal: Do you remember how the timing API stuff had been resolved in virtualC64web?

hoff@linuxvm:~/vAmiga/Emulator$ make
g++ -c -o Amiga.o Amiga.cpp -I Foundation
In file included from Foundation/AmigaObject.h:13,
                 from Foundation/HardwareComponent.h:13,
                 from Foundation/AmigaComponent.h:13,
                 from Amiga.h:14,
                 from Amiga.cpp:10:
Foundation/Utils.h:15:10: fatal error: mach/mach_time.h: No such file or directory
   15 | #include <mach/mach_time.h>
      |          ^~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:7: Amiga.o] Error 1
hoff@linuxvm:~/vAmiga/Emulator$ 

[emoon]

Also notice there is no need to have the header file include mach/mach_time.h it would be better to move it to the cpp file instead (as it make it easier to ifdef for other platforms)

[dirkwhoffmann]

But I have the following variable defined in Amiga.h:

    /* System timer information. Used to match the emulation speed with the
     * speed of a real Amiga.
     */
    mach_timebase_info_data_t tb;

I'll clean this up so that no machine dependent data types will be used in class definitions.

A good thing about compiling the project under Linux is that it'll reveal inconsistencies in the include file hierarchy.

[mithrendal]

Do you remember how the timing API stuff had been resolved in virtualC64web?

it was documented here https://github.com/dirkwhoffmann/virtualc64web/issues/5

I just read it again ...I think it ended with the result that the WASM/SDL2 port did not needed it ... because syncing was handled by SDL2 itself instead of your own syncing code ...

I just checked out vAmiga ... I will do the same steps as emoon and you but on the emcc toolchain ... I will try to implant it into a copy of vc64web ... lets do some surgery now ...
oh this will get bloody

[mithrendal]

emcc -c -I. -IEmulator -IEmulator/Agnus -IEmulator/Agnus/Blitter -IEmulator/Agnus/Copper -IEmulator/CIA -IEmulator/CPU -IEmulator/CPU/Moira -IEmulator/Denise -IEmulator/Drive -IEmulator/Expansion -IEmulator/Files -IEmulator/FileSystems -IEmulator/Foundation -IEmulator/Memory -IEmulator/Paula -IEmulator/Paula/Audio -IEmulator/Paula/DiskController -IEmulator/Paula/UART -IEmulator/Peripherals -IEmulator/RTC -IEmulator/xdms -Wall -Wno-unused-variable -std=c++17 -O0 -s USE_SDL=2 -s EXTRA_EXPORTED_RUNTIME_METHODS=['cwrap'] Emulator/Amiga.cpp -o obj/Amiga.o
In file included from Emulator/Amiga.cpp:10:
In file included from Emulator/Amiga.h:14:
In file included from Emulator/Foundation/AmigaComponent.h:13:
In file included from Emulator/Foundation/HardwareComponent.h:13:
In file included from Emulator/Foundation/AmigaObject.h:13:
Emulator/Foundation/Utils.h:15:10: fatal error: 'mach/mach_time.h' file not found

include

     ^~~~~~~~~~~~~~~~~~

1 error generated.

😎 I am here too

----- solved by out commenting ---

now this

In file included from Emulator/Amiga.cpp:10:
In file included from Emulator/Amiga.h:14:
In file included from Emulator/Foundation/AmigaComponent.h:13:
In file included from Emulator/Foundation/HardwareComponent.h:14:
In file included from Emulator/Foundation/Serialization.h:21:
Emulator/Paula/Audio/AudioStream.h:15:15: warning: anonymous non-C-compatible type given name for linkage purposes by typedef declaration; add a tag name here [-Wnon-c-typedef-for-linkage]
typedef struct
^
SamplePair
Emulator/Paula/Audio/AudioStream.h:20:5: note: type is not C-compatible due to this member declaration
template
^~~~~~
Emulator/Paula/Audio/AudioStream.h:30:1: note: type is given name 'SamplePair' for linkage purposes by this typedef declaration
SamplePair;
^

[dirkwhoffmann]

I've added a new Oscillator class to the project which encapsulates all timing related stuff. One of its core methods is nanos() which returns a time stamp in nanoseconds. The current implementation continues to use the mach kernel API on macOS (fastest way to read the kernel clock there) and falls back to the POSIX clock_gettime() in Linux:

u64
Oscillator::nanos()
{
#ifdef __MACH__

    return abs_to_nanos(mach_absolute_time());

#else

    struct timespec ts;
    (void)clock_gettime(CLOCK_MONOTONIC, &ts);
    return ts.tv_sec * 1000000000 + ts.tv_nsec;

#endif
}

I haven't compiled it under Linux, yet. My goal was to group all timing related stuff in a single class first.

[dirkwhoffmann]

Regarding the VA_ENUM problem, I've replaced all typedef VA_ENUM constructs by VAMIGA_ENUM. The macros are now defined as follows:

/* All enumeration types are declared via VAMIGA_ENUM. We don't use the
 * standard C enum style to make enumerations easily accessible in Swift.
 */

// Definition for Swift
#ifdef VA_ENUM
#define VAMIGA_ENUM(_type, _name) \
typedef VA_ENUM(_type, _name)

// Definition for clang
#elif defined(__clang__)
#define VAMIGA_ENUM(_type, _name) \
typedef enum __attribute__((enum_extensibility(open))) _name : _type _name; \
enum _name : _type

// Definition for gcc
#else
#define VAMIGA_ENUM(_type, _name) \
enum _name : _type
#endif

[dirkwhoffmann]

Now, I'm facing one of these nasty C++ strangenesses which are high-level sorcery to me 😳:

Agnus/Agnus.h:66:33: error: use of deleted function ‘Copper::Copper(Copper&&)’
   66 |     Copper copper = Copper(amiga);
      |                                 ^
In file included from Agnus/Agnus.h:16,
                 from Amiga.h:19,
                 from Amiga.cpp:10:
Agnus/Copper/Copper.h:15:7: note: ‘Copper::Copper(Copper&&)’ is implicitly deleted because the default definition would be ill-formed:
   15 | class Copper : public AmigaComponent
      |       ^~~~~~

[emoon]

I never had that error myself, but a quick netsearch show some people having the same problem so I suggest taking a look at those.

[dirkwhoffmann]

The std::recursive_mutex (AmigaComponent class) is causing the issue. The easiest solution might be to replace it by a POSIX mutex. 🤔

https://stackoverflow.com/questions/24272641/how-to-use-a-stdmutex-in-a-class-context

[emoon]

If possible have a wrapper for it (I plan to build the code for Windows also and that would help me keeping the code cleaner)

[dirkwhoffmann]

Just added a Mutex class which wraps a POSIX mutex:

class Mutex
{
    pthread_mutex_t mutex;

public:

    Mutex();
    ~Mutex();

    int lock();
    int unlock();
};

The new synchronized macro looks like this:

#define synchronized \
    for(int _sync = (mutex.lock(), 1); _sync; _sync = (mutex.unlock(), 0))

I was wondering if the compiler might be allowed to optimize the calls to lock resp. unlock away, because a constant value is assigned. I hope the compiler is not allowed to, because the called functions can cause side effects (which is what they do).

I'll check tomorrow if gcc is happy with the AmigaObject class.

[emoon]

well, all modern sync primitives these days will do a spinlock first before entering the kernel so doing "smart" opts around it isn't usually needed. So in the case the mutex isn't locked it's fast.

[dirkwhoffmann]

It's not supposed to be an optimization. The reason for the for loop is to be able to use the syntax

synchronized { some code }.

To make it compile, I used a for loop that executes exactly once.

[emoon]

For that case I would suggest something like this:

    class AutoLock
    {
        Mutex& m_mutex;

    public:
        AutoLock(Mutex& mutex) : m_mutex(mutex) { m_mutex.lock(); }
        ~AutoLock() { m_mutex.unlock(); }
    };

Then in your code do this (or wrap it in a macro)

{
  AutoLock _(my_mutex);
  // some code to run here
}

The reason why this is better is that you also can have return statements and such and the lock will be released because the destructor of the temporary object will always be executed and unlock the mutex on scope exit.

[dirkwhoffmann]

For that case I would suggest something like this...

Love it. Easy and clean! In short: vAmiga like 😎.

I've adapted it like this (in Concurrency.h):

class AutoMutex
{
    Mutex &mutex;

public:

    bool active = true;

    AutoMutex(Mutex &ref) : mutex(ref) { mutex.lock(); }
    ~AutoMutex() { mutex.unlock(); }
};

The reason for the additional Boolean is the definition of my synchronization macro:

#define synchronized \
for (AutoMutex _am(mutex); _am.active; _am.active = false)

Using the for loop allows me to write something like synchronized { some code } and the lock is assured to be released at the end of the closing curly brace. Otherwise, I had to embed the macro like this: { synchronized { some code } }.

[emoon]

Just be aware that this macro has safety issues. for example this

synchronized printf("foo\n"); // this code is locked

will work correct

synchronized printf("foo\n"); printf("bar\n"); // only first statement is locked

So one has to remember to put the braces there and the compiler will not complain.

[dirkwhoffmann]

Update:

• Now, all *.cpp files can be compiled with gcc.
• All native vAmiga files can be compiled with -Wall without complaints (files belonging to the xdms library do create some warnings if compiled with -Wall).
• The SSE optimization functions (SSEUtils.c) produce compile errors with gcc. As a quick work-around, SSE extensions only get enabled if the project is compiled on (Intel) Macs.
• Oscillator.waitUntil() has no meaningful implementation on Linux yet. It simply executes assert(false). A replacement for mach_wait_until() has to be inserted here.

Now, here is the strange thing. I've linked everything together and wrote a dummy main function which instantiates a virtual Amiga. Unfortunately, this immediately produces a Segmentation Fault:

Strange enough, it does not even print "Hello, vAmiga". 🤔

[emoon]

Getting the same issue here

➜  vAmiga git:(dev) ✗ t2-output/linux-gcc-debug-default/vamiga  
[1]    87045 segmentation fault (core dumped)  t2-output/linux-gcc-debug-default/vamiga
➜  vAmiga git:(dev) ✗ lldb t2-output/linux-gcc-debug-default/vamiga
(lldb) target create "t2-output/linux-gcc-debug-default/vamiga"
Current executable set to 't2-output/linux-gcc-debug-default/vamiga' (x86_64).
(lldb) run
Process 87069 launched: '/home/emoon/code/other/vAmiga/t2-output/linux-gcc-debug-default/vamiga' (x86_64)
Process 87069 stopped
* thread #1, name = 'vamiga', stop reason = signal SIGSEGV: invalid address (fault address: 0x7fffff38c3b8)
    frame #0: 0x00005555555628eb vamiga`main at vamiga.cpp:5
   2    #include <Amiga.h>
   3    
   4    int main() {
-> 5        printf("hello\n");
   6    
   7        Amiga amiga;
   8        amiga.dump();
(lldb) bt
* thread #1, name = 'vamiga', stop reason = signal SIGSEGV: invalid address (fault address: 0x7fffff38c3b8)
  * frame #0: 0x00005555555628eb vamiga`main at vamiga.cpp:5
    frame #1: 0x00007ffff6ea7b97 libc.so.6`__libc_start_main(main=0x00007fffffffdde8, argc=-1, argv=0x00007ffff6ea7b97, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdde0) at libc-start.c:310
    frame #2: 0x00005555555627b0 vamiga

[emoon]

Works with this

t2-output/linux-gcc-debug-default/vamiga 
hello
Amiga (memory location: 0x7fa2b2b6c040)

    poweredOn: no
   poweredOff: yes
       paused: no
      running: no

Current configuration:

          df0: yes Drive 3.5" DD
          df1: no Drive 3.5" DD
          df2: no Drive 3.5" DD
          df3: no Drive 3.5" DD

         warp: 0

int main() {
    printf("hello\n");

    Amiga* amiga = new Amiga();
    amiga->dump();

    return 0;
}

You must have some really large data inside Amiga that causes a stack overflow

[dirkwhoffmann]

You must have some really large data inside Amiga that causes a stack overflow

That's not really good programming style and should be changed, I guess 🤭.

I'll go for a quick walk (need to catch some sunlight)...

[emoon]

It looks the size of Amiga is 13 048 000 bytes big (~13 meg) so there must be some large arrays in there (or some of the inherit classes)

[emoon]

Moira.h

Has this for example

    // Table holding instruction infos
    InstrInfo info[65536];

[emoon]

Ok, ~10 meg of those above is caused by this

template <class T, size_t capacity> struct RingBuffer
{
    // Element storage
    T elements[capacity];

[emoon]

For example Denise has this

    // A screen recorder for creating video streams
    ScreenRecorder screenRecorder = ScreenRecorder(amiga);

which has a bunch of these buffers, it seems to be it's not needed to create this directly as part of Amiga object. It would be better to create it if when screen capture is being used.

[dirkwhoffmann]

Seems like I took the task to emulate "Fat Agnus" a bit too literally 😬.

       Agnus : 686352 bytes
         CIA : 992 bytes
 ControlPort : 1312 bytes
         CPU : 2907832 bytes
      Denise : 4756352 bytes
       Drive : 480 bytes
    Keyboard : 552 bytes
      Memory : 5024 bytes
  Oscillator : 384 bytes
       Paula : 4683208 bytes
         RTC : 472 bytes
  SerialPort : 392 bytes
       Zorro : 376 bytes

https://www.youtube.com/watch?v=QEiZOLqzHLE

[mithrendal]

Oh dirk this is so funny 😂 @emoon you have to watch the linked movie clip. So funny 😍

[dirkwhoffmann]

I've moved Moira's InstrInfo array to the heap:

    // Jump table holding the instruction handlers
    void (Moira::*exec[65536])(u16);

    // Jump table holding the disassebler handlers
    void (Moira::*dasm[65536])(StrWriter&, u32&, u16);

    // Table holding instruction infos
    InstrInfo *info;

Now, the two jump tables are responsible for most of the memory footprint. Every function pointer is 64 bit, so this sums up to 2 * 512KB = 1 MB. However, sizeof(Moira) still reports something about 2 MB. I don't get it 🤔.

moira::Breakpoints : 40 bytes
moira::Watchpoints : 40 bytes
   moira::Debugger : 23656 bytes
      moira::Moira : 2120960 bytes

[dirkwhoffmann]

Another C++ mystery solved. Pointers to member functions aren't real pointers. They are "fat pointers" (16 byte for the two tables in question):

It is nicely explained here:

https://stackoverflow.com/questions/13875786/pointers-to-members-representations

The Underlying Representation of Pointers to Members

Although pointers to members behave like ordinary pointers, behind the scenes their representation is quite different. In fact, a pointer to member usually consists of a struct containing up to four fields in certain cases. This is because pointers to members have to support not only ordinary member functions, but also virtual member functions, member functions of objects that have multiple base classes, and member functions of virtual base classes. Thus, the simplest member function can be represented as a set of two pointers: one holding the physical memory address of the member function, and a second pointer that holds the this pointer. However, in cases like a virtual member function, multiple inheritance and virtual inheritance, the pointer to member must store additional information. Therefore, you can't cast pointers to members to ordinary pointers nor can you safely cast between pointers to members of different types.

[dirkwhoffmann]

I've cleaned up Moira a bit. The dasm table (Disassembler callback table) is now on the heap, because it is only needed in debug mode. The instr table (Instruction handler callback table) is still on the stack, because access time might be slightly faster (but I guess it's not really noticeable). The info table (Instruction infos) is not needed by the core CPU at all. Now, it is only build if the new Moira config option BUILD_INSTR_INFO_TABLE is set to true (it is false by default).

This reduces the CPU footprint as follows:

moira::Breakpoints : 40 bytes
moira::Watchpoints : 40 bytes
   moira::Debugger : 23656 bytes
      moira::Moira : 1072392 bytes

The instruction table occupies 1 MB on the stack (65536 "fat pointers").

[dirkwhoffmann]

I think there is nothing to do here at the moment. I'll close the issue for now.