The free standing functions in std.loader work as expected, but the ExeModule class is just plain broken. There are two major failings:

1) The class is auto. I don't understand the reasoning behind this at all. Consider this code:

**************************************************************
import std.loader;
import std.c.stdio;

typedef int function(uint) pfSDL_Init;
typedef int function() pfSDL_Quit;
pfSDL_Init				SDL_Init;
pfSDL_Quit				SDL_Quit;

void loadLib()
{
	auto ExeModule em = new ExeModule("sdl.dll");
	SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
	SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
}

int main(char[][] args)
{
	ExeModule_Init();

	loadLib();
	printf("Initializing SDL\n");
	SDL_Init(0);
	SDL_Quit();	
	ExeModule_Uninit();
	return 0;
}
**************************************************************

When loadLib returns, em has gone out of scope and as it's an auto class the destructor is called. ExeModule's destructor releases the handle to the shared library, thereby invalidating the previously loaded function function addresses and causing an Access Violation.

2) The two-arg constructor is whacked:

**************************************************************
/// Constructs from an existing image handle
    this(in HXModule hModule, boolean bTakeOwnership)
    in
    {
        assert(null !== hModule);
    }
    body
    {
        if(bTakeOwnership)
        {
            m_hModule = hModule;
        }
        else
        {
	    version (Windows)
	    {
		char[] path = Path();
		m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
		if (m_hModule == null)
		    throw new ExeModuleException(GetLastError());
	    }
	    else version (linux)
	    {
		m_hModule = ExeModule_AddRef(hModule);
	    }
	    else
		static assert(0);
        }
    }
**************************************************************

First, when bTakeOwnership is true the same problem described in 1) arises. Second, when bTakeOwnership is false, the Windows version does something really whacky. Looking in the Path() method, you'll see the following:

uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName, 			 szFileName.length);
...
return szFileName[0 .. cch].dup;

The problem is that at this point, m_hModule is null. There is precondition check for this, but it will always fail if bTakeOwnership is false. If compiling without dbc you wind up loading a handle to the executable itself as the default behavior of GetModuleFileName with a null module is to store the exe name in szFileName. This will cause ExeModule.getSymbol to fail later on. And to top it off, the orginal module handle passed to the constructor is never used at all. So when bTakeOwnership is false, you are 100% gauranteed to fail either with an assertion failure in Path() in debug mode or an ExeModuleExeption in getSymbol() otherwise.

3) When an ExeModuleException is thrown via ExeModule.getSymbol() there is absolutely no way of knowing which symbol failed to load unless you wrap each call in a try...catch block, which just isn't practical. The only thing that is output is a system error code. At the very least ExeModule should concat the name of the symbol with the error code string. This also applies to the loading of the shared lib handles.

Currently I'm using the free standing functions, and was getting ready to submit improved exception messages when I stumbled into the other problems. I'll be happy to submit a more usable version (with Matthew's permission).

It's a bit hard to comment, since the version in Phobos is not the version I submitted to Walter. We had quite a disagreement on the implementation of the class, and on the provision of free functions and class.

I'm not abrogating responsibility, but just explaining my difficulties.

If you sketch two or three scenarios, preferably complete (small) progs, I can try them with _my_ implementation. If that's still out of whack - which may well be so - then I'll amend my impl to your requirements (assuming they're sensible, which I assume at the mo).

I'll also do a bit of documentation/rationale for the design (or my design of it), and then open that for comment.

The issue of the auto-ness of the class is certainly not something I can run away from. Walter suggested it should be auto, and I complied without thinking about it. Given the current - and I say current because I expect them to be relaxed sometime in the not so distant future - rules regarding auto, I agree that the class is flawed in this regard.

Can I say/do more? :-)

Cheers

Matthew


"Mike Parker" <aldacron71@yahoo.com> wrote in message news:cada7o$2nnh$1@digitaldaemon.com...
> The free standing functions in std.loader work as expected, but the ExeModule class is just plain broken. There are two major failings:
>
> 1) The class is auto. I don't understand the reasoning behind this at all. Consider this code:
>
> **************************************************************
> import std.loader;
> import std.c.stdio;
>
> typedef int function(uint) pfSDL_Init;
> typedef int function() pfSDL_Quit;
> pfSDL_Init SDL_Init;
> pfSDL_Quit SDL_Quit;
>
> void loadLib()
> {
> auto ExeModule em = new ExeModule("sdl.dll");
> SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
> SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
> }
>
> int main(char[][] args)
> {
> ExeModule_Init();
>
> loadLib();
> printf("Initializing SDL\n");
> SDL_Init(0);
> SDL_Quit();
> ExeModule_Uninit();
> return 0;
> }
> **************************************************************
>
> When loadLib returns, em has gone out of scope and as it's an auto class the destructor is called. ExeModule's destructor releases the handle to the shared library, thereby invalidating the previously loaded function function addresses and causing an Access Violation.
>
> 2) The two-arg constructor is whacked:
>
> **************************************************************
> /// Constructs from an existing image handle
>      this(in HXModule hModule, boolean bTakeOwnership)
>      in
>      {
>          assert(null !== hModule);
>      }
>      body
>      {
>          if(bTakeOwnership)
>          {
>              m_hModule = hModule;
>          }
>          else
>          {
>     version (Windows)
>     {
> char[] path = Path();
> m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
> if (m_hModule == null)
>     throw new ExeModuleException(GetLastError());
>     }
>     else version (linux)
>     {
> m_hModule = ExeModule_AddRef(hModule);
>     }
>     else
> static assert(0);
>          }
>      }
> **************************************************************
>
> First, when bTakeOwnership is true the same problem described in 1) arises. Second, when bTakeOwnership is false, the Windows version does something really whacky. Looking in the Path() method, you'll see the following:
>
> uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName,
> szFileName.length);
> ...
> return szFileName[0 .. cch].dup;
>
> The problem is that at this point, m_hModule is null. There is precondition check for this, but it will always fail if bTakeOwnership is false. If compiling without dbc you wind up loading a handle to the executable itself as the default behavior of GetModuleFileName with a null module is to store the exe name in szFileName. This will cause ExeModule.getSymbol to fail later on. And to top it off, the orginal module handle passed to the constructor is never used at all. So when bTakeOwnership is false, you are 100% gauranteed to fail either with an assertion failure in Path() in debug mode or an ExeModuleExeption in getSymbol() otherwise.
>
> 3) When an ExeModuleException is thrown via ExeModule.getSymbol() there is absolutely no way of knowing which symbol failed to load unless you wrap each call in a try...catch block, which just isn't practical. The only thing that is output is a system error code. At the very least ExeModule should concat the name of the symbol with the error code string. This also applies to the loading of the shared lib handles.
>
> Currently I'm using the free standing functions, and was getting ready to submit improved exception messages when I stumbled into the other problems. I'll be happy to submit a more usable version (with Matthew's permission).
>

On Sat, 12 Jun 2004 16:25:44 +1000, Matthew wrote:
> The issue of the auto-ness of the class is certainly not something I can run away from. Walter suggested it should be auto, and I complied without thinking about it. Given the current - and I say current because I expect them to be relaxed sometime in the not so distant future - rules regarding auto, I agree that the class is flawed in this regard.
> 

I take it you know something we don't about auto ;)

I'd like to request that auto not be changed. I think a 'relaxed' version of auto would be very useful, but when you need something as strict as auto, it would be nice to have it there.

Mike Swieton
__
Has this world been so kind to you that you should leave with regret? There
are better things ahead than any we leave behind.
	- C. S. Lewis

Gah! I'm an ignoramus.

The debate between myself and Walter were on ExeModule, not on MmFile. The guilt, such as it is, is all mine.

I'll try and take a look at it again right now.

"Mike Parker" <aldacron71@yahoo.com> wrote in message news:cada7o$2nnh$1@digitaldaemon.com...
> The free standing functions in std.loader work as expected, but the ExeModule class is just plain broken. There are two major failings:
>
> 1) The class is auto. I don't understand the reasoning behind this at all. Consider this code:
>
> **************************************************************
> import std.loader;
> import std.c.stdio;
>
> typedef int function(uint) pfSDL_Init;
> typedef int function() pfSDL_Quit;
> pfSDL_Init SDL_Init;
> pfSDL_Quit SDL_Quit;
>
> void loadLib()
> {
> auto ExeModule em = new ExeModule("sdl.dll");
> SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
> SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
> }
>
> int main(char[][] args)
> {
> ExeModule_Init();
>
> loadLib();
> printf("Initializing SDL\n");
> SDL_Init(0);
> SDL_Quit();
> ExeModule_Uninit();
> return 0;
> }
> **************************************************************
>
> When loadLib returns, em has gone out of scope and as it's an auto class the destructor is called. ExeModule's destructor releases the handle to the shared library, thereby invalidating the previously loaded function function addresses and causing an Access Violation.
>
> 2) The two-arg constructor is whacked:
>
> **************************************************************
> /// Constructs from an existing image handle
>      this(in HXModule hModule, boolean bTakeOwnership)
>      in
>      {
>          assert(null !== hModule);
>      }
>      body
>      {
>          if(bTakeOwnership)
>          {
>              m_hModule = hModule;
>          }
>          else
>          {
>     version (Windows)
>     {
> char[] path = Path();
> m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
> if (m_hModule == null)
>     throw new ExeModuleException(GetLastError());
>     }
>     else version (linux)
>     {
> m_hModule = ExeModule_AddRef(hModule);
>     }
>     else
> static assert(0);
>          }
>      }
> **************************************************************
>
> First, when bTakeOwnership is true the same problem described in 1) arises. Second, when bTakeOwnership is false, the Windows version does something really whacky. Looking in the Path() method, you'll see the following:
>
> uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName,
> szFileName.length);
> ...
> return szFileName[0 .. cch].dup;
>
> The problem is that at this point, m_hModule is null. There is precondition check for this, but it will always fail if bTakeOwnership is false. If compiling without dbc you wind up loading a handle to the executable itself as the default behavior of GetModuleFileName with a null module is to store the exe name in szFileName. This will cause ExeModule.getSymbol to fail later on. And to top it off, the orginal module handle passed to the constructor is never used at all. So when bTakeOwnership is false, you are 100% gauranteed to fail either with an assertion failure in Path() in debug mode or an ExeModuleExeption in getSymbol() otherwise.
>
> 3) When an ExeModuleException is thrown via ExeModule.getSymbol() there is absolutely no way of knowing which symbol failed to load unless you wrap each call in a try...catch block, which just isn't practical. The only thing that is output is a system error code. At the very least ExeModule should concat the name of the symbol with the error code string. This also applies to the loading of the shared lib handles.
>
> Currently I'm using the free standing functions, and was getting ready to submit improved exception messages when I stumbled into the other problems. I'll be happy to submit a more usable version (with Matthew's permission).
>

Matthew wrote:

> If you sketch two or three scenarios, preferably complete (small) progs, I can
> try them with _my_ implementation. If that's still out of whack - which may well
> be so - then I'll amend my impl to your requirements (assuming they're sensible,
> which I assume at the mo).

Thanks, Matthew. In addition to the first example I posted, there is this:

***********************************************************
import std.loader;
import std.c.stdio;

typedef int function(uint) pfSDL_Init;
typedef int function() pfSDL_Quit;
pfSDL_Init				SDL_Init;
pfSDL_Quit				SDL_Quit;

HXModule hlib;

void loadLib()
{
	auto ExeModule em = new ExeModule(hlib, false);
	SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
	SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
}

int main(char[][] args)
{
	ExeModule_Init();
	hlib = ExeModule_Load("sdl.dll");
	loadLib();	
	SDL_Init(0);
	SDL_Quit();
	ExeModule_Release(hlib);	
	ExeModule_Uninit();
	return 0;
}
***********************************************************

In this scenario, an exception will be thrown when trying to load SDL_Init. The reason being that the ExeModule constructor will ignore the first parameter and actully will load a handle to the executable itself.

Changing the second constructor parameter to true will exhibit the same behavior as the first example I posted (Access Violation). ExeModule will store the hlib reference, closing it via the destructor when loadLib() exits. This will cause the function pointers to be invalid.

I suggest the following changes to ExeModule:

1) do away with the autoness
2) remove the Path() method
3) always store the reference to the module passed to the constructor, regardless of the value of bTakeOwnership
4) create a new class member (m_isOwner) to store the value of bTakeOwnership. Test this in the destructor and only call close() if m_isOwner == true.

And one last thing that would be useful is a change to ExeModuleException and the way it is used. Changing the second constructor to something like this:

this(char[] message, uint errcode)
{
    super(message ~ " [Sys Error: " ~ SysError.msg(errcode) ~ "]");
}

And then doing something like this in when a shared lib or a proc address canot be loaded:

throw new ExeModuleException("Failed to find shared lib symbol " ~ symbolName, GetLastError());

will greatly assist in debugging.