add symbol lookup functions for dfsdl and dfimg

[dhthwy]

I want something like this.

[myk002]

do you actually need the address of the function instead of an API to call through to that function (the pattern for all the existing DFSDL functions)?

[dhthwy]

I don't know how many I will ultimately need and the list may change:

int Console_Init(void* (*sdl_resolver)(const char*), img_resolver...)
{
      resolve_sdl_symbols(sdl_resolver);
      ...
}

#if defined(CONSOLE_SDL_LINK_AT_RUNTIME) && (CONSOLE_SDL_LINK_AT_RUNTIME) == 1
#define CONSOLE_SYMBOL_ADDR(sym) nullptr
#else
#define CONSOLE_SYMBOL_ADDR(sym) ::sym
#endif

#define CONSOLE_DEFINE_SYMBOL(sym) decltype(sym)* sym = CONSOLE_SYMBOL_ADDR(sym)

CONSOLE_DEFINE_SYMBOL(SDL_ConvertSurfaceFormat);
CONSOLE_DEFINE_SYMBOL(SDL_CreateRenderer);
CONSOLE_DEFINE_SYMBOL(SDL_CreateRGBSurface);
CONSOLE_DEFINE_SYMBOL(SDL_CreateTexture);
CONSOLE_DEFINE_SYMBOL(SDL_CreateTextureFromSurface);
CONSOLE_DEFINE_SYMBOL(SDL_CreateWindow);
CONSOLE_DEFINE_SYMBOL(SDL_DestroyRenderer);
CONSOLE_DEFINE_SYMBOL(SDL_DestroyTexture);
CONSOLE_DEFINE_SYMBOL(SDL_DestroyWindow);
CONSOLE_DEFINE_SYMBOL(SDL_free);
CONSOLE_DEFINE_SYMBOL(SDL_GetClipboardText);
CONSOLE_DEFINE_SYMBOL(SDL_GetError);
CONSOLE_DEFINE_SYMBOL(SDL_GetEventFilter);
CONSOLE_DEFINE_SYMBOL(SDL_GetModState);
CONSOLE_DEFINE_SYMBOL(SDL_GetRendererOutputSize);
CONSOLE_DEFINE_SYMBOL(SDL_GetWindowFlags);
CONSOLE_DEFINE_SYMBOL(SDL_GetWindowID);
CONSOLE_DEFINE_SYMBOL(SDL_HideWindow);
CONSOLE_DEFINE_SYMBOL(SDL_iconv_string);
CONSOLE_DEFINE_SYMBOL(SDL_InitSubSystem);
CONSOLE_DEFINE_SYMBOL(SDL_MapRGB);
CONSOLE_DEFINE_SYMBOL(SDL_memset);
CONSOLE_DEFINE_SYMBOL(SDL_RenderClear);
CONSOLE_DEFINE_SYMBOL(SDL_RenderCopy);
CONSOLE_DEFINE_SYMBOL(SDL_RenderDrawRect);
CONSOLE_DEFINE_SYMBOL(SDL_RenderFillRect);
CONSOLE_DEFINE_SYMBOL(SDL_RenderPresent);
CONSOLE_DEFINE_SYMBOL(SDL_RenderSetIntegerScale);
CONSOLE_DEFINE_SYMBOL(SDL_RenderSetViewport);
/* CONSOLE_DEFINE_SYMBOL(SDL_PointInRect); inline defined in SDL_rect.h header */ 
CONSOLE_DEFINE_SYMBOL(SDL_SetClipboardText);
CONSOLE_DEFINE_SYMBOL(SDL_SetColorKey);
CONSOLE_DEFINE_SYMBOL(SDL_SetEventFilter);
CONSOLE_DEFINE_SYMBOL(SDL_SetHint);
CONSOLE_DEFINE_SYMBOL(SDL_SetRenderDrawColor);
CONSOLE_DEFINE_SYMBOL(SDL_SetTextureBlendMode);
CONSOLE_DEFINE_SYMBOL(SDL_SetTextureColorMod);
CONSOLE_DEFINE_SYMBOL(SDL_SetWindowMinimumSize);
CONSOLE_DEFINE_SYMBOL(SDL_ShowWindow);
CONSOLE_DEFINE_SYMBOL(SDL_StartTextInput);
CONSOLE_DEFINE_SYMBOL(SDL_StopTextInput);
CONSOLE_DEFINE_SYMBOL(SDL_UpperBlit);
CONSOLE_DEFINE_SYMBOL(SDL_UpdateTexture);
CONSOLE_DEFINE_SYMBOL(SDL_QuitSubSystem);

[myk002]

it's fine to extend the DFSDL API as needed. you might want to refactor to reduce the boilerplate if you'll be adding so many functions, though.

[lethosor]

I'm not opposed to adding something like this for development/debugging purposes, but any code we integrate should call out to "proper" functions defined in DFSDL so that future code can benefit from having those functions available.

[dhthwy]

I don't object to adding these to DFSDL. I guess my only concern would be it might eventually include all SDL functions if there's plugins, etc making lots of use of SDL.

[dhthwy]

Do we want DFSDL init() to resolve all SDL functions on startup, including dozens that some component may or not use?

What if we did late binding for the functions that aren't common and not used in core?

DFSDL::Uncommon(int x) {
    if (!g_SDL_Uncommon) {
        // lookup and bind
    }
    return ...
}

[myk002]

is there any significant resource usage or time penalty? linking symbols should be pretty fast

[dhthwy]

Storage should already be allocated, right? A few dozen pointers worth. I'd say no on resource usage.

Time to call a few dozen symbol lookups... probably still in the nanoseconds on any modern puter. dlsym and GetProcAddress should be highly optimized. Probably not worth benching.

[lethosor]

Binding everything at init time is almost certainly faster than checking if binding is necessary at call time (i.e. the late bindings you proposed), at least beyond a certain number of calls.

dlsym() is not particularly fast. Maybe microseconds, maybe closer to milliseconds with our number of calls.

I think the main reason we don't just "bind everything" is the number of stubs that would have to be written manually. C barely offers any way to programmatically generate these stubs; the closest approaches I've seen involve defining function prototypes with macros which can be redefined to process each function in other ways, but the SDL headers aren't defined in that way. C++ will get us closer, but there's still a fair amount of manual effort.

[dhthwy]

we can't forward declare enums, so we'll have to include some SDL headers in DFSDL.h?, if want this. :/

[myk002]

we can't forward declare enums

True, but we also don't technically need to use the enums in the function prototypes. We can substitute the underlying integer types.

Do we already do that for the other functions? pixel_format looks like it could be an enum..

[dhthwy]

I was hoping we wouldn't have to specify the signatures manually 3 times, and let SDL headers be the canonical source for that. But since we can't forward declare enums, and we don't want to include SDL headers. We will to have a long list of pointer declarations that I was hoping to avoid.

The only other alternative that I can think of is to make our own enums:

int DFSDL::DFSDL_SetTextureBlendMode(SDL_Texture* texture, DFSDL_BlendMode blendMode) {
return g_SDL_SetTextureBlendMode(texture, (SDL_BlendMode)blendMode);
}

but I don't think I like that approach.

[dhthwy]

Oh, but some functions are, inline, defined in SDL headers. Do we copy and paste those in?

[dhthwy]

These two headers from SDL included by DFSDL.h, primarily just define enums aside for a 'setup' header - I don't know if the configuration done by begin_code.h does anything bad to dfhack.

#include <SDL_blendmode.h>
#include <SDL_keycode.h>

So they aren't pulling in the world.

The implementation file only needs:

#include <SDL_messagebox.h>
#include <SDL_hints.h>
#include <SDL_events.h>
#include <SDL_clipboard.h>
#include <SDL_render.h>
#include <SDL_surface.h>
#include <SDL_pixels.h>

This assuming we don't do all the declarations ourselves.

[dhthwy]

I guess it isn't safe to include any SDL headers in DFSDL.h, since it has this.
Why can't they just have a header for the enum, wth.

#ifdef _WIN64
/* Use 8-byte alignment on 64-bit architectures, so pointers are aligned */
#pragma pack(push,8)
#else
#pragma pack(push,4)
#endif
#endif /* Compiler needs structure packing set */

oh, but it reverses that at the end. ugh.

[dhthwy]

SDL_PointInRect defined in SDL_rect.h, I don't know why that's included there. Wasn't supposed to be.

Here:

 #define bind(handle, name) \
        g_##name = (decltype(g_##name))LookupPlugin(handle, #name); \
        if (!g_##name) { \
            std::cerr << "Debug: LookupPlugin failed for " #name "\n"; \
            out.printerr("DFHack could not find: " #name "\n"); \
            return false; \
        }

The out.printerr line never prints the error to my terminal launching with ./dfhack, but std::cerr does
Debug: LookupPlugin failed for SDL_PointInRect

See: #4814

[myk002]

@lethosor what do you think about including SDL header files in DFSDL.h? I personally kept them out of there since it seemed "messy" and to avoid potential contamination of the rest of the build, but I'm not sure if that's a sufficiently valid concern to force a redeclaration of all the enums or other hacky workarounds.

[lethosor]

We've done that before. I don't really have an issue with it for things like enums. But since SDL is C, defining "stubs" for the enums we need is pretty straightforward, if we don't also need the enum values.

[dhthwy]

enum SDL_Keymod; // Can't do this, forbidden.
typedef enum SDL_Keymod SDL_Keymod; // Can't do this, forbidden.

Ok - not forbidden - if specify storage, but

In included file: typedef redefinition with different types ('enum SDL_BlendMode' vs 'SDL_BlendMode')
enum SDL_Keymod : unsigned int;
enum SDL_bool : unsigned int;
enum SDL_BlendMode : unsigned int;

Ok, try this then:

enum SDL_Keymod : unsigned int;
enum SDL_bool : unsigned int;
enum SDL_BlendMode : unsigned int;
typedef enum SDL_Keymod SDL_Keymod;
typedef enum SDL_bool SDL_bool;
typedef enum SDL_BlendMode SDL_BlendMode;

Same error:

In included file: typedef redefinition with different types ('enum SDL_BlendMode' vs 'enum SDL_BlendMode')

/home/dh/projects/dfhack/depends/SDL2/SDL2-2.26.2/include/SDL_keycode.h:354:3: error: conflicting declaration ‘typedef enum SDL_Keymod SDL_Keymod’
  354 | } SDL_Keymod;
      |   ^~~~~~~~~~
/home/dh/projects/dfhack/library/include/modules/DFSDL.h:75:25: note: previous declaration as ‘typedef enum SDL_Keymod SDL_Keymod’
   75 | typedef enum SDL_Keymod SDL_Keymod;

I even tried to patch SDL itself by giving it a storage type:

typedef enum : unsigned int
{
    SDL_BLENDMODE_NONE = 0x00000000,     /**< no blending
                                              dstRGBA = srcRGBA */
    SDL_BLENDMODE_BLEND = 0x00000001,    /**< alpha blending
                                              dstRGB = (srcRGB * srcA) + (dstRGB * (1-srcA))
                                              dstA = srcA + (dstA * (1-srcA)) */
    SDL_BLENDMODE_ADD = 0x00000002,      /**< additive blending
                                              dstRGB = (srcRGB * srcA) + dstRGB
                                              dstA = dstA */
    SDL_BLENDMODE_MOD = 0x00000004,      /**< color modulate
                                              dstRGB = srcRGB * dstRGB
                                              dstA = dstA */
    SDL_BLENDMODE_MUL = 0x00000008,      /**< color multiply
                                              dstRGB = (srcRGB * dstRGB) + (dstRGB * (1-srcA))
                                              dstA = (srcA * dstA) + (dstA * (1-srcA)) */
    SDL_BLENDMODE_INVALID = 0x7FFFFFFF

    /* Additional custom blend modes can be returned by SDL_ComposeCustomBlendMode() */

} SDL_BlendMode;

I believe this may work in MSVC, but GCC is being a dick. 'They' claim it is because it requires the storage type for memory allocation, type safety, etc. Well the storage type was provided and the compiler is still throwing a hissy fit.

[dhthwy]

It should work? Provided we don't include any SDL headers - I guess that's probably what you were thinking. As I mentioned previously, it would be nice to use SDL headers as the canonical source for the declarations which will catch mismatch errors at compile time, and prevents us from having to repeat ourselves 3 times, instead of twice. And the long list of function pointer decls are a monster.

[dhthwy]

Arr. We won't be able to include SDL headers, anywhere, including 3rd party code, plugins if we attempt stubs for enums in the DFSDL header. Speaking of brittle. The more I spend on this, the more I realize headers are gonna be required for sanity.

This one is on SDL for making enum definitions pull in too much crap, and C++ for unnecessary type safety restrictions - we don't trust the programmer type crap.

[dhthwy]

We can try patching SDL like this:

In SDL_keycode.h:

//#include "SDL_stdinc.h"
#include <stdint.h>
#include "SDL_scancode.h"

/**
 *  \brief The SDL virtual key representation.
 *
 *  Values of this type are used to represent keyboard keys using the current
 *  layout of the keyboard.  These values include Unicode values representing
 *  the unmodified character that would be generated by pressing the key, or
 *  an SDLK_* constant for those keys that do not generate characters.
 *
 *  A special exception is the number keys at the top of the keyboard which
 *  always map to SDLK_0...SDLK_9, regardless of layout.
 */
//typedef Sint32 SDL_Keycode;
typedef int32_t SDL_Keycode;

In SDL_scancode.h:
Simply remove SDL_stdinc as it isn't used.

//#include "SDL_stdinc.h"

And for SDL_bool defined in SDL_stdinc.h:

#if 0
#ifdef __CC_ARM
/* ARM's compiler throws warnings if we use an enum: like "SDL_bool x = a < b;" */
#define SDL_FALSE 0
#define SDL_TRUE 1
typedef int SDL_bool;
#else
typedef enum
{
    SDL_FALSE = 0,
    SDL_TRUE = 1
} SDL_bool;
#endif
#endif

/**
 * A boolean false.
 *
 * \since This macro is available since SDL 3.0.0.
 *
 * \sa SDL_bool
 */
#define SDL_FALSE 0

/**
 * A boolean true.
 *
 * \since This macro is available since SDL 3.0.0.
 *
 * \sa SDL_bool
 */
#define SDL_TRUE 1

/**
 * A boolean type: true or false.
 *
 * \since This datatype is available since SDL 3.0.0.
 *
 * \sa SDL_TRUE
 * \sa SDL_FALSE
 */
typedef int SDL_bool;

Then we can:
typedef int SDL_bool in DFSDL.h

and include SDL_keymod.h and SDL_blendmode.h without including SDL_stdinc.h which includes quite a bit of stuff.

This is compiling and working for me.

But this approach doesn't play nicely with 3rd party code unless it uses the same headers.

[dhthwy]

This might be our culprit here. Nevermind. Tired I guess.
But this is the reason they insist on pulling in stdinc.h everywhere.

/* Check to make sure enums are the size of ints, for structure packing.
   For both Watcom C/C++ and Borland C/C++ the compiler option that makes
   enums having the size of an int must be enabled.
   This is "-b" for Borland C/C++ and "-ei" for Watcom C/C++ (v11).
*/

/** \cond */
#ifndef DOXYGEN_SHOULD_IGNORE_THIS
#if !defined(__ANDROID__) && !defined(__VITA__) && !defined(__3DS__)
   /* TODO: include/SDL_stdinc.h:174: error: size of array 'SDL_dummy_enum' is negative */
typedef enum
{
    DUMMY_ENUM_VALUE
} SDL_DUMMY_ENUM;

SDL_COMPILE_TIME_ASSERT(enum, sizeof(SDL_DUMMY_ENUM) == sizeof(int));

Nevermind.. sigh

[ab9rf]

we can't forward declare enums, so we'll have to include some SDL headers in DFSDL.h?, if want this. :/

this isn't entirely true, you can declare an "opaque" enum by declaring the enum with a underlying type (for sizing) but without enumerators (e.g. enum foo : int16_t;), and then later redeclare the enum with enumerators as long as the size is the same in the redeclaration as it was in the original. this is technically not a forward declaration because the enum is a complete type at this point, since its size is known and an incomplete type does not have a known size, but it conceptually works much the same as a declaration of a compound as an incomplete type

i actually think the problem here is the use of typedef enum X X; which is a namespace collision in C++. in C this is legal because enums live in their own namespace in C (you have to say enum X to refer to the enum with the name X), but in C++ enums, classes, and structs are in the same namespace as typedefs

[myk002]

@lethosor @ab9rf now that DFHack no longer shadows SDL entrypoints, is DFSDL still necessary? What exactly are we gaining by avoiding the link-time dependency?

[ab9rf]

@lethosor @ab9rf now that DFHack no longer shadows SDL entrypoints, is DFSDL still necessary? What exactly are we gaining by avoiding the link-time dependency?

You're right, we probably don't have to do an explicit runtime binding. The "downside" of using automatic binding is that the load of dfhack itself will fail if SDL doesn't bind, but if that happens we're going to be dead in the water anyway. The main disadvantage is that we won't be able to issue a useful diagnostic, since we'll never get loaded and DF will just continue as if were were not there. In any case, if SDL isn't available at runtime, either DF itself won't be working or Bay12 has done something truly unexpected, so I think we can probably safely switch to using automatic binding for SDL.

[myk002]

Ok, let's try that switch directly after we get -r2 out so we can beta test it. There will be build system changes that need to happen too, and those are always risky.

[lethosor]

There is a chance that we would need to put in some extra work to get SDL linking properly at build time for everyone, especially on Windows.

[dhthwy]

This is good news for plugins, etc. Thanks!

[ab9rf]

There is a chance that we would need to put in some extra work to get SDL linking properly at build time for everyone, especially on Windows.

I don't anticipate an issue as long as the dependencies specified in the DLL specifies the SDL libraries with no pathname or with a relative path name, and the SDL libraries have already been loaded by DF before attempting to map dfhooks is called. Since DF on Windows lists SDL2 and SDL2_image as load-time dependencies, there is no way that DF will ever launch without those basenames already loaded; the app won't launch at all if they can't be loaded.

We will need to provide an appropriate SDL2.lib and SDL2_image.lib to make the linker happy, but those should be in the SDL2 package already (and I've just confirmed that they are).