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

login
register
mail settings
Submitter Mark Hatle
Date Nov. 21, 2013, 7:33 a.m.
Message ID <1385019198-24458-2-git-send-email-mark.hatle@windriver.com>
Download mbox | patch
Permalink /patch/62115/
State New
Headers show

Comments

Mark Hatle - Nov. 21, 2013, 7:33 a.m.
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(+)
Phil Blundell - Nov. 21, 2013, 2:25 p.m.
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.
Richard Purdie - Nov. 21, 2013, 2:35 p.m.
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
Mark Hatle - Nov. 21, 2013, 2:47 p.m.
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.
>
>
Mark Hatle - Nov. 21, 2013, 2:48 p.m.
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
>
Phil Blundell - Nov. 21, 2013, 3:57 p.m.
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.
Phil Blundell - Nov. 21, 2013, 5:43 p.m.
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.
Phil Blundell - Nov. 21, 2013, 9:22 p.m.
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.

Patch

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.