[PATCH] warnkill trivia 2/2

[PATCH] warnkill trivia 2/2

[Tomas Szepe]

2.4.20-pre5: prevent sparc32's atomic_read() from possibly discarding
const qualifiers from pointers passed as its argument.


diff -urN linux-2.4.20-pre5/include/asm-sparc/atomic.h linux-2.4.20-pre5.n/include/asm-sparc/atomic.h
--- linux-2.4.20-pre5/include/asm-sparc/atomic.h	2001-11-08 17:42:19.000000000 +0100
+++ linux-2.4.20-pre5.n/include/asm-sparc/atomic.h	2002-09-01 12:29:36.000000000 +0200
@@ -35,7 +35,7 @@
 
 #define ATOMIC_INIT(i)	{ (i << 8) }
 
-static __inline__ int atomic_read(atomic_t *v)
+static __inline__ int atomic_read(const atomic_t *v)
 {
 	int ret = v->counter;
 

Re: [PATCH] warnkill trivia 2/2

[David S. Miller]

   From: Tomas Szepe <szepe@pinerecords.com>
   Date: Sun, 1 Sep 2002 12:56:43 +0200

   2.4.20-pre5: prevent sparc32's atomic_read() from possibly discarding
   const qualifiers from pointers passed as its argument.
   
   -static __inline__ int atomic_read(atomic_t *v)
   +static __inline__ int atomic_read(const atomic_t *v)

So the atomic_t is const is it?  That's news to me.

I think you mean something like "atomic_t const * v" which means the
pointer is constant, not the value.

Re: [PATCH] warnkill trivia 2/2

[Tomas Szepe]

>    From: Tomas Szepe <szepe@pinerecords.com>
>    Date: Sun, 1 Sep 2002 12:56:43 +0200
> 
>    2.4.20-pre5: prevent sparc32's atomic_read() from possibly discarding
>    const qualifiers from pointers passed as its argument.
>    
>    -static __inline__ int atomic_read(atomic_t *v)
>    +static __inline__ int atomic_read(const atomic_t *v)
> 
> So the atomic_t is const is it?  That's news to me.
> 
> I think you mean something like "atomic_t const * v" which means the
> pointer is constant, not the value.

Precisely.


diff -u linux-2.4.20-pre5/include/asm-sparc/atomic.h linux-2.4.20-pre5.n/include/asm-sparc/atomic.h
--- linux-2.4.20-pre5/include/asm-sparc/atomic.h	2001-11-08 17:42:19.000000000 +0100
+++ linux-2.4.20-pre5.n/include/asm-sparc/atomic.h	2002-09-01 12:29:36.000000000 +0200
@@ -35,7 +35,7 @@
 
 #define ATOMIC_INIT(i)	{ (i << 8) }
 
-static __inline__ int atomic_read(atomic_t *v)
+static __inline__ int atomic_read(atomic_t const *v)
 {
 	int ret = v->counter;
 

Re: [PATCH] warnkill trivia 2/2

[David S. Miller]

   From: Tomas Szepe <szepe@pinerecords.com>
   Date: Sun, 1 Sep 2002 13:28:56 +0200

   > I think you mean something like "atomic_t const * v" which means the
   > pointer is constant, not the value.
   
   Precisely.

BTW who even passes around const atomic_t's?  Ie. what
genrated the warning and made you even edit this to begin with?

Re: [PATCH] warnkill trivia 2/2

[Tomas Szepe]

>    From: Tomas Szepe <szepe@pinerecords.com>
>    Date: Sun, 1 Sep 2002 13:28:56 +0200
> 
>    > I think you mean something like "atomic_t const * v" which means the
>    > pointer is constant, not the value.
>    
>    Precisely.
> 
> BTW who even passes around const atomic_t's?  Ie. what
> genrated the warning and made you even edit this to begin with?

fs/reiserfs/buffer2.c, line ~28:
atomic_t gets the const quality on account of being a member
of a const struct buffer_head instance.

void wait_buffer_until_released (const struct buffer_head * bh)
{
  int repeat_counter = 0;

  while (atomic_read (&(bh->b_count)) > 1) {

    if ( !(++repeat_counter % 30000000) ) {
      reiserfs_warning ("vs-3050: wait_buffer_until_released: nobody releases buffer (%b). Still waiting (%d) %cJDIRTY %cJWAIT\n",
			bh, repeat_counter, buffer_journaled(bh) ? ' ' : '!',
			buffer_journal_dirty(bh) ? ' ' : '!');
    }
    run_task_queue(&tq_disk);
    yield();
  }
  if (repeat_counter > 30000000) {
    reiserfs_warning("vs-3051: done waiting, ignore vs-3050 messages for (%b)\n", bh) ;
  }
}

Re: [PATCH] warnkill trivia 2/2

[David S. Miller]

   From: Tomas Szepe <szepe@pinerecords.com>
   Date: Sun, 1 Sep 2002 13:37:42 +0200

   > BTW who even passes around const atomic_t's?  Ie. what
   > genrated the warning and made you even edit this to begin with?
   
   fs/reiserfs/buffer2.c, line ~28:
   atomic_t gets the const quality on account of being a member
   of a const struct buffer_head instance.
   
   void wait_buffer_until_released (const struct buffer_head * bh)
   {

Reiserfs is buggy, it means struct  buffer_head const * bh

Let's keep the sparc atomic_read() how it is so more bugs
like this can be found.

Re: [PATCH] warnkill trivia 2/2

[Tomas Szepe]

> Let's keep the sparc atomic_read() how it is so more bugs
> like this can be found.

I don't know, though... scratching my head here -- Is GCC actually
able to distinguish between 'const int *a' and 'int const *a'?

Because if 'int const *a' means that the pointer is constant but
not the actual value it points to,

void a(int const *a) { *a = 1; }

shouldn't be generating 'warning: assignment of read-only location'.
Right?

> Reiserfs is buggy, it means struct  buffer_head const * bh

Okay so that gives us the third reiserfs patch of the day:

--- buffer2.c~	2002-09-01 13:52:39.000000000 +0200
+++ buffer2.c	2002-09-01 13:44:19.000000000 +0200
@@ -21,7 +21,7 @@
    hold we did free all buffers in tree balance structure
    (get_empty_nodes and get_nodes_for_preserving) or in path structure
    only (get_new_buffer) just before calling this */
-void wait_buffer_until_released (const struct buffer_head * bh)
+void wait_buffer_until_released (struct buffer_head const *bh)
 {
   int repeat_counter = 0;
 

Re: [PATCH] warnkill trivia 2/2

[David S. Miller]

   From: Tomas Szepe <szepe@pinerecords.com>
   Date: Sun, 1 Sep 2002 14:10:53 +0200

   > Let's keep the sparc atomic_read() how it is so more bugs
   > like this can be found.
   
   I don't know, though... scratching my head here -- Is GCC actually
   able to distinguish between 'const int *a' and 'int const *a'?
   
No I don't mean this in a "C" sense, I mean conceptually that marking
an object const which has members which are volatile and updated
asynchronously makes zero sense.

Re: [PATCH] warnkill trivia 2/2

[Tomas Szepe]

>    From: Tomas Szepe <szepe@pinerecords.com>
>    Date: Sun, 1 Sep 2002 14:10:53 +0200
> 
>    > Let's keep the sparc atomic_read() how it is so more bugs
>    > like this can be found.
>    
>    I don't know, though... scratching my head here -- Is GCC actually
>    able to distinguish between 'const int *a' and 'int const *a'?
>    
> No I don't mean this in a "C" sense, I mean conceptually that marking
> an object const which has members which are volatile and updated
> asynchronously makes zero sense.

True.

I've been playing a bit with how gcc handles the const qualifiers
and made an interesting discovery:

Trying to compile

typedef int *p_int;
void a(const p_int t) { *t = 0; }
void b(const p_int t) { t = (int *) 0; }
void c(const int *t) { *t = 0; }
void d(const int *t) { t = (int *) 0; }
void e(int const *t) { *t = 0; }
void f(int const *t) { t = (int *) 0; }

will give 'assignment of read-only location' warnings for
b(), c() and e(), i.e. it's impossible to have a constant
pointer to a non-constant value w/o using a qualified
typedef.

W/o a typedef, gcc seems unable to tell the difference
between 'const int *' and 'int const *' altogether. In case
one needs a constant pointer to a constant value, something
like this should do:

typedef const int *p_int;
void f(const p_int a);

Usable? I don't quite think so.

T.

Re: [PATCH] warnkill trivia 2/2

[Bruce Guenter]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On Sun, Sep 01, 2002 at 02:39:03PM +0200, Tomas Szepe wrote:
> I've been playing a bit with how gcc handles the const qualifiers
> and made an interesting discovery:
> 
> Trying to compile
> 
> typedef int *p_int;
> void a(const p_int t) { *t = 0; }
> void b(const p_int t) { t = (int *) 0; }
> void c(const int *t) { *t = 0; }
> void d(const int *t) { t = (int *) 0; }
> void e(int const *t) { *t = 0; }
> void f(int const *t) { t = (int *) 0; }
> 
> will give 'assignment of read-only location' warnings for
> b(), c() and e(), i.e. it's impossible to have a constant
> pointer to a non-constant value w/o using a qualified
> typedef.

If you want a constant *pointer*, use:
	void f(int* const t)
	(read "f is a function, taking parameter constant pointer to
	int, returning void)

> W/o a typedef, gcc seems unable to tell the difference
> between 'const int *' and 'int const *' altogether.

That's because there is no difference ("pointer to integer constant" vs
"pointer to constant integer").

See http://untroubled.org/articles/cdecls.txt for one of the best
references I've ever seen to understanding C type declarations.
-- 
Bruce Guenter <bruceg@em.ca> http://em.ca/~bruceg/ http://untroubled.org/
OpenPGP key: 699980E8 / D0B7 C8DD 365D A395 29DA  2E2A E96F B2DC 6999 80E8

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] warnkill trivia 2/2

[Jan Hudec]

On Sun, Sep 01, 2002 at 01:28:56PM +0200, Tomas Szepe wrote:
> >    From: Tomas Szepe <szepe@pinerecords.com>
> >    Date: Sun, 1 Sep 2002 12:56:43 +0200
> > 
> >    2.4.20-pre5: prevent sparc32's atomic_read() from possibly discarding
> >    const qualifiers from pointers passed as its argument.
> >    
> >    -static __inline__ int atomic_read(atomic_t *v)
> >    +static __inline__ int atomic_read(const atomic_t *v)
> > 
> > So the atomic_t is const is it?  That's news to me.
> > 
> > I think you mean something like "atomic_t const * v" which means the
> > pointer is constant, not the value.
> 
> Precisely.
> 

No, you don't. Having the pointer constant means the symbolic argument
can't be changed inside the function. But it means nothing at all to the
caller, because the caller's variable itself is never changed by the
call. 

> 
> diff -u linux-2.4.20-pre5/include/asm-sparc/atomic.h linux-2.4.20-pre5.n/include/asm-sparc/atomic.h
> --- linux-2.4.20-pre5/include/asm-sparc/atomic.h	2001-11-08 17:42:19.000000000 +0100
> +++ linux-2.4.20-pre5.n/include/asm-sparc/atomic.h	2002-09-01 12:29:36.000000000 +0200
> @@ -35,7 +35,7 @@
>  
>  #define ATOMIC_INIT(i)	{ (i << 8) }
>  
> -static __inline__ int atomic_read(atomic_t *v)
> +static __inline__ int atomic_read(atomic_t const *v)
>  {
>  	int ret = v->counter;
-------------------------------------------------------------------------------
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

[PATCH] warnkill trivia 2/2 (correction)

[Tomas Szepe]

> 2.4.20-pre5: prevent sparc32's atomic_read() from possibly discarding
> const qualifiers from pointers passed as its argument.

Ok, the only reasonable way to deal with the last reiserfs vs.
sparc32 compilation warning is apparently to strip the const from
the wait_buffer_until_released() prototype, as it doesn't make any
sense there. Marcelo please disregard the atomic_read() patch and
apply the following instead:


diff -urN linux-2.4.20-pre5/fs/reiserfs/buffer2.c linux-2.4.20-pre5.n/fs/reiserfs/buffer2.c
--- linux-2.4.20-pre5/fs/reiserfs/buffer2.c	2002-09-01 17:19:26.000000000 +0200
+++ linux-2.4.20-pre5.n/fs/reiserfs/buffer2.c	2002-09-01 17:07:27.000000000 +0200
@@ -21,7 +21,7 @@
    hold we did free all buffers in tree balance structure
    (get_empty_nodes and get_nodes_for_preserving) or in path structure
    only (get_new_buffer) just before calling this */
-void wait_buffer_until_released (const struct buffer_head * bh)
+void wait_buffer_until_released (struct buffer_head *bh)
 {
   int repeat_counter = 0;
 
diff -urN linux-2.4.20-pre5/include/linux/reiserfs_fs.h linux-2.4.20-pre5.n/include/linux/reiserfs_fs.h
--- linux-2.4.20-pre5/include/linux/reiserfs_fs.h	2002-09-01 17:19:27.000000000 +0200
+++ linux-2.4.20-pre5.n/include/linux/reiserfs_fs.h	2002-09-01 17:16:32.000000000 +0200
@@ -1845,7 +1847,7 @@
 
 /* buffer2.c */
 struct buffer_head * reiserfs_getblk (kdev_t n_dev, int n_block, int n_size);
-void wait_buffer_until_released (const struct buffer_head * bh);
+void wait_buffer_until_released (struct buffer_head *bh);
 struct buffer_head * reiserfs_bread (struct super_block *super, int n_block, 
 				     int n_size);