Re: [openMSX-commits] [SCM] wxCatapult branch master updated. RELEASE_0_9_1-70-g50eb4b8

[Wouter V. <ver...@gm...>]

On Mon, Aug 5, 2013 at 11:05 AM, joxy <jo...@us...> wrote:

> commit 50eb4b828ce7de186cdc51d92ec965cc6e7503f4
> Author: joxy <egp...@gm...>
> Date:   Mon Aug 5 18:05:01 2013 +0900

> diff --git a/src/openMSXController.cpp b/src/openMSXController.cpp
> index 78bdeff..9817d98 100644
> --- a/src/openMSXController.cpp
> +++ b/src/openMSXController.cpp
> @@ -23,6 +23,9 @@
>  #include <config.h>
>  #include "PipeConnectThread.h"
>  #include <process.h>
> +
> +//for malloc
> +#include <malloc.h>
>  #else
>  #include <unistd.h>
>  #include <wx/process.h>
> @@ -325,7 +328,12 @@ void openMSXController::WriteCommand(wxString msg, TargetType target)
>
>         auto len = strlen(reinterpret_cast<const char*>(buffer));
>         auto len2 = 9 + len + 11;
> -       char* cmd = static_cast<char*>(alloca(len2));
> +#if !defined(__WXMSW__)
> +#define CA_ALLOC(a) alloca(a)
> +#else
> +#define CA_ALLOC(a) malloc(a)
> +#endif
> +       char* cmd = static_cast<char*>(CA_ALLOC(len2));
>         memcpy(cmd, "<command>", 9);
>         memcpy(cmd + 9, buffer, len);
>         memcpy(cmd + 9 + len, "</command>\n", 11);

This is wrong, malloc() and alloca() are very different things:

malloc()
 -> allocates memory on the heap
 -> memory should later be freed with a call to free()
alloca()
 -> allocates memory on the stack
 -> the memory is automatically freed when the current function returns
 -> because the memory is allocated on the stack, you shouldn't
     allocate too big memory blocks (at most a couple of kilo bytes)
 -> alloca() is much more efficient than malloc()

Unfortunately alloca() doesn't exist in windows, but the function
_alloca() (with exactly the same semantics) does exist in windows.
I overlooked this fact because alloca() is already used in openMSXController.cpp
(I failed to see that it was in linux specific code, sorry).

In openMSX itself, instead of alloca() we use VLAs (variable length
arrays). This is a C99 feature. Gcc does support it, but visual studio
does not. I remembered that in openMSX we emulate VLAs with
alloca(). So that's why i thought it was ok to use alloca() also here.
(But again I overlooked that the function is actually called _alloca()
in windows). If you wish you can look at src/utils/vla.hh in the openmsx
source tree for some more background information.

Please revert or fix this commit. If you're not comfortable with this code
then I'll do it this evening when I'm back at home.

Thanks.

Wouter


BTW: I know I told you to not be afraid to make changes (and make
mistakes). But in this case you clearly knew you were doing something
wrong (because you immediately opened a tracker item for it). I prefer
that in such cases you first check with someone else before introducing
buggy code. I know you only wanted to fix a broken windows build.
I'm sorry, I should have been more careful myself.