curcpu() and curlwp()

[David Laight]

I've just been looking at the definitions of curcpu() etc on
i386 and amd64 (they differ only in the segment register used).

in sys/arch/i386/include/cpu.h we have:

__inline static lwp_t * __attribute__ ((const))
x86_curlwp(void)
{
lwp_t *l;

__asm volatile("movl %%fs:%1, %0" :
"=r" (l) :
"m"
(*(struct cpu_info * const *)offsetof(struct cpu_info, ci_curlwp)));
return l;
}

I think it should be:
"m" (((const struct cpu_info *)0)->ci_curlwp)

The actual code seems to be a reference to a pointer to 'struct cpu_info'.
The generated code is probably identical, but the inferences are different.

(cpu_set_curpri() is broken in a different, but similar way.)

I also think that the curcpu() and culwp() functions need not be volatile.

David

--
David Laight: da...@l8s.co.uk

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de

[Christos Zoulas]

In article <2011060517...@snowdrop.l8s.co.uk>,

David Laight <da...@l8s.co.uk> wrote:
>I've just been looking at the definitions of curcpu() etc on
>i386 and amd64 (they differ only in the segment register used).
>
>in sys/arch/i386/include/cpu.h we have:
>
>__inline static lwp_t * __attribute__ ((const))
>x86_curlwp(void)
>{
> lwp_t *l;
>
> __asm volatile("movl %%fs:%1, %0" :
> "=r" (l) :
> "m"
> (*(struct cpu_info * const *)offsetof(struct cpu_info, ci_curlwp)));
> return l;
>}
>
>I think it should be:
> "m" (((const struct cpu_info *)0)->ci_curlwp)
>
>The actual code seems to be a reference to a pointer to 'struct cpu_info'.
>The generated code is probably identical, but the inferences are different.
>
>(cpu_set_curpri() is broken in a different, but similar way.)
>
>I also think that the curcpu() and culwp() functions need not be volatile.

I think that if offsetof() produces suboptimal code, it should be fixed,
not expanded in place differently.

christos

[der Mouse]

[Christos, quoting David Laight]

>> (*(struct cpu_info * const *)offsetof(struct cpu_info, ci_curlwp)));

>> I think it should be:
>> "m" (((const struct cpu_info *)0)->ci_curlwp)

> I think that if offsetof() produces suboptimal code, it should be

> fixed, not expanded in place differently.

I would agree. But, as David said...

>> The generated code is probably identical, but the inferences are
>> different.

...the difference is not the generated code and its optimality, but the
conceptual thing the code is expressing.

Consider the following, which (assuming a not totally brain-dead
compiler, and given "void foo(int);") produce bit-identical code:

foo((int)M_PI);

foo(ICMP6_TIME_EXCEEDED);

foo(D_TTY);

(given "int shifts[] = { 2, 4, 16 };")
foo(sizeof(shifts)/sizeof(shifts[0]));

foo(ESRCH);

foo(3); /* first fd above stdin/stdout/stderr */

#define MAX_RETRIES 3
foo(MAX_RETRIES);

#define LG_PAGE_SIZE 12 /* this hardware uses 4k pages */
#define LG_DEVBLK_SIZE 9 /* the drive uses half-k blocks */
foo(LG_PAGE_SIZE-LG_DEVBLK_SIZE);

But the implications, the concepts they are expressing, differ wildly,
and writing one of them when meaning one of the others could quite
rightly be called a bug. Even though it'll work fine.

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML mo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B