Is newlib's realloc() have issues ? & optimisation discussion

There is one function from original code which works fine on linux/win32, which aim to get the ogg file and decode it to the memory:




When i compiled it for aos4 via gcc 8.2.0, with usage latest beta newlib 53.50 and with -O3 optimisation (don't tested on puclib ones, but probabaly that not related), i have got some weird issues, named that buffer epanding (reallocation) is never ends. It keep bring me "2048" for rc and size in a loop, till not eat all my memory (whole 2gb) and then just crashes.

Once i change "long allocated = 4*1024*16;" on "long allocated = 1024*1024*16;", so to avoid many reallocs by enlarge the buffer , it start to works ! It just eat as expected about 30-40mb of memory for my decompressed ogg file, and continue futher.

Question is : WTF? Is there something amigaos4 specific should be done about ? Is static_cast may cause issues there ?

Any help and ideas apprecated. Some shorted test-cases will be interesting to check as well, just dunno where to look at.

Thanks !

@kas1e

There was a thread on the Hyperion forum about Newlib's realloc() having some problem but I don't remember the details. I vaguely recollect Steven Solie mentioned that the bug occurs rarely and that fixing it is not high priority.

@Trixie
Oh, i found it !

http://forum.hyperion-entertainment.c ... f=14&t=1591&hilit=realloc

Damn !

I even see you asking there what was in the 4458 , but there is indeed nothing important, only asking for necessary stuff to be implemented in kernel, before realloc() thing can be fixed.

Through i can see in that thread Salas00 bring some workarounds for it, so maybe i better reuse it instead and be done with it..

Through if it can be replaced like this, why not replace it like this in newlib itself ? Probabaly there will be other issues arise then of course..

@Trixie
Well i was over optimistic, it seems that i can't simple made replacement for realloc() in the big projects , which indirectly or directly call malloc() (as newlib is shared library, and it all can cause crashes, as need to replace all other memory functions too).

In turn it mean that it too much hassle and better or get rid of realloc() at all in that code, or, keep 1024x1024x16, which mean it eat up for realloc() by itself about 100mb, which then returned right after reallocs loop done.

I even trying 2048x1024x16, and then it eat up just 30-40mb only.

In other words : don't use newlib's realloc() ever if possible ! It works, and memory after realloc() loop ends are freed, but in the process it take A LOT of memory, and can easyly fillup whole memory if you trying to realloc looots of small chunks.

@kas1e

Is this realloc() bug looked after/has an entry in the database or is this some kind of "will never get fixed, just don't use realloc() at all" situation?

I guess this may be the reason why a certain engine fails/crashes/freezes in ScummVM as the engines behaviour on how to function completely covers your description...

@Raziel
As far as i understand, newlib's realloc() works, it just for its own needs it take much memory and in worse situations when there a looot of small chunks, it can lead to filling up of whole memory and a freeze as result.

You may try to run some memory monitor (i use zzd10h's dockie for RAM), and run scummwm over it. If before you crash you will see that all your memory is filled up - then it is. But if not, then it something else

I overrode malloc, free, and realloc in JamVM. Can’t remember the details, but I think I ended up using the memory pools (even though the code had Agee other methods in ifdef’s).

Hmm... looking at the code I seem to have changed the functions to call my memory functions, thought I’d made some defines for it.

Anyway, below is the code. Look for mymalloc, myfree, and myrealloc:
src/os/amiga/memory.c

And here is where I had to change it. JamVM already had its own memory allocation functions, so it didn’t call them directly:
src/alloc.c

@kas1e
Why are you using -O3, seems like asking for trouble? I'd try using just -Os (or even no -O at all). And then add better optimisation at the end, after everything is working.

@ChrisH
With -O3 optimisation i tested already many big projects which prove that at least on recent gcc (8.2.0) all fine. Maybe there can be problems in some situations, but for my own needs i find none.

In end of all that realloc() is wellknown bug/feature not related to optimisation, which not fixed for long.

@Salass00
Do you think it possible to deal with realloc in newlib, so it will not eat lots of memory when realloc() do reallocation of many small chunks ? Or its better to completely avoid it ?

@kas1e
Quote:

I think you are right here, using optimisation enables a quite afew imporant warnings that are not other detected, and the only time it causes problems is when trying to trace Grim Reapers, then you can disabled it for that case and rebuild.

Quote:

I would avoid it or use clib2 as advised else where .

@ChrisH
Quote:
I second kas1e and broadblues. And I for myself do more or less the opposite of your approach:
I always use -O3 (or even more) optimizations and only lower or disable them if I run into trouble and need to debug in depth.
The reason is that very high compiler optimization modes usually don't cause any problems at all (at least if you're using more or less recent gcc) - if your code is truely clean and doesn't contain subtle bugs, which often won't show up when optimizations are disabled.
Really, in 95% of the cases where -O3 seemed to be the cause for problems it turned out that in reality it was a side-effect of a bug made by myself. So from my experience -O3 is a nice tool to uncover bugs

And right, there is some prove with which i meet today about way of doing optimisation (when no optimisation just hide real issues):

Porting one game, and it seems to works. Added -O3, and it start crashes on some `strcpy(s,&s[strlen(s)-4]);` string. While with no optimisation it works for sure (optimisation with -O3 probabaly not only replace it with memmove, but doing something else ?).

Then, i create simple test case which show the problem (test case with parts of code from the game):



To run it you need directory "music" and put there some .mp3 files (or just text files with .mp3 or .ogg names).

Then, if you compile it without optimisation, it didn't crash. Will works fine (visually). If you compile it with -O3, it will crashes. But not only that , -Wall will say you when optimisation enabled:

Quote:

So, anyone can see what the problem is with that code and why it fail with -O3 ? Replacing strcpy(s,&s[strlen(s)-4]); with just memmove(s, s+strlen(s)-4, 5); make it works even with -O3.

Anyone see error in original code ?

@kas1e
Just don't do it, it's illegal, results are undefined / depend on implementation if you use overlapping src / dst for strcpy, simple as that. Your favorite strcpy-man-page will tell you.
That is, if the code would be correct at all, actually it's completely bogus:

strcpy(s,&s[strlen(s)-4]);

At that point s is completely undefined, so the overlapping issue is only the top of the iceberg.
What the author meant is most likely

strcpy(s,&s1[strlen(s1)-4]);

Why your snippet produces anything working at all with -O0 is beyond me, to be honest, must be some weird funny stack-coincidence.

But well, yet another reason why to embrace -O3

@Daniel
Yeah, what mean that usage of -O3 is very good idea all the time.

I also found another bug because of -O3 in that trashy memory game, when no optimisation enabled, app can be opened and in window mode, and in fullscreen. When -O3 enabled, then again something trashes, and only fullscreen works, and mouse cursor distored. So it for sure 100% bugs, which caused not by -O3, but because of -O3 they didn't left unnoticed.

ps. and check the mail for some good news :)

@kas1e
Quote:
Holy cow! Yeah, that day work was really worth it