[master][dora][PATCH 0/2] Few perf fixes

[master][dora][PATCH 0/2] Few perf fixes

[Mark Hatle]

Konrad Scherer (1):
  perf: Disable -Werror flag

Randy MacLeod (1):
  perf: disallow debug optimization.

 meta/recipes-kernel/perf/perf.bb | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
1.8.1.2.545.g2f19ada

[master][dora][PATCH 1/2] perf: disallow debug optimization.

[Mark Hatle]

From: Randy MacLeod <Randy.MacLeod@windriver.com>

perf fails to compile if someone tries to compile an entire image as -O0
so in this case, force to use -O2 and give a note about it.

Signed-off-by: Randy MacLeod <Randy.MacLeod@windriver.com>
---
 meta/recipes-kernel/perf/perf.bb | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/meta/recipes-kernel/perf/perf.bb b/meta/recipes-kernel/perf/perf.bb
index 269069f..138595d 100644
--- a/meta/recipes-kernel/perf/perf.bb
+++ b/meta/recipes-kernel/perf/perf.bb
@@ -60,6 +60,17 @@ B = "${WORKDIR}/${BPN}-${PV}"
 SCRIPTING_DEFINES = "${@perf_feature_enabled('perf-scripting', '', 'NO_LIBPERL=1 NO_LIBPYTHON=1',d)}"
 TUI_DEFINES = "${@perf_feature_enabled('perf-tui', '', 'NO_NEWT=1',d)}"
 
+# perf can't be built without optimization, if someone tries to compile an
+# entire image as -O0, we override it with -O2 here and give a note about it.
+def get_optimization(d):
+    selected_optimization = d.getVar("SELECTED_OPTIMIZATION", True)
+    if base_contains("SELECTED_OPTIMIZATION", "-O0", "x", "", d) == "x":
+        bb.note("eglibc can't be built with -O0, -O2 will be used instead.")
+        return selected_optimization.replace("-O0", "-O2")
+    return selected_optimization
+
+SELECTED_OPTIMIZATION := "${@get_optimization(d)}"
+
 # The LDFLAGS is required or some old kernels fails due missing
 # symbols and this is preferred than requiring patches to every old
 # supported kernel.
-- 
1.8.1.2.545.g2f19ada

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Phil Blundell]

On Thu, 2013-11-21 at 01:33 -0600, Mark Hatle wrote:
> +def get_optimization(d):
> +    selected_optimization = d.getVar("SELECTED_OPTIMIZATION", True)
> +    if base_contains("SELECTED_OPTIMIZATION", "-O0", "x", "", d) == "x":
> +        bb.note("eglibc can't be built with -O0, -O2 will be used instead.")

Although the text of that warning is correct, users might find the
reference to eglibc slightly confusing if it's perf that they're trying
to build.

Also, as I mentioned in a different thread not all that long ago when
someone submitted a similar patch for gcc-runtime, the proliferation of
parse-time python functions to bash SELECTED_OPTIMIZATION around doesn't
seem like all that good a thing: this will cause extra overhead for
everyone, even those who are not using -O0 and have no interest in perf.

And, finally, it remains slightly unclear to me that this is really a
problem that the metadata needs to be solving.  I haven't seen any
particularly convincing explanation of why this can't or shouldn't just
be fixed in the distro configuration.

p.

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Richard Purdie]

On Thu, 2013-11-21 at 14:25 +0000, Phil Blundell wrote:
> On Thu, 2013-11-21 at 01:33 -0600, Mark Hatle wrote:
> > +def get_optimization(d):
> > +    selected_optimization = d.getVar("SELECTED_OPTIMIZATION", True)
> > +    if base_contains("SELECTED_OPTIMIZATION", "-O0", "x", "", d) == "x":
> > +        bb.note("eglibc can't be built with -O0, -O2 will be used instead.")
> 
> Although the text of that warning is correct, users might find the
> reference to eglibc slightly confusing if it's perf that they're trying
> to build.
> 
> Also, as I mentioned in a different thread not all that long ago when
> someone submitted a similar patch for gcc-runtime, the proliferation of
> parse-time python functions to bash SELECTED_OPTIMIZATION around doesn't
> seem like all that good a thing: this will cause extra overhead for
> everyone, even those who are not using -O0 and have no interest in perf.
> 
> And, finally, it remains slightly unclear to me that this is really a
> problem that the metadata needs to be solving.  I haven't seen any
> particularly convincing explanation of why this can't or shouldn't just
> be fixed in the distro configuration.

I have to admit at this point, this may look better as an include file
along the lines of:

SELECTED_OPTIMIZATION = "-O0"
SELECTED_OPTIMIZATION_pn-eglibc = "-O2"
SELECTED_OPTIMIZATION_pn-perf = "-O2"

since clutter the recipes with anonymous python fragments isn't
particular desirable.

Cheers,

Richard

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Mark Hatle]

On 11/21/13, 8:35 AM, Richard Purdie wrote:
> On Thu, 2013-11-21 at 14:25 +0000, Phil Blundell wrote:
>> On Thu, 2013-11-21 at 01:33 -0600, Mark Hatle wrote:
>>> +def get_optimization(d):
>>> +    selected_optimization = d.getVar("SELECTED_OPTIMIZATION", True)
>>> +    if base_contains("SELECTED_OPTIMIZATION", "-O0", "x", "", d) == "x":
>>> +        bb.note("eglibc can't be built with -O0, -O2 will be used instead.")
>>
>> Although the text of that warning is correct, users might find the
>> reference to eglibc slightly confusing if it's perf that they're trying
>> to build.
>>
>> Also, as I mentioned in a different thread not all that long ago when
>> someone submitted a similar patch for gcc-runtime, the proliferation of
>> parse-time python functions to bash SELECTED_OPTIMIZATION around doesn't
>> seem like all that good a thing: this will cause extra overhead for
>> everyone, even those who are not using -O0 and have no interest in perf.
>>
>> And, finally, it remains slightly unclear to me that this is really a
>> problem that the metadata needs to be solving.  I haven't seen any
>> particularly convincing explanation of why this can't or shouldn't just
>> be fixed in the distro configuration.
>
> I have to admit at this point, this may look better as an include file
> along the lines of:
>
> SELECTED_OPTIMIZATION = "-O0"
> SELECTED_OPTIMIZATION_pn-eglibc = "-O2"
> SELECTED_OPTIMIZATION_pn-perf = "-O2"
>
> since clutter the recipes with anonymous python fragments isn't
> particular desirable.

Thats part of the problem.  We only need to set -O2, when someone sets -O0.  But 
if they set -O1 or -Os (or any other -O...) it appears to work properly...

So the python fragment is used to establish a known functional set for that item.

--Mark

> Cheers,
>
> Richard
>

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Phil Blundell]

On Thu, 2013-11-21 at 14:35 +0000, Richard Purdie wrote:
> I have to admit at this point, this may look better as an include file
> along the lines of:
> 
> SELECTED_OPTIMIZATION = "-O0"
> SELECTED_OPTIMIZATION_pn-eglibc = "-O2"
> SELECTED_OPTIMIZATION_pn-perf = "-O2"
> 
> since clutter the recipes with anonymous python fragments isn't
> particular desirable.

Right, exactly.  This is what I already do in my distro configuration to
deal with a similar situation involving -fasynchronous-unwind-tables and
it works fine. 

p.

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Mark Hatle]

On 11/21/13, 8:25 AM, Phil Blundell wrote:
> On Thu, 2013-11-21 at 01:33 -0600, Mark Hatle wrote:
>> +def get_optimization(d):
>> +    selected_optimization = d.getVar("SELECTED_OPTIMIZATION", True)
>> +    if base_contains("SELECTED_OPTIMIZATION", "-O0", "x", "", d) == "x":
>> +        bb.note("eglibc can't be built with -O0, -O2 will be used instead.")
>
> Although the text of that warning is correct, users might find the
> reference to eglibc slightly confusing if it's perf that they're trying
> to build.

I'll get that fixed.

> Also, as I mentioned in a different thread not all that long ago when
> someone submitted a similar patch for gcc-runtime, the proliferation of
> parse-time python functions to bash SELECTED_OPTIMIZATION around doesn't
> seem like all that good a thing: this will cause extra overhead for
> everyone, even those who are not using -O0 and have no interest in perf.

We have users who desire to build their system at different levels of 
optimizations for debug, size, profiling, etc.  So they do change the default 
optimization levels from -O2 to -O0, etc.  The python fragement is used to only 
adjust -O0, as -O1 (or -Os) work correctly.

--Mark

> And, finally, it remains slightly unclear to me that this is really a
> problem that the metadata needs to be solving.  I haven't seen any
> particularly convincing explanation of why this can't or shouldn't just
> be fixed in the distro configuration.
>
> p.
>
>

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Phil Blundell]

On Thu, 2013-11-21 at 08:47 -0600, Mark Hatle wrote:
> We have users who desire to build their system at different levels of 
> optimizations for debug, size, profiling, etc.  So they do change the default 
> optimization levels from -O2 to -O0, etc.  The python fragement is used to only 
> adjust -O0, as -O1 (or -Os) work correctly.

Sure, I understand what the python is doing.  The things I'm not quite
so clear about are:

a) If the user asks to build with -O0, is it appropriate for the
metadata to second-guess this and quietly switch to using -O2 instead
when it thinks it knows best?  

Personally I am inclined to say that attempting to use -O0 with packages
that don't support it should just produce an error and the user should
fix their configuration to not do that.  And, if we're going to enable
optimisation that the user hasn't asked for, shouldn't it be the minimum
level consistent with getting the package to build rather than the full
-O2 set?

b) If the answer to (a) is that the metadata should indeed be doing
this, can it be made to do so in a way that doesn't involve running
extra python fragments for all users every time the recipe is parsed?

c) If the answer to (b) is no, is the feature really so important that
it's worth adding extra overhead to the parse for all users in order to
benefit the (presumably tiny) minority who might actually be trying to
build perf at -O0?

p.

Re: [master][dora][PATCH 1/2] perf: disallow debug optimization.

[Phil Blundell]

On Thu, 2013-11-21 at 15:57 +0000, Phil Blundell wrote:
> On Thu, 2013-11-21 at 08:47 -0600, Mark Hatle wrote:
> > We have users who desire to build their system at different levels of 
> > optimizations for debug, size, profiling, etc.  So they do change the default 
> > optimization levels from -O2 to -O0, etc.  The python fragement is used to only 
> > adjust -O0, as -O1 (or -Os) work correctly.
> 
> Sure, I understand what the python is doing.  The things I'm not quite
> so clear about are:
> 
> a) If the user asks to build with -O0, is it appropriate for the
> metadata to second-guess this and quietly switch to using -O2 instead
> when it thinks it knows best?  

I suppose the other question is: why exactly does perf fail to build at
-O0, and can we just patch it so that it works rather than forcing
optimisation on?  Even if it can't be fixed, it would be good for the
commit message associated with any workaround to explain what the
problem is.

p.

[master][dora][PATCH 2/2] perf: Disable -Werror flag

[Mark Hatle]

From: Konrad Scherer <Konrad.Scherer@windriver.com>

If the sed command does not run before make is invoked, the compile
fails. Defining the environment variable is the proper way to disable
warnings as errors build option and eliminates the race condition.

Signed-off-by: Konrad Scherer <Konrad.Scherer@windriver.com>
Signed-off-by: Randy MacLeod <Randy.MacLeod@windriver.com>
---
 meta/recipes-kernel/perf/perf.bb | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/meta/recipes-kernel/perf/perf.bb b/meta/recipes-kernel/perf/perf.bb
index 138595d..1b889aa 100644
--- a/meta/recipes-kernel/perf/perf.bb
+++ b/meta/recipes-kernel/perf/perf.bb
@@ -40,6 +40,7 @@ export STAGING_INCDIR
 export STAGING_LIBDIR
 export BUILD_SYS
 export HOST_SYS
+export WERROR = "0"
 
 do_populate_lic[depends] += "virtual/kernel:do_populate_sysroot"
 
@@ -118,10 +119,6 @@ do_install() {
 	fi
 }
 
-do_configure_prepend () {
-    sed -i 's,-Werror ,,' ${S}/tools/perf/Makefile
-}
-
 python do_package_prepend() {
     bb.data.setVar('PKGV', get_kernelversion('${S}').split("-")[0], d)
 }
-- 
1.8.1.2.545.g2f19ada

Re: [master][dora][PATCH 2/2] perf: Disable -Werror flag

[Konrad Scherer]

On 13-11-21 02:33 AM, Mark Hatle wrote:
> From: Konrad Scherer <Konrad.Scherer@windriver.com>
>
> If the sed command does not run before make is invoked, the compile
> fails. Defining the environment variable is the proper way to disable
> warnings as errors build option and eliminates the race condition.
>
> Signed-off-by: Konrad Scherer <Konrad.Scherer@windriver.com>
> Signed-off-by: Randy MacLeod <Randy.MacLeod@windriver.com>
> ---
>   meta/recipes-kernel/perf/perf.bb | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/meta/recipes-kernel/perf/perf.bb b/meta/recipes-kernel/perf/perf.bb
> index 138595d..1b889aa 100644
> --- a/meta/recipes-kernel/perf/perf.bb
> +++ b/meta/recipes-kernel/perf/perf.bb
> @@ -40,6 +40,7 @@ export STAGING_INCDIR
>   export STAGING_LIBDIR
>   export BUILD_SYS
>   export HOST_SYS
> +export WERROR = "0"
>
>   do_populate_lic[depends] += "virtual/kernel:do_populate_sysroot"
>
> @@ -118,10 +119,6 @@ do_install() {
>   	fi
>   }
>
> -do_configure_prepend () {
> -    sed -i 's,-Werror ,,' ${S}/tools/perf/Makefile
> -}
> -

This is will not work for kernels < 3.1. I did submit another patch 
which works for all kernel versions.

-- 
Konrad Scherer, MTS, Linux Products Group, Wind River