From owner-freebsd-arch@FreeBSD.ORG Tue Sep 18 22:09:03 2012 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5CAE9106578B for ; Tue, 18 Sep 2012 22:09:03 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (unknown [IPv6:2001:610:1108:5012::107]) by mx1.freebsd.org (Postfix) with ESMTP id B85C48FC08 for ; Tue, 18 Sep 2012 22:09:02 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 1FFA612013E; Wed, 19 Sep 2012 00:08:59 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id E251A2847B; Wed, 19 Sep 2012 00:08:58 +0200 (CEST) Date: Wed, 19 Sep 2012 00:08:58 +0200 From: Jilles Tjoelker To: Poul-Henning Kamp Message-ID: <20120918220858.GB56160@stack.nl> References: <20120918195353.GA56160@stack.nl> <23178.1347999292@critter.freebsd.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <23178.1347999292@critter.freebsd.dk> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: arch@freebsd.org Subject: Re: Aliasing issue with TAILQ on ppc64 ? X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2012 22:09:03 -0000 On Tue, Sep 18, 2012 at 08:14:52PM +0000, Poul-Henning Kamp wrote: > In message <20120918195353.GA56160@stack.nl>, Jilles Tjoelker writes: > >An obvious fix is to make TAILQ_ENTRY and TAILQ_HEAD the same type (and > >not just structurally identical) or to add an intermediate struct which > >is the same between them. > I've tried something along those lines several times, but I have so far > not been able to make it work, without a major change to the TAILQ_* > api, which I am not prepared to push. > >However, I think the TAILQ_LAST and TAILQ_PREV macros are better > >rewritten using __containerof, > I really don't think that is an improvement, I'd prefer a typesafe > standard C solution which static checker tools like Coverity and > FlexeLint can see how works. I think it is impossible to access another item's tqe_prev with a plain pointer cast and no GCC __typeof__. This is because each time TAILQ_ENTRY is used, it creates a new struct type that cannot be accessed other than via the outer struct (hence needing a 'field' macro parameter (which TAILQ_LAST does not have) and a way to refer to the outer struct type (__typeof__ or a new macro parameter). This is basically the __containerof solution. In the TAILQ_PREV macro, the field parameter together with __typeof__ can be used to construct a cast that will allow legal access to another entry's tqe_prev. However, an extra macro parameter would be required to ensure we do not access the tail queue head using such a pointer. The TAILQ_LAST macro lacks the field parameter to do this. Probably even uglier is to construct a pointer to tqe_prev without involving an lvalue of type struct headname. For example (untested): #define TAILQ_LAST(head, headname, type) \ (**(struct type ***)((char *)(head)->tqh_last + \ offsetof(struct headname, tqh_last))) With this, there is no strict-aliasing violation: a struct type ** object is accessed using an lvalue of the same type. The offsetof expression returns an integer (size_t) and the pointer arithmetic remains within bounds. As with the current illegal code using a pointer cast, it is assumed that TAILQ_HEAD and TAILQ_ENTRY have the same representation. However, it again needs __typeof__ or an extra macro parameter. A less ugly option breaks ABI, but I think API can be preserved: #define TAILQ_HEAD(name, type) \ struct name { struct type *tqh_first, *tqh_last; } #define TAILQ_ENTRY(type) \ struct { struct type *tqe_next, *tqe_prev; } The macros will be easier to understand; they will have more branches but fewer pointer dereferences than what we have now. > I suspect it would be enough to make the tqh_last and tqe_prev > pointer be volatile pointers to struct type pointers, but absent > a deeper understanding of whats actually going on I can't tell > if that would be a proper solution or merely obfuscation and > workaround. The text in the C standard about strict-aliasing does not make an exception for volatile pointers, so it remains undefined behaviour. However, it is quite likely that compilers will generate code as expected. The strict-aliasing rules are generally used to assume certain objects cannot alias, but this is irrelevant when reads and writes have to go to memory directly anyway. Another workaround is GCC's __attribute__((__may_alias__)), added to the TAILQ_HEAD and TAILQ_ENTRY macros. It is also an option to use a "proper" solution using __typeof__ if available and a hack using volatile if not. -- Jilles Tjoelker