Date: Wed, 19 Sep 2012 00:08:58 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Poul-Henning Kamp <phk@phk.freebsd.dk> Cc: arch@freebsd.org Subject: Re: Aliasing issue with TAILQ on ppc64 ? Message-ID: <20120918220858.GB56160@stack.nl> In-Reply-To: <23178.1347999292@critter.freebsd.dk> References: <20120918195353.GA56160@stack.nl> <23178.1347999292@critter.freebsd.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120918220858.GB56160>