Skip to content

Conversation

@escalade
Copy link
Contributor

With OPTIMIZATIONS=normal set in project options, Kodi crashes with segmentation fault when adding to the library. Reproduced only on arm/RPi2, Generic seems fine. Using -Os instead of -O2 avoids the issue.

References:

https://forum.libreelec.tv/thread-302-post-9754.html#pid9754
http://forum.odroid.com/viewtopic.php?f=62&t=20345

@MilhouseVH
Copy link
Contributor

Is this fixed by gcc-6?

@escalade
Copy link
Contributor Author

Yes, but with GCC6 there's an issue with reading FLAC tags (for generic as well). Was thinking to test 6.2 today.

@popcornmix
Copy link
Contributor

Sounds like a compiler bug, so moving backwards/forwards to a version that doesn't have the bug may be a better choice than changing optimisation level (which could affect performance and will probably be forgotten about long after the compiler bug is fixed).

@popcornmix
Copy link
Contributor

Obviously a newer gcc may fix this bug and introduce a new one.
But, no guarantee that -Os doesn't fix this bug and introduce a new one.

I guess bumping gcc to latest - check if this bug is fixed and if so get the new build tested in Milhouse/nightly builds to try to spot any regressions.

@escalade
Copy link
Contributor Author

I agree, except when a new compiler introduces more bugs :) Other than the FLAC issue mentioned 6.1 works fine though (with a few minor fixes). It does however drastically increase build time.

@escalade
Copy link
Contributor Author

@popcornmix Actually -Os is the default so it will surely not introduce new bugs. This change only reverts the Kodi package to default optimization when OPTIMIZATIONS=normal (-O2) is set in the project options. Eveery package is compiled with -Os by default.

@MilhouseVH
Copy link
Contributor

Yes, the default OPTIMIZATIONS for all projects is size:

neil@nm-linux:~/projects/LibreELEC.tv/projects$ grep -r OPTIMIZATIONS
WeTek_Core/options:    OPTIMIZATIONS="size"
imx6/options:    OPTIMIZATIONS="size"
RPi/options:    OPTIMIZATIONS="size"
Odroid_C2/options:    OPTIMIZATIONS="size"
Virtual/options:    OPTIMIZATIONS="size"
WeTek_Play/options:    OPTIMIZATIONS="size"
Generic/options:    OPTIMIZATIONS="size"
RPi2/options:    OPTIMIZATIONS="size"
WeTek_Hub/options:    OPTIMIZATIONS="size"

The two supported options are normal and size: https://github.com/LibreELEC/LibreELEC.tv/blob/master/config/optimize#L1-L9

if [ "$OPTIMIZATIONS" = normal ];then
  GCC_OPTIM="-O2"
  LD_OPTIM=""
fi

if [ "$OPTIMIZATIONS" = size ];then
  GCC_OPTIM="-Os -fexcess-precision=fast"
  LD_OPTIM="-Wl,--as-needed"
fi

so this change would have no effect whatsoever on current builds that use the default OPTIMIZATIONS (size), which may help explain why it's such a rarely seen issue (never seen it reported by users one of my builds, for instance). This is only an issue for custom builds that use the non-standard normal optimisation.

I'd personally rather see if we can work around this with a compiler bump, either gcc 6.1.0 or the future 6.2,

@escalade
Copy link
Contributor Author

Might be good to have in the 7.0 branch?

@stefansaraev
Copy link
Contributor

stefansaraev commented Aug 22, 2016

OPTIMIZATIONS=normal serves no real purpose and you should completely remove it.

EDIT: you should remove any OPTIMIZATIONS, and always use -Os

nijna edit: you should also remove -fexcess-precision=..

@escalade
Copy link
Contributor Author

The purpose is faster code.

@stefansaraev
Copy link
Contributor

I know what it is, @popcornmix but too many things may (and will) interfere right now :)

I dont think -O2 gives you much speed compared to -Os.

@escalade
Copy link
Contributor Author

I've compiled with -O2 for quite a while (gcc 5.3, 5.4 and 6.1) both for generic/rpi and so far this is the only issue I've come across. The improvement can be neglible but can also be much faster for certain code. Lots of distributions use -O2 as default (Arch for example), and there's plenty of benchmarks that proves it's worth.

@stefansaraev
Copy link
Contributor

as you wish ;)

@escalade
Copy link
Contributor Author

Don't confuse me for popcornmix btw, we just have the same cool avatar ;)

@stefansaraev
Copy link
Contributor

ouch ;)

@stefansaraev
Copy link
Contributor

@escalade

Yes, but with GCC6 there's an issue with reading FLAC tags (for generic as well). Was thinking to test 6.2 today.

can you share a sample ?

@lrusak lrusak mentioned this pull request Sep 5, 2016
@chewitt
Copy link
Member

chewitt commented Sep 10, 2016

Closing as superseded by #696

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants