Message-ID: <4AC5EA67.1080806@kernel.org>
Date: Fri, 02 Oct 2009 20:56:23 +0900
From: Tejun Heo <tj@kernel.org>
User-Agent: Thunderbird 2.0.0.22 (X11/20090605)
MIME-Version: 1.0
To: blp@cs.stanford.edu
CC: linux-kernel@vger.kernel.org, Jeff Garzik <jeff@garzik.org>,
       Alan Cox <alan@lxorguk.ukuu.org.uk>,
       David Howells <dhowells@redhat.com>, Ingo Molnar <mingo@elte.hu>,
       Andrew Morton <akpm@linux-foundation.org>,
       Jens Axboe <jens.axboe@oracle.com>,
       Rusty Russell <rusty@rustcorp.com.au>,
       Christoph Lameter <cl@linux-foundation.org>,
       Arjan van de Ven <arjan@infradead.org>
Subject: Re: [PATCH 10/19] workqueue: update cwq alignement and make one more
 flag bit available
References: <1254384558-1018-11-git-send-email-tj@kernel.org>	<1254384558-1018-1-git-send-email-tj@kernel.org>	<10133.1254402320@redhat.com> <87zl8bhs0s.fsf@blp.benpfaff.org>
In-Reply-To: <87zl8bhs0s.fsf@blp.benpfaff.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2394
Lines: 83

(Restoring cc list.  Please don't drop them)

Ben Pfaff wrote:
> David Howells <dhowells@redhat.com> writes:
> 
>> Tejun Heo <tj@kernel.org> wrote:
>>
>>> +enum {
>>> +	WORK_STRUCT_PENDING	= 0,	/* work item is pending execution */
>>> +
>>> +	/*
>>> +	 * Reserve 3bits off of cwq pointer.  This is enough and
>>> +	 * provides acceptable alignment on both 32 and 64bit
>>> +	 * machines.
>>> +	 */
>>> +	WORK_STRUCT_FLAG_BITS	= 3,
>>> +
>>> +	WORK_STRUCT_FLAG_MASK	= (1UL << WORK_STRUCT_FLAG_BITS) - 1,
>>> +	WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
>>> +};
>> There's some great enum abuse going on here:-)
> 
> The "1UL" part is a bit worrisome.  Enumeration constants always
> have type "int"[*], so if code that uses WORK_STRUCT_WQ_DATA_MASK
> actually depends on getting a full "long" worth of bits, it is
> not going to work on 64-bit systems.
> 
> [*] See C99:
> 
>      6.4.4.3 Enumeration constants
>      Syntax
> 1              enumeration-constant:
>                       identifier
>      Semantics
> 2    An identifier declared as an enumeration constant has type int.

Aieee... oops.  Well, this isn't how gcc behaves.

  $ cat test.c
  #include <stdio.h>

  enum {
	  ENUM		= ~0U,
  };

  enum {
	  LONG_ENUM	= ~0UL,
  };

  int main(void)
  {
	  printf("%zu %zu\n", sizeof(ENUM), sizeof(LONG_ENUM));
	  return 0;
  }
  $ gcc test.c && ./a.out; gcc -std=c99 test.c && ./a.out
  4 8
  4 8

But, yeah, this definitely is a problem.  6.7.2.2 also says that

 Each enumerated type shall be compatible with char, a signed integer
 type, or an unsigned integer type. The choice of type is
 implementation-defined,113) but shall be capable of representing the
 values of all the members of the enumeration. The enumerated type is
 incomplete until after the } that terminates the list of enumerator
 declarations.

gcc probably is being a bit too generous with possible integer types
here.  BTW, does c++ define it like this too?

Hmmm... So, should we go back to using defines for these or keep
(ab)using gcc's generousity and maybe hope the next iteration of the
standard to become a bit more generous too?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/