Hi Dave,
Today's linux-next build (sparc64 defconfig) failed like this:
kernel/time/tick-common.c: In function `tick_check_new_device':
kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
gcc is version 3.4.5 sparc64 cross compiler (powercp64 host).
The below patch fixes it.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
From: Stephen Rothwell <[email protected]>
Date: Tue, 29 Jul 2008 16:07:37 +1000
Subject: [PATCH] cpumask: statement expressions confuse some versions of gcc
when you take the address of the result. Noticed on a sparc64 compile
using a version 3.4.5 cross compiler.
kernel/time/tick-common.c: In function `tick_check_new_device':
kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
Signed-off-by: Stephen Rothwell <[email protected]>
---
include/linux/cpumask.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 96d0509..d3219d7 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -287,7 +287,7 @@ static inline const cpumask_t *get_cpu_mask(unsigned int cpu)
* gcc optimizes it out (it's a constant) and there's no huge stack
* variable created:
*/
-#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
+#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
#define CPU_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(NR_CPUS)
--
1.5.6.3
* Stephen Rothwell <[email protected]> wrote:
> Hi Dave,
>
> Today's linux-next build (sparc64 defconfig) failed like this:
>
> kernel/time/tick-common.c: In function `tick_check_new_device':
> kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
> kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
> kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
>
> gcc is version 3.4.5 sparc64 cross compiler (powercp64 host).
>
> The below patch fixes it.
>
> when you take the address of the result. Noticed on a sparc64 compile
> using a version 3.4.5 cross compiler.
>
> kernel/time/tick-common.c: In function `tick_check_new_device':
> kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
> kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
> kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
>
> Signed-off-by: Stephen Rothwell <[email protected]>
> ---
> include/linux/cpumask.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 96d0509..d3219d7 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -287,7 +287,7 @@ static inline const cpumask_t *get_cpu_mask(unsigned int cpu)
> * gcc optimizes it out (it's a constant) and there's no huge stack
> * variable created:
> */
> -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
hm, i'm wondering - is this a compiler bug?
Ingo
Hi Ingo,
On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
>
> > -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> > +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
>
> hm, i'm wondering - is this a compiler bug?
Or maybe a deficiency in such an old compiler (v3.4.5) but the fix makes
sense anyway, right?
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
Ingo Molnar wrote:
> * Stephen Rothwell <[email protected]> wrote:
>
>> Hi Dave,
>>
>> Today's linux-next build (sparc64 defconfig) failed like this:
>>
>> kernel/time/tick-common.c: In function `tick_check_new_device':
>> kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
>> kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
>> kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
>>
>> gcc is version 3.4.5 sparc64 cross compiler (powercp64 host).
>>
>> The below patch fixes it.
>>
>> when you take the address of the result. Noticed on a sparc64 compile
>> using a version 3.4.5 cross compiler.
>>
>> kernel/time/tick-common.c: In function `tick_check_new_device':
>> kernel/time/tick-common.c:210: error: invalid lvalue in unary `&'
>> kernel/time/tick-common.c:223: error: invalid lvalue in unary `&'
>> kernel/time/tick-common.c:255: error: invalid lvalue in unary `&'
>>
>> Signed-off-by: Stephen Rothwell <[email protected]>
>> ---
>> include/linux/cpumask.h | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index 96d0509..d3219d7 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -287,7 +287,7 @@ static inline const cpumask_t *get_cpu_mask(unsigned int cpu)
>> * gcc optimizes it out (it's a constant) and there's no huge stack
>> * variable created:
>> */
>> -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
>> +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
>
> hm, i'm wondering - is this a compiler bug?
>
> Ingo
Same problem on x86/gcc 3.4.6, but will pass on gcc 4.x
Regards,
Wenji
* Stephen Rothwell <[email protected]> wrote:
> Hi Ingo,
>
> On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
> >
> > > -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> > > +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
> >
> > hm, i'm wondering - is this a compiler bug?
>
> Or maybe a deficiency in such an old compiler (v3.4.5) but the fix
> makes sense anyway, right?
yeah, i was just wondering.
Ingo
>
> * Stephen Rothwell <[email protected]> wrote:
>
> > Hi Ingo,
> >
> > On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
> > >
> > > > -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> > > > +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
> > >
> > > hm, i'm wondering - is this a compiler bug?
> >
> > Or maybe a deficiency in such an old compiler (v3.4.5) but the fix
> > makes sense anyway, right?
>
> yeah, i was just wondering.
in linux/README
COMPILING the kernel:
- Make sure you have at least gcc 3.2 available.
For more information, refer to Documentation/Changes.
So, if 3.4.5 is old, Should we change readme?
* KOSAKI Motohiro <[email protected]> wrote:
> >
> > * Stephen Rothwell <[email protected]> wrote:
> >
> > > Hi Ingo,
> > >
> > > On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
> > > >
> > > > > -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> > > > > +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
> > > >
> > > > hm, i'm wondering - is this a compiler bug?
> > >
> > > Or maybe a deficiency in such an old compiler (v3.4.5) but the fix
> > > makes sense anyway, right?
> >
> > yeah, i was just wondering.
>
> in linux/README
>
> COMPILING the kernel:
>
> - Make sure you have at least gcc 3.2 available.
> For more information, refer to Documentation/Changes.
>
> So, if 3.4.5 is old, Should we change readme?
the fix is simple enough.
but the question is, wont it generate huge artificial stackframes with
CONFIG_MAXSMP and NR_CPUS=4096? Maybe it is unable to figure out and
simplify the arithmetics there - or something like that.
Ingo
Ingo Molnar wrote:
> * KOSAKI Motohiro <[email protected]> wrote:
>
>>> * Stephen Rothwell <[email protected]> wrote:
>>>
>>>> Hi Ingo,
>>>>
>>>> On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
>>>>>> -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
>>>>>> +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
>>>>> hm, i'm wondering - is this a compiler bug?
>>>> Or maybe a deficiency in such an old compiler (v3.4.5) but the fix
>>>> makes sense anyway, right?
>>> yeah, i was just wondering.
>> in linux/README
>>
>> COMPILING the kernel:
>>
>> - Make sure you have at least gcc 3.2 available.
>> For more information, refer to Documentation/Changes.
>>
>> So, if 3.4.5 is old, Should we change readme?
>
> the fix is simple enough.
>
> but the question is, wont it generate huge artificial stackframes with
> CONFIG_MAXSMP and NR_CPUS=4096? Maybe it is unable to figure out and
> simplify the arithmetics there - or something like that.
>
> Ingo
I've looked at stack frames quite extensively and usually they are
not generated unless you explicitly use a named cpumask variable,
pass a cpumask by value, expect a cpumask function return, create
an initializer that contains a cpumask field, and (probably a couple
more I missed).
Almost all others are done efficiently via pointers or simple
struct copies:
cpus_xxx(*cpumask_of_cpu(), ...
struct->cpumask_var = *cpumask_of_cpu()
global_cpumask_var = *cpumask_of_cpu()
etc.
Thanks,
Mike
Mike Travis wrote:
> Ingo Molnar wrote:
>> * KOSAKI Motohiro <[email protected]> wrote:
>>
>>>> * Stephen Rothwell <[email protected]> wrote:
>>>>
>>>>> Hi Ingo,
>>>>>
>>>>> On Tue, 29 Jul 2008 10:00:55 +0200 Ingo Molnar <[email protected]> wrote:
>>>>>>> -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
>>>>>>> +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
>>>>>> hm, i'm wondering - is this a compiler bug?
>>>>> Or maybe a deficiency in such an old compiler (v3.4.5) but the fix
>>>>> makes sense anyway, right?
>>>> yeah, i was just wondering.
>>> in linux/README
>>>
>>> COMPILING the kernel:
>>>
>>> - Make sure you have at least gcc 3.2 available.
>>> For more information, refer to Documentation/Changes.
>>>
>>> So, if 3.4.5 is old, Should we change readme?
>> the fix is simple enough.
>>
>> but the question is, wont it generate huge artificial stackframes with
>> CONFIG_MAXSMP and NR_CPUS=4096? Maybe it is unable to figure out and
>> simplify the arithmetics there - or something like that.
>>
>> Ingo
>
> I've looked at stack frames quite extensively and usually they are
> not generated unless you explicitly use a named cpumask variable,
> pass a cpumask by value, expect a cpumask function return, create
> an initializer that contains a cpumask field, and (probably a couple
> more I missed).
>
> Almost all others are done efficiently via pointers or simple
> struct copies:
>
> cpus_xxx(*cpumask_of_cpu(), ...
> struct->cpumask_var = *cpumask_of_cpu()
> global_cpumask_var = *cpumask_of_cpu()
> etc.
>
> Thanks,
> Mike
Geez, I edited the above after I first used *cpumask_var and didn't
get the semantics right!
cpus_xxx(cpumask_of_cpu(), ...
struct->cpumask_var = cpumask_of_cpu()
global_cpumask_var = cpumask_of_cpu()
On Tue, 29 Jul 2008, Ingo Molnar wrote:
>
> > @@ -287,7 +287,7 @@ static inline const cpumask_t *get_cpu_mask(unsigned int cpu)
> > * gcc optimizes it out (it's a constant) and there's no huge stack
> > * variable created:
> > */
> > -#define cpumask_of_cpu(cpu) ({ *get_cpu_mask(cpu); })
> > +#define cpumask_of_cpu(cpu) (*get_cpu_mask(cpu))
>
> hm, i'm wondering - is this a compiler bug?
Not necessarily. The code is fragile.
Doing a statement expression basically creates a new temporary variable
with pretty much undefined scope. Taking the address of it *may* be
optimized away to the original cpu_mask pointer, but it's not really a
well-defined operation: there really _is_ a local temporary variable, and
if you were to change things through the address-of thing, gcc would be
buggy if it had done the optimization.
So the "address-of statement expression" really is a dubious construct.
That said, the change that Stephen introduces is _not_ a no-op. It very
fundamentally changes the code - exactly because now there is no temporary
value created any more: it's a real lvalue, and now anybody who does
&cpumask_of_cpu(cpu) will fundamentally get the original pointer back,
except it has lost the "const".
And that's actually dangerous - exactly because it now loses the "const"
thing, there may be people who end up modifying the result without
getting any compile-time warnings.
I would _seriously_ suggest that both the original code and Stephen's
modified version is really bad. And you should have taken my interface
as-is - one that returns a "const cpumask_t *", and nothing else.
Anything else is simply fundamentally broken.
Linus
On Tue, 29 Jul 2008, Ingo Molnar wrote:
>
> the fix is simple enough.
>
> but the question is, wont it generate huge artificial stackframes with
> CONFIG_MAXSMP and NR_CPUS=4096?
Quite the reverse.
The "address-of statement expression" is the one that is more likely to
generate artificial stack-frames because of a temporary variable (of
course, I wouldn't count on it, since statement expressions are gcc
extensions, and as such the gcc people could make up any semantics they
want to them, including just defining that a statement expression with
an lvalue value is the same lvalue rather than any temporary).
In contrast, "address-of lvalue" is _guaranteed_ to not do anything stupid
like that, and gives just the address-of.
Oh, and I was wrong about the &*x losing the 'const'. It doesn't. So I
think Stephen's patch is fine after all - if somebody tries to modify the
end result through the pointer, it will give a big compiler warning.
Linus
* Linus Torvalds <[email protected]> wrote:
> In contrast, "address-of lvalue" is _guaranteed_ to not do anything
> stupid like that, and gives just the address-of.
>
> Oh, and I was wrong about the &*x losing the 'const'. It doesn't. So I
> think Stephen's patch is fine after all - if somebody tries to modify
> the end result through the pointer, it will give a big compiler
> warning.
yeah, both variants do that, i've checked it earlier today - i tried to
find a way to get something more drastic than a compiler warning. (but
failed)
Ingo
On Tue, 29 Jul 2008, Linus Torvalds wrote:
>
> [...] since statement expressions are gcc
> extensions, and as such the gcc people could make up any semantics they
> want to them, including just defining that a statement expression with
> an lvalue value is the same lvalue rather than any temporary).
In fact, that does seem what gcc-4.x does. The way to tell is to do
const int *x;
({ *x }) = 1;
and it's (a) legal (assignments to non-lvalues wouldn't work) and (b)
gives a nice warning about assignment to read-only location, which in turn
implies that the compiler properly just peeled off the de-reference even
though it was inside the statement expression.
IOW, at least in gcc-4.3 (and apparently in earlier gcc-4 versions, but
not in gcc-3.4.5), a statement expression with an lvalue return value _is_
actually an lvalue.
But that also means that there is no difference what-so-ever between (x)
and ({ x; }) in gcc-4. And in gcc-3 there is, because apparently in gcc-3
a statement expression is never an lvalue (which is actually the sane
thing, imho).
Linus