Feature request/question (malloc return value)

[khevessy]

Hello Curt, would you consider implementing some checks for g_malloc() return value (in case of NULL)? Just asking if it is planned or not.

[jingoro2112]

I'm of two minds when it comes to checking the return value of malloc.. on the one hand it feels like a good idea because running out of memory is definitely something you want to know about. The second is.. what now? There is no possible recovery, the program is about to crash and burn. Sure there are some "because I ran out of memory!" logging that could be added I suppose, but I have yet to be on any real project where that actually was worth anything. We always knew it crashed, and why.

So adding an "if(!null)" on every malloc? just so it can always be false except when it's true and no one cares because nothing can be done anyway?

Sure, for some mission-critical systems it does make some sense I suppose, maybe some kind of global process shutdown.. but we already have that with any system incoprorating an MMU so...? embedded maybe? I could see that, which is why I added:

typedef void (WR_ALLOC)(size_t size); typedef void (WR_FREE)(void ptr); void wr_setGlobalAllocator( WR_ALLOC wralloc, WR_FREE wrfree );

In the first place, if you need it, add it :) but I don't plan to ever implement it as the default.

-Curt

On Tue, Jun 11, 2024 at 9:59 AM Karel @.***> wrote:

Hello Curt, would you consider implementing some checks for g_malloc() return value (in case of NULL)? Just asking if it is planned or not.

— Reply to this email directly, view it on GitHub https://github.com/jingoro2112/wrench/issues/33, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIKA4ZTWWXZIRIARD6AU3ZG37EVAVCNFSM6AAAAABJENA5K2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGM2DMNJRHAYTIMY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[khevessy]

Well I am using STM32H7 without MMU so I think here it makes sense. Thanks for response.

[ohordiichuk]

Checking malloc return value is definitely worths it. Crashing is not a good option at all. Specifically for microcontrollers with memory constraints and lack of good debugging tools. This is the way all modern C libraries are written. If they require malloc - they return error in case of no memory.

For example in our project we are having a custom heap that's dedicated to the wrench. We don't want wrench script to destroy whole application, so instead we allocate 10Kb of memory for wrench VM. If the application exceeds this memory - it means an invalid script was uploaded to the microcontroller. Which is a possible error, because it's being uploaded by the user and there is always a human factor. In this case the application must continue running as nothing happened, but report an error to the user and system logs, that it exceeded the memory! It shouldn't crash, this is the worst it can do.

In our case I found a workaround for this. Since we execute the VM in a separate thread of RTOS, in case malloc failed and with wrench malloc hook - we can simply abort() the thread. This is not a clean way of course, but may be help you.

However from source code it's clear that it will require to change it in multiple function signatures, also it seems it will break some library's logic and tests should be changed\added. So it will be a time consuming task for sure... May be later I can contribute to this, but unfortunately not now, since busy with Emscripten port and other tasks. But I think this is a must have feature in general

[jingoro2112]

I can see that use-case. I would have to visit everywhere malloc is called and do something sensible if it failed. I'll make a mental note.

On Wed, Jun 12, 2024 at 1:43 PM ohordiichuk @.***> wrote:

Checking malloc return value is definitely worths it. Crashing is not a good option at all. Specifically for microcontrollers with memory constraints and lack of good debugging tools. This is the way all modern C libraries are written. If they require malloc - they return error in case of no memory.

For example in our project we are having a custom heap that's dedicated to the wrench. We don't want wrench script to destroy whole application, so instead we allocate 10Kb of memory for wrench VM. If the application exceeds this memory - it means an invalid script was uploaded to the microcontroller. Which is a possible error, because it's being uploaded by the user and there is always a human factor. In this case the application must continue running as nothing happened, but report an error to the user and system logs, that it exceeded the memory! It shouldn't crash, this is the worst it can do.

In our case I found a workaround for this. Since we execute the VM in a separate thread of RTOS, in case malloc failed and with wrench malloc hook

• we can simply abort() the thread. This is not a clean way of course, but may be help you.

However from source code it's clear that it will require to change it in multiple function signatures, also it seems it will break some library's logic and tests should be changed\added. So it will be a time consuming task for sure... May be later I can contribute to this, but unfortunately not now, since busy with Emscripten port and other tasks. But I think this is a must have feature in general

— Reply to this email directly, view it on GitHub https://github.com/jingoro2112/wrench/issues/33#issuecomment-2163588984, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIKA7S3RQDTF4KGKATADTZHCCENAVCNFSM6AAAAABJENA5K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRTGU4DQOJYGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

[jingoro2112]

After thinking on this a bit I am convinced it is a worthwhile addition.

define WRENCH_HANDLE_MALLOC_FAIL

will be incoming in 6.1.0 as a major feature inclusion, I have a working implementation now but it needs some corners filled in. In particular getSVA() is assumed to succeed everywhere it's used, which is about 30 places so I will have to visit them all.

Note this adds a small bit of overhead to every instruction (one if{} check) since a malloc failure needs to halt the vm on the instruction it happens on to be of any use.

[jingoro2112]

6.0.1 implements this, graceful recovery from g_malloc returning null in all cases, see:

/************************************************************************
for embedded systems that need to know if they have run out of memory,

WARNING: This imposes a small if() check on EVERY INSTRUCTION so the
malloc failure is detected the moment it happens, but guarantees
graceful exit if g_malloc() ever returns null
*/
//#define WRENCH_HANDLE_MALLOC_FAIL

[khevessy]

Of course that proper library should implement the check, I just didn't want to argue with you. Thank you for the implementation, you are so fast!

[jingoro2112]

As I tell my kids: Arguing is good, fighting is bad. Arguing is where you both explain your ideas and each other listens, fighting is where you both shout about why you are right.

Make a good argument and I'll change my mind, like I did here.

On Fri, Jun 14, 2024 at 4:01 AM Karel @.***> wrote:

Of course that proper library should implement the check, I just didn't want to argue with you. Thank you for the implementation, you are so fast!

— Reply to this email directly, view it on GitHub https://github.com/jingoro2112/wrench/issues/33#issuecomment-2167472375, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIKA5A4Z3RQG5SI2KCNTTZHKPMDAVCNFSM6AAAAABJENA5K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRXGQ3TEMZXGU . You are receiving this because you modified the open/close state.Message ID: @.***>

[ohordiichuk]

Thanks for the update, Curt!

Could you please clarify what will happen with wr_run, wr_callFunction if there is no memory? From source code I see parts like this:

        if ( !table->va )
        {
            table->p2 = INIT_AS_INT;
            return;
        }

what will happen with bytecode execution? If I understand this code right it makes the second register as INT. Will this INIT_AS_INT immediately stop execution (runtime error) of the bytecode later after function return or what will happen next?

If it silently continues execution just with the default register value, then I think this is a bad design, because the outcomes of the bytecode execution will be unpredictable.

[jingoro2112]

To answer your question, wr_run() will return null and wr_getLastError(...) will show: WR_ERR_malloc_failed

The way this works is that any time g_malloc returns null, the global flag: g_mallocFailed is set true. This flag is inspected on every instruction (refer to the CONTINUE macro) so that if it ever becomes true the VM immediately detects this and exits gracefully.

since the main way memory is allocated is through garbage-collected objects (with getSVA()) if that routine ever detects null g_malloc it has already set the flag, so the job of the rest of the code is just not crash which is what the code snippet you showed does. It puts a good state into the table value and returns, knowing the next CONTINUE will be returning from the vm.

Try it, let me know how it goes.

-Curt

On Fri, Jun 14, 2024 at 11:41 AM ohordiichuk @.***> wrote:

Thanks for the update, Curt!

Could you please clarify what will happen with wr_run, wr_callFunction if there is no memory? From source code I see parts like this:

  if ( !table->va )
  {
      table->p2 = INIT_AS_INT;
      return;
  }

what will happen with bytecode execution? If I understand this code right it makes the second register as INT. Will this INIT_AS_INT immediately stop execution (runtime error) of the bytecode later after function return or what will happen next?

If it silently continues execution just with the default register value, then I think this is a bad design, because the outcomes of the bytecode execution will be unpredictable.

— Reply to this email directly, view it on GitHub https://github.com/jingoro2112/wrench/issues/33#issuecomment-2168295102, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIKA75GT33Y2LC3MXLE2TZHMFJPAVCNFSM6AAAAABJENA5K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGI4TKMJQGI . You are receiving this because you modified the open/close state.Message ID: @.***>