Hello all,
While a 'make bzImage', I received the following compile error:
Regards,
Frank
smpboot.c:1008: parse error before `0'
smpboot.c: In function `smp_boot_cpus':
smpboot.c:1023: invalid lvalue in assignment
make[1]: *** [smpboot.o] Error 1
make[1]: Leaving directory `/usr/src/linux/arch/i386/kernel'
make: *** [_dir_arch/i386/kernel] Error 2
On Mon, Apr 15, 2002 at 10:19:24PM -0400, Frank Davis wrote:
> Hello all,
> While a 'make bzImage', I received the following compile error:
> Regards,
> Frank
>
> smpboot.c:1008: parse error before `0'
> smpboot.c: In function `smp_boot_cpus':
> smpboot.c:1023: invalid lvalue in assignment
> make[1]: *** [smpboot.o] Error 1
> make[1]: Leaving directory `/usr/src/linux/arch/i386/kernel'
> make: *** [_dir_arch/i386/kernel] Error 2
There's an optimization in io.h for !CONFIG_MULTIQUAD which doesn't seem to have been
carried through all the way...
The following patch seems logical, but I'm no expert. In particular, I'm not sure
if the ioremapping bit ever needs to be run when !CONFIG_MULTIQUAD...
--Adam
--- linux-2.5.8-dj1-virgin/arch/i386/kernel/smpboot.c Tue Apr 16 16:21:24 2002
+++ linux-2.5.8-dj1+smpboot-fix/arch/i386/kernel/smpboot.c Tue Apr 16 21:12:13 2002
@@ -1004,8 +1004,11 @@
extern int prof_counter[NR_CPUS];
static int boot_cpu_logical_apicid;
+
/* Where the IO area was mapped on multiquad, always 0 otherwise */
+#ifdef CONFIG_MULTIQUAD
void *xquad_portio = NULL;
+#endif
int cpu_sibling_map[NR_CPUS] __cacheline_aligned;
@@ -1013,6 +1016,7 @@
{
int apicid, cpu, bit;
+#ifdef CONFIG_MULTIQUAD
if (clustered_apic_mode && (numnodes > 1)) {
printk("Remapping cross-quad port I/O for %d quads\n",
numnodes);
@@ -1022,6 +1026,7 @@
xquad_portio = ioremap (XQUAD_PORTIO_BASE,
numnodes * XQUAD_PORTIO_LEN);
}
+#endif
#ifdef CONFIG_MTRR
/* Must be done before other processors booted */
xquad_portio is indeed only for CONFIG_MULTIQUAD. However, you
shouldn't need the #ifdef's in the code to make this work -
clustered_apic_mode isn't a variable at all, it's a magic
trick that's actually 1 or 0 depending on CONFIG_MULTIQUAD.
Look at 2.5.8 virgin, it has the same code.
M.
--On Tuesday, April 16, 2002 10:47 PM -0400 Adam Kropelin
<[email protected]> wrote:
> On Mon, Apr 15, 2002 at 10:19:24PM -0400, Frank Davis wrote:
>> Hello all,
>> While a 'make bzImage', I received the following compile error:
>> Regards,
>> Frank
>>
>> smpboot.c:1008: parse error before `0'
>> smpboot.c: In function `smp_boot_cpus':
>> smpboot.c:1023: invalid lvalue in assignment
>> make[1]: *** [smpboot.o] Error 1
>> make[1]: Leaving directory `/usr/src/linux/arch/i386/kernel'
>> make: *** [_dir_arch/i386/kernel] Error 2
>
> There's an optimization in io.h for !CONFIG_MULTIQUAD which doesn't seem
> to have been carried through all the way...
>
> The following patch seems logical, but I'm no expert. In particular, I'm
> not sure if the ioremapping bit ever needs to be run when
> !CONFIG_MULTIQUAD...
>
> --Adam
>
> --- linux-2.5.8-dj1-virgin/arch/i386/kernel/smpboot.c Tue Apr 16 16:21:24
> 2002 +++ linux-2.5.8-dj1+smpboot-fix/arch/i386/kernel/smpboot.c Tue Apr
> 16 21:12:13 2002 @@ -1004,8 +1004,11 @@
> extern int prof_counter[NR_CPUS];
>
> static int boot_cpu_logical_apicid;
> +
> /* Where the IO area was mapped on multiquad, always 0 otherwise */
> +#ifdef CONFIG_MULTIQUAD
> void *xquad_portio = NULL;
> +#endif
>
> int cpu_sibling_map[NR_CPUS] __cacheline_aligned;
>
> @@ -1013,6 +1016,7 @@
> {
> int apicid, cpu, bit;
>
> +#ifdef CONFIG_MULTIQUAD
> if (clustered_apic_mode && (numnodes > 1)) {
> printk("Remapping cross-quad port I/O for %d quads\n",
> numnodes);
> @@ -1022,6 +1026,7 @@
> xquad_portio = ioremap (XQUAD_PORTIO_BASE,
> numnodes * XQUAD_PORTIO_LEN);
> }
> +#endif
>
> #ifdef CONFIG_MTRR
> /* Must be done before other processors booted */
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Tue, Apr 16, 2002 at 09:59:08PM -0700, Martin J. Bligh wrote:
> xquad_portio is indeed only for CONFIG_MULTIQUAD. However, you
> shouldn't need the #ifdef's in the code to make this work -
> clustered_apic_mode isn't a variable at all, it's a magic
> trick that's actually 1 or 0 depending on CONFIG_MULTIQUAD.
>
> Look at 2.5.8 virgin, it has the same code.
Not quite.
As I said, -dj has an optimization in asm-i386/io.o:
> #ifdef CONFIG_MULTIQUAD
> extern void *xquad_portio; /* Where the IO area was mapped */
> #else
> #define xquad_portio (0)
> #endif
So the preprocessed smpboot.c contains gems like:
> void *(0) = ((void *)0);
...and...
> (0) = ioremap (0xfe400000,
> numnodes * 0x80000);
Even though clustered_apic_mode is 0, the compiler still complains
about the second one and the first one doesn't depend on
clustered_apic_mode at all.
I don't like spreading around more #ifdef's, but the spirit of the
changes seemed to be to get rid of the declaration of xquad_portio
when !CONFIG_MULTIQUAD. Suggestions for improvement welcome.
--Adam
On Wed, Apr 17, 2002 at 08:30:44AM -0400, Adam Kropelin wrote:
> As I said, -dj has an optimization in asm-i386/io.o:
^
io.h, of course...
--Adam
cc'ed Brian - this is your io.h cleanup patch
> As I said, -dj has an optimization in asm-i386/io.o:
>
>> # ifdef CONFIG_MULTIQUAD
>> extern void *xquad_portio; /* Where the IO area was mapped */
>> # else
>> # define xquad_portio (0)
>> # endif
Ah, OK, I missed that - makes more sense now ;-).
> Even though clustered_apic_mode is 0, the compiler still complains
> about the second one and the first one doesn't depend on
> clustered_apic_mode at all.
Hmmm ... not sure why the compiler complains about the second one,
that's very strange ;-)
> I don't like spreading around more #ifdef's, but the spirit of the
> changes seemed to be to get rid of the declaration of xquad_portio
> when !CONFIG_MULTIQUAD. Suggestions for improvement welcome.
The cleanups we gained in io.h by Brian's patch more than compensate
for this, but it's still a shame to have to do.
I wonder if we can play the same trick we've played before ....
haven't tested the appended, but maybe it, or something like it
will work without the ifdef's?
M.
--- linux-2.5.8-dj1/include/asm-i386/io.h Wed Apr 17 05:06:20 2002
+++ linux-2.5.8-dj1/include/asm-i386/io.h.new Wed Apr 17 15:07:11 2002
@@ -330,7 +330,8 @@
}
#ifdef CONFIG_MULTIQUAD
-extern void *xquad_portio; /* Where the IO area was mapped */
+#define xquad_portio real_xquad_portio
+extern void *real_xquad_portio; /* Where the IO area was mapped */
#else
#define xquad_portio (0)
#endif
--- linux-2.5.8-dj1/arch/i386/kernel/smpboot.c Sun Apr 14 12:18:52 2002
+++ linux-2.5.8-dj1/arch/i386/kernel/smpboot.c.new Wed Apr 17 15:08:25 2002
@@ -1005,7 +1005,7 @@
static int boot_cpu_logical_apicid;
/* Where the IO area was mapped on multiquad, always 0 otherwise */
-void *xquad_portio = NULL;
+void *real_xquad_portio = NULL;
int cpu_sibling_map[NR_CPUS] __cacheline_aligned;
On Wed, Apr 17, 2002 at 08:28:19AM -0700, Martin J. Bligh wrote:
> Adam Kropelin wrote:
> > Even though clustered_apic_mode is 0, the compiler still complains
> > about the second one and the first one doesn't depend on
> > clustered_apic_mode at all.
>
> Hmmm ... not sure why the compiler complains about the second one,
> that's very strange ;-)
I agree. The cpp ouput clealy shows
if ((0) && (numnodes > 1)) {
so I'm not sure why there's a problem.
> I wonder if we can play the same trick we've played before ....
> haven't tested the appended, but maybe it, or something like it
> will work without the ifdef's?
IMHO, this sort of trickery in the name of improving readability
is misguided. To me, anyway, the #ifdef's are much easer to read than
magic name-changing macros buried in a header somewhere.
--Adam
Adam Kropelin wrote:
> On Wed, Apr 17, 2002 at 08:28:19AM -0700, Martin J. Bligh wrote:
>
>>Adam Kropelin wrote:
>>
>>>Even though clustered_apic_mode is 0, the compiler still complains
>>>about the second one and the first one doesn't depend on
>>>clustered_apic_mode at all.
>>>
>>Hmmm ... not sure why the compiler complains about the second one,
>>that's very strange ;-)
>>
>
> I agree. The cpp ouput clealy shows
>
> if ((0) && (numnodes > 1)) {
>
> so I'm not sure why there's a problem.
Is the thing generating the "(0)" a macro? If so, then the rules
of C and the "&&" operator say that "if the first is false, the
second needn't even be evaluated". Could that be what's causing
the warning?
----------------------------------------------------------------------
- Rick Stevens, SSE, VitalStream, Inc. [email protected] -
- 949-743-2010 (Voice) http://www.vitalstream.com -
- -
- We have enough youth, how about a fountain of SMART? -
----------------------------------------------------------------------
>> I wonder if we can play the same trick we've played before ....
>> haven't tested the appended, but maybe it, or something like it
>> will work without the ifdef's?
>
> IMHO, this sort of trickery in the name of improving readability
> is misguided. To me, anyway, the #ifdef's are much easer to read than
> magic name-changing macros buried in a header somewhere.
Well, except that you can take that abstraction inside your head, and
not worry about it when reading the mainline code. I don't really care
one way or the other, at least io.h is readable now ;-)
M.
>>>> Even though clustered_apic_mode is 0, the compiler still complains
>>>> about the second one and the first one doesn't depend on
>>>> clustered_apic_mode at all.
>>>>
>>> Hmmm ... not sure why the compiler complains about the second one,
>>> that's very strange ;-)
>>>
>>
>> I agree. The cpp ouput clealy shows
>>
>> if ((0) && (numnodes > 1)) {
>>
>> so I'm not sure why there's a problem.
>
> Is the thing generating the "(0)" a macro? If so, then the rules
#define clustered_apic_mode (0)
> of C and the "&&" operator say that "if the first is false, the
> second needn't even be evaluated".
That's what I would have thought.
But I don't think it's the second part that causes the warning,
it's the thing *inside* the if clause.
> Could that be what's causing the warning?
To my mind, that's why we should *not* be getting a warning ?
M.
On Wed, Apr 17, 2002 at 01:53:55PM -0700, Martin J. Bligh wrote:
> > of C and the "&&" operator say that "if the first is false, the
> > second needn't even be evaluated".
>
> That's what I would have thought.
> But I don't think it's the second part that causes the warning,
> it's the thing *inside* the if clause.
Exactly.
> > Could that be what's causing the warning?
>
> To my mind, that's why we should *not* be getting a warning ?
Indeed. The optimization step that (presumably) removes the body
of the if() must happen after the body has been fully evaluated.
Makes sense, I guess, now that I think about it...
--Adam
Adam Kropelin wrote:
> On Wed, Apr 17, 2002 at 01:53:55PM -0700, Martin J. Bligh wrote:
>
>>>of C and the "&&" operator say that "if the first is false, the
>>>second needn't even be evaluated".
>>>
>>That's what I would have thought.
>>But I don't think it's the second part that causes the warning,
>>it's the thing *inside* the if clause.
>>
>
> Exactly.
>
>
>>>Could that be what's causing the warning?
>>>
>>To my mind, that's why we should *not* be getting a warning ?
>>
>
> Indeed. The optimization step that (presumably) removes the body
> of the if() must happen after the body has been fully evaluated.
> Makes sense, I guess, now that I think about it...
Right. If the first condition of a logical AND statement is false,
the remainder need not be evaluated at all. Hence, the entire if
statement can (and perhaps should) be eliminated by the compiler,
since the condition is false.
I didn't see what the actual message from the compiler was, but it
was probably just a warning.
----------------------------------------------------------------------
- Rick Stevens, SSE, VitalStream, Inc. [email protected] -
- 949-743-2010 (Voice) http://www.vitalstream.com -
- -
- Never put off 'til tommorrow what you can forget altogether! -
----------------------------------------------------------------------
Rick Stevens wrote:
> Adam Kropelin wrote:
>> Indeed. The optimization step that (presumably) removes the body
>> of the if() must happen after the body has been fully evaluated.
>> Makes sense, I guess, now that I think about it...
>
> Right. If the first condition of a logical AND statement is false,
> the remainder need not be evaluated at all. Hence, the entire if
> statement can (and perhaps should) be eliminated by the compiler,
> since the condition is false.
>
> I didn't see what the actual message from the compiler was, but it
> was probably just a warning.
Thanks for the help, but it was an error, not a warning.
It is caused by invalid code inside the body of the if(). The question
is not what is causing the error but why the compiler is evaluating the
body at all, given that the conditional is always false.
Apparently the body is evaluated prior to the optimizer getting a
chance to nullify the whole construct.
--Adam
On 2002.04.17 Adam Kropelin wrote:
>Rick Stevens wrote:
>> Adam Kropelin wrote:
>>> Indeed. The optimization step that (presumably) removes the body
>>> of the if() must happen after the body has been fully evaluated.
>>> Makes sense, I guess, now that I think about it...
>>
>> Right. If the first condition of a logical AND statement is false,
>> the remainder need not be evaluated at all. Hence, the entire if
>> statement can (and perhaps should) be eliminated by the compiler,
>> since the condition is false.
>>
>> I didn't see what the actual message from the compiler was, but it
>> was probably just a warning.
>
>Thanks for the help, but it was an error, not a warning.
>
>It is caused by invalid code inside the body of the if(). The question
>is not what is causing the error but why the compiler is evaluating the
>body at all, given that the conditional is always false.
>
>Apparently the body is evaluated prior to the optimizer getting a
>chance to nullify the whole construct.
>
AFAIK, you first parse C to build a tree of commands, expresions, etc,
and then you do the rest on the tree (optimizations at source-code level,
like CSE, ie, detection of equal subtrees, or for example
killig and if tree if it is always false).
Hope this helps.
--
J.A. Magallon # Let the source be with you...
mailto:[email protected]
Mandrake Linux release 8.3 (Cooker) for i586
Linux werewolf 2.4.19-pre7-jam1 #2 SMP Wed Apr 17 21:20:31 CEST 2002 i686
At 08:28 AM 4/17/2002 -0700, Martin J. Bligh wrote:
>> Even though clustered_apic_mode is 0, the compiler still complains
>> about the second one and the first one doesn't depend on
>> clustered_apic_mode at all.
>
>Hmmm ... not sure why the compiler complains about the second one,
>that's very strange ;-)
That's because we're using C. If we rewrote the kernel in FORTRAN, the FORTRAN compiler would happily let us redefine 0 to any other value :)
(How bout it guys? We could rewrite the kernel in C++, then run a C++-to-FORTRAN conversion script on it!)
--
Stevie-O
Real programmers use COPY CON PROGRAM.EXE
>>> Even though clustered_apic_mode is 0, the compiler still complains
>>> about the second one and the first one doesn't depend on
>>> clustered_apic_mode at all.
>>
>> Hmmm ... not sure why the compiler complains about the second one,
>> that's very strange ;-)
>
> That's because we're using C. If we rewrote the kernel in FORTRAN, the
> FORTRAN compiler would happily let us redefine 0 to any other value :)
I think you're missing the point. It shouldn't be compiling that whole if
clause
at all, as far as I can see.
M.
> Indeed. The optimization step that (presumably) removes the body
> of the if() must happen after the body has been fully evaluated.
> Makes sense, I guess, now that I think about it...
Personally, I think that's a really sick and twisted way for a compiler
to work ... what the hell is the point of compiling something you know
perfectly well you're going to dispose of 2 nanoseconds later?
M.
At 05:34 PM 4/17/2002 -0700, Martin J. Bligh wrote:
>Personally, I think that's a really sick and twisted way for a compiler
>to work ... what the hell is the point of compiling something you know
>perfectly well you're going to dispose of 2 nanoseconds later?
>
>M.
<RANT>
Granted, I could make a rather similar argument for the reverse: What the hell is the point of giving something to the compiler if we know perfectly well that it would be optimized out anyway?
(Especially if the alternative, leaving it up to the compiler, generates totally insane code like '0 = f();')
When I downloaded and put together 2.5.7, I couldn't even build the kernel because I didn't compile NFS support in ("undefined ref to sys_nfsservctl"); I dread to think of the number of errors I'd see if the compiler's optimizer dropped out blatantly erroneous code because it happened to be surrounded by 'if (0 && other_func())' and optimized out on certain builds.
Because if:
(void*) 0 = (void*) 0;
isn't blatantly erroneous (even if not evident at first hand), I don't know what is.
And if the fact that it decomposed into "0 = 0;" wasn't blatantly obvious, all the more reason for my next point:
----
Yes, I agree that as code becomes #ifdef-laden, it becomes hard to read -- at an exponential rate,
especially if it's poorly done:
#ifdef OPTION_X
x
#else
#ifdef OPTION_Y
y
#else
z
#endif
#endif
=
#ifdef OPTION_X
x_func(x,
#else
yz_func(
#ifdef OPTION_Y
y
#else
z
#endif
,
#endif
32);
Yes, I've seen beautiful code mauled by nested #if's and #ifdef's and such.
However... Which of the following is more obviously code geared towards a specific config option?
#ifdef CONFIG_OBSCURE
if (obscure_variable == 0) {
obscure_variable = init_obscure_variable_function();
}
#endif
-or-
if (obscure_variable == 0) {
obscure_variable = init_obscure_variable_function();
}
If I didn't know that 'obscure_variable' was #defined to 0 in 'obscure_header.h':
#ifdef CONFIG_OBSCURE
extern void* obscure_variable;
#else
#define obscure_variable ((void*)0)
#endif
I might wonder why my system never links in 'init_obscure_variable_function' despite the reference.
The preprocessor is a tool. It's designed to help us. Just because it's misused often doesn't mean we need to reject it in every situation. (Just like guns, the preprocessor will let you shoot yourself in the foot! :P)
Okay, I'm out of breath for now.
</RANT>
--
Stevie-O
Real programmers write code like this:
const char *FlashPROMFunc;
...
(((void)(*)(void*,void*,int,int))(FlashPROMFunc))(dst, src, count, 1);
[yes, I wrote that line of code once. yes, it was for production use. yes, it worked.]
diff -urN linux-2.5.8-dj1/include/asm-i386/io.h linux/include/asm-i386/io.h
--- linux-2.5.8-dj1/include/asm-i386/io.h Sun Apr 21 09:14:24 2002
+++ linux/include/asm-i386/io.h Sun Apr 21 09:27:20 2002
@@ -331,16 +331,8 @@
#ifdef CONFIG_MULTIQUAD
extern void *xquad_portio; /* Where the IO area was mapped */
-#else
-#define xquad_portio (0)
-#endif
-
#define XQUAD_PORT_ADDR(port, quad) (xquad_portio + (XQUAD_PORTIO_QUAD*quad) + port)
-
#define __BUILDIO(bwl,bw,type) \
-static inline void out##bwl##_local(unsigned type value, int port) { \
- __asm__ __volatile__("out" #bwl " %" #bw "0, %w1" : : "a"(value), "Nd"(port)); \
-} \
static inline void out##bwl##_quad(unsigned type value, int port, int quad) { \
if (xquad_portio) \
write##bwl(value, XQUAD_PORT_ADDR(port, quad)); \
@@ -350,18 +342,6 @@
static inline void out##bwl(unsigned type value, int port) { \
out##bwl##_quad(value, port, 0); \
} \
-static inline void out##bwl##_p(unsigned type value, int port) { \
- out##bwl(value, port); \
- slow_down_io(); \
-} \
-static inline void outs##bwl(int port, const void *addr, unsigned long count) { \
- __asm__ __volatile__("rep; outs" #bwl : "+S"(addr), "+c"(count) : "d"(port)); \
-} \
-static inline unsigned type in##bwl##_local(int port) { \
- unsigned type value; \
- __asm__ __volatile__("in" #bwl " %w1, %" #bw "0" : "=a"(value) : "Nd"(port)); \
- return value; \
-} \
static inline unsigned type in##bwl##_quad(int port, int quad) { \
if (xquad_portio) \
return read##bwl(XQUAD_PORT_ADDR(port, quad)); \
@@ -370,18 +350,46 @@
} \
static inline unsigned type in##bwl(int port) { \
return in##bwl##_quad(port, 0); \
+}
+#else
+#define __BUILDIO(bwl,bw,type) \
+static inline void out##bwl(unsigned type value, int port) { \
+ out##bwl##_local(value, port); \
+} \
+static inline unsigned type in##bwl(int port) { \
+ return in##bwl##_local(port); \
+}
+#endif
+
+
+#define BUILDIO(bwl,bw,type) \
+static inline void out##bwl##_local(unsigned type value, int port) { \
+ __asm__ __volatile__("out" #bwl " %" #bw "0, %w1" : : "a"(value), "Nd"(port)); \
+} \
+static inline unsigned type in##bwl##_local(int port) { \
+ unsigned type value; \
+ __asm__ __volatile__("in" #bwl " %w1, %" #bw "0" : "=a"(value) : "Nd"(port)); \
+ return value; \
+} \
+__BUILDIO(bwl,bw,type) \
+static inline void out##bwl##_p(unsigned type value, int port) { \
+ out##bwl(value, port); \
+ slow_down_io(); \
} \
static inline unsigned type in##bwl##_p(int port) { \
unsigned type value = in##bwl(port); \
slow_down_io(); \
return value; \
} \
+static inline void outs##bwl(int port, const void *addr, unsigned long count) { \
+ __asm__ __volatile__("rep; outs" #bwl : "+S"(addr), "+c"(count) : "d"(port)); \
+} \
static inline void ins##bwl(int port, void *addr, unsigned long count) { \
__asm__ __volatile__("rep; ins" #bwl : "+D"(addr), "+c"(count) : "d"(port)); \
}
-__BUILDIO(b,b,char)
-__BUILDIO(w,w,short)
-__BUILDIO(l,,long)
+BUILDIO(b,b,char)
+BUILDIO(w,w,short)
+BUILDIO(l,,long)
#endif