FS#7264 - Build with -Os switch for coldfire targets.

Attached to Project: Rockbox
Opened by Nils Wallménius (nls) - Thursday, 07 June 2007, 10:14 GMT
Last edited by Nils Wallménius (nls) - Wednesday, 13 June 2007, 15:36 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch has a few different pieces.

0) Change the configure script to enable the -Os switch for m68k-elf-gcc.

1) It adds wrapper functions for memcpy and memset to some plugins to allow gcc to use those functions when optimizing.

2) Some small changes to playback_control to avoid memcpy here.

3) In playback.c change voicebuf to be unsigned char and adapt callbacks to this and make them accept the size parameter as size_t.

Both these changes were to avoid casting which made gcc throw a warning about dereferencing type-punned pointers.

Size drops about 20kb for rockbox.iriver for h300 builds.
I tested a build on my player and everything works as far as i can see.

Test built for a number of other targets without errors too.

I would like if some playback guru out there could take a look at this to see if my changes seem ok, i'm not very confident with changing playback.c and mp3_playback.c...

I tried to build with -O2 too but it fails on IRAM_FULL for the nsf codec.
   os.diff (12.5 KiB)
This task depends upon

Closed by  Nils Wallménius (nls)
Wednesday, 13 June 2007, 15:36 GMT
Reason for closing:  Accepted
Comment by Nils Wallménius (nls) - Thursday, 07 June 2007, 11:35 GMT
hmmm, forgot to bump plugin api version, that can be done when (if?) this gets committed.
Comment by Nils Wallménius (nls) - Sunday, 10 June 2007, 19:10 GMT
new patch,
* change the voice_info struct to have the same types as voicebuf and the size parameters to get rid of some uglyness

* bumped plugin api

Would really like to get someone else to test this, both on coldfire and archos targets
and some plaback.c guru to take a quick look at the changes in there too.
   os2.diff (13.2 KiB)
Comment by Nils Wallménius (nls) - Monday, 11 June 2007, 14:31 GMT
* Moved the wrapper functions to a macro called MEM_FUNCTION_WRAPPERS(api)
in a new file apps/plugins/lib/mem_function_wrappers.h
and put this in every plugin that needed it.
   os3.diff (16.2 KiB)
Comment by Magnus Holmgren (learman) - Monday, 11 June 2007, 16:53 GMT
Have you tried putting the wrappers in the codec lib?
Comment by Nils Wallménius (nls) - Monday, 11 June 2007, 20:59 GMT
Magnus, Yes I tried that and it worked, but there was one nasty thing. I had to call an
init function to point the api pointer correctly (like in playback_control.c for example).
Doing that worked fine but if I did not do it the linker didn't complain because the plugins
are linked against everything in the plugin lib and the plugins just froze when they were run.

The bad thing is that gcc decides itself which plugins should use the mem* functions and some
arbitrary change might trigger it which then will cause the plugin to freeze, not nice.
A workaround would be to add the init call to every plugin but I thought this was nicer.
I would like to hear suggestions on how to make it better though
Comment by Nils Wallménius (nls) - Tuesday, 12 June 2007, 15:26 GMT
hmm, I wanted to try another idea, to add the mem* functions themselves to the pluginlib, that did not work however.
I added


to the end of apps/plugins/lib/SOURCES

and everything compiled fine but plugins using any of the two functions froze when started...

as a test I also copied the .c files to the lib dir and changed SOURCES accordingly but with the same result.

Is it worth it to continue trying to make that work or should we go with one of the other ways?