Re: [PATCH 1/1] Work around gcc-4.7's strict aliasing checks

David Brown <david.brown@xxxxxxxxxxxx> · Thu, 12 Jan 2012 08:14:59 +0100

On 12/01/12 00:43, NeilBrown wrote:
On Thu,  5 Jan 2012 12:16:41 +0100 Jes.Sorensen@xxxxxxxxxx wrote:

From: Jes Sorensen<Jes.Sorensen@xxxxxxxxxx>

Signed-off-by: Jes Sorensen<Jes.Sorensen@xxxxxxxxxx>
---
  sha1.c      |    8 +++++---
  super-ddf.c |   22 ++++++++++++++--------
  2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/sha1.c b/sha1.c
index 556d9ca..0258515 100644
--- a/sha1.c
+++ b/sha1.c
@@ -101,6 +101,7 @@ sha1_finish_ctx (struct sha1_ctx *ctx, void *resbuf)
    /* Take yet unprocessed bytes into account.  */
    md5_uint32 bytes = ctx->buflen;
    size_t pad;
+  md5_uint32 *ptr;

    /* Now count remaining bytes.  */
    ctx->total[0] += bytes;
@@ -111,9 +112,10 @@ sha1_finish_ctx (struct sha1_ctx *ctx, void *resbuf)
    memcpy (&ctx->buffer[bytes], fillbuf, pad);

    /* Put the 64-bit file length in *bits* at the end of the buffer.  */
-  *(md5_uint32 *)&ctx->buffer[bytes + pad + 4] = SWAP (ctx->total[0]<<  3);
-  *(md5_uint32 *)&ctx->buffer[bytes + pad] = SWAP ((ctx->total[1]<<  3) |
-						    (ctx->total[0]>>  29));
+  ptr = (md5_uint32 *)&ctx->buffer[bytes + pad + 4];
+  *ptr = SWAP (ctx->total[0]<<  3);
+  ptr = (md5_uint32 *)&ctx->buffer[bytes + pad];
+  *ptr = SWAP ((ctx->total[1]<<  3) | (ctx->total[0]>>  29));

    /* Process last bytes.  */
    sha1_process_block (ctx->buffer, bytes + pad + 8, ctx);
diff --git a/super-ddf.c b/super-ddf.c
index b5b0b42..abd6793 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1336,6 +1336,7 @@ static void getinfo_super_ddf(struct supertype *st, struct mdinfo *info, char *m
  {
  	struct ddf_super *ddf = st->sb;
  	int map_disks = info->array.raid_disks;
+	__u32 *cptr;

  	if (ddf->currentconf) {
  		getinfo_super_ddf_bvd(st, info, map);
@@ -1347,8 +1348,9 @@ static void getinfo_super_ddf(struct supertype *st, struct mdinfo *info, char *m
  	info->array.level	  = LEVEL_CONTAINER;
  	info->array.layout	  = 0;
  	info->array.md_minor	  = -1;
-	info->array.ctime	  = DECADE + __be32_to_cpu(*(__u32*)
-							 (ddf->anchor.guid+16));
+	cptr = (__u32 *)(ddf->anchor.guid + 16);
+	info->array.ctime	  = DECADE + __be32_to_cpu(*cptr);
+
  	info->array.utime	  = 0;
  	info->array.chunk_size	  = 0;
  	info->container_enough	  = 1;
@@ -1407,6 +1409,7 @@ static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, cha
  	int j;
  	struct dl *dl;
  	int map_disks = info->array.raid_disks;
+	__u32 *cptr;

  	memset(info, 0, sizeof(*info));
  	/* FIXME this returns BVD info - what if we want SVD ?? */
@@ -1416,8 +1419,8 @@ static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, cha
  	info->array.layout	  = rlq_to_layout(vc->conf.rlq, vc->conf.prl,
  						  info->array.raid_disks);
  	info->array.md_minor	  = -1;
-	info->array.ctime	  = DECADE +
-		__be32_to_cpu(*(__u32*)(vc->conf.guid+16));
+	cptr = (__u32 *)(vc->conf.guid + 16);
+	info->array.ctime	  = DECADE + __be32_to_cpu(*cptr);
  	info->array.utime	  = DECADE + __be32_to_cpu(vc->conf.timestamp);
  	info->array.chunk_size	  = 512<<  vc->conf.chunk_shift;
  	info->custom_array_size	  = 0;
@@ -2192,6 +2195,7 @@ static int add_to_super_ddf(struct supertype *st,
  	struct phys_disk_entry *pde;
  	unsigned int n, i;
  	struct stat stb;
+	__u32 *tptr;

  	if (ddf->currentconf) {
  		add_to_super_ddf_bvd(st, dk, fd, devname);
@@ -2220,8 +2224,9 @@ static int add_to_super_ddf(struct supertype *st,
  	tm = localtime(&now);
  	sprintf(dd->disk.guid, "%8s%04d%02d%02d",
  		T10, tm->tm_year+1900, tm->tm_mon+1, tm->tm_mday);
-	*(__u32*)(dd->disk.guid + 16) = random32();
-	*(__u32*)(dd->disk.guid + 20) = random32();
+	tptr = (__u32 *)(dd->disk.guid + 16);
+	*tptr++ = random32();
+	*tptr = random32();

  	do {
  		/* Cannot be bothered finding a CRC of some irrelevant details*/
@@ -2967,6 +2972,7 @@ static struct mdinfo *container_content_ddf(struct supertype *st, char *subarray
  		unsigned int j;
  		struct mdinfo *this;
  		char *ep;
+		__u32 *cptr;

  		if (subarray&&
  		(strtoul(subarray,&ep, 10) != vc->vcnum ||
@@ -2986,8 +2992,8 @@ static struct mdinfo *container_content_ddf(struct supertype *st, char *subarray
  		this->array.md_minor      = -1;
  		this->array.major_version = -1;
  		this->array.minor_version = -2;
-		this->array.ctime         = DECADE +
-			__be32_to_cpu(*(__u32*)(vc->conf.guid+16));
+		cptr = (__u32 *)(vc->conf.guid + 16);
+		this->array.ctime         = DECADE + __be32_to_cpu(*cptr);
  		this->array.utime	  = DECADE +
  			__be32_to_cpu(vc->conf.timestamp);
  		this->array.chunk_size	  = 512<<  vc->conf.chunk_shift;


Applied - thanks...

Do you understand this stuff?  If this really "fixing" anything, or is it
still doing something bad, but is now hiding it from gcc's warning so we just
don't get told how naughty we are?

Should these things be changed to use unions ??


I don't understand much about the code in question (having not read it), 
but I /do/ understand a little about pointer aliasing.  And at first 
glance, this looks to me like it might be glossing over the problems. 
Maybe it will help now - but that depends on optimisation levels and 
exact code generation.

The basic rule is that the compiler can assume that objects whose type 
has different sizes, cannot appear at the same address.  Unions are one 
way to avoid this.

The relevant parts of the gcc manual are definitely worth reading:

<http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-fstrict_002daliasing-849>

<http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstrict_002daliasing_003dn-349>

Make sure you enable -Wstrict-aliasing=3 - this will let the compiler 
warn you of potential problems.  As a last resort, it is also possible 
to use pragmas to turn off strict-aliasing in the relevant functions.

mvh.,

David

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html





