[PATCH 1/2] evaluate: split out implementation of compatible_assignment_types

[PATCH 1/2] evaluate: split out implementation of compatible_assignment_types

[John Keeping]

This will allow us to reuse the logic when processing a transparent
union by checking each member in turn without printing a warning unless
none of the members match.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 evaluate.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 6655615..2e6511b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1307,10 +1307,9 @@ static int whitelist_pointers(struct symbol *t1, struct symbol *t2)
 	return !Wtypesign;
 }
 
-static int compatible_assignment_types(struct expression *expr, struct symbol *target,
-	struct expression **rp, const char *where)
+static int check_assignment_types(struct symbol *target, struct expression **rp,
+	const char **typediff)
 {
-	const char *typediff;
 	struct symbol *source = degenerate(*rp);
 	struct symbol *t, *s;
 	int tclass = classify_type(target, &t);
@@ -1327,8 +1326,8 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 				return 1;
 		} else if (!(sclass & TYPE_RESTRICT))
 			goto Cast;
-		typediff = "different base types";
-		goto Err;
+		*typediff = "different base types";
+		return 0;
 	}
 
 	if (tclass == TYPE_PTR) {
@@ -1342,8 +1341,8 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 			goto Cast;
 		}
 		if (!(sclass & TYPE_PTR)) {
-			typediff = "different base types";
-			goto Err;
+			*typediff = "different base types";
+			return 0;
 		}
 		b1 = examine_pointer_target(t);
 		b2 = examine_pointer_target(s);
@@ -1356,19 +1355,19 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 			 * or mix address spaces [sparse].
 			 */
 			if (t->ctype.as != s->ctype.as) {
-				typediff = "different address spaces";
-				goto Err;
+				*typediff = "different address spaces";
+				return 0;
 			}
 			if (mod2 & ~mod1) {
-				typediff = "different modifiers";
-				goto Err;
+				*typediff = "different modifiers";
+				return 0;
 			}
 			goto Cast;
 		}
 		/* It's OK if the target is more volatile or const than the source */
-		typediff = type_difference(&t->ctype, &s->ctype, 0, mod1);
-		if (typediff)
-			goto Err;
+		*typediff = type_difference(&t->ctype, &s->ctype, 0, mod1);
+		if (*typediff)
+			return 0;
 		return 1;
 	}
 
@@ -1379,22 +1378,34 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 		/* XXX: need to turn into comparison with NULL */
 		if (t == &bool_ctype && (sclass & TYPE_PTR))
 			goto Cast;
-		typediff = "different base types";
-		goto Err;
+		*typediff = "different base types";
+		return 0;
 	}
-	typediff = "invalid types";
-
-Err:
-	warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
-	info(expr->pos, "   expected %s", show_typename(target));
-	info(expr->pos, "   got %s", show_typename(source));
-	*rp = cast_to(*rp, target);
+	*typediff = "invalid types";
 	return 0;
+
 Cast:
 	*rp = cast_to(*rp, target);
 	return 1;
 }
 
+static int compatible_assignment_types(struct expression *expr, struct symbol *target,
+	struct expression **rp, const char *where)
+{
+	const char *typediff;
+	struct symbol *source = degenerate(*rp);
+
+	if (!check_assignment_types(target, rp, &typediff)) {
+		warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
+		info(expr->pos, "   expected %s", show_typename(target));
+		info(expr->pos, "   got %s", show_typename(source));
+		*rp = cast_to(*rp, target);
+		return 0;
+	}
+
+	return 1;
+}
+
 static void mark_assigned(struct expression *expr)
 {
 	struct symbol *sym;
-- 
1.9.0.6.g037df60.dirty

[PATCH 2/2] Support GCC's transparent unions

[John Keeping]

This stops warnings in code using socket operations with a modern glibc,
which otherwise result in warnings of the form:

	warning: incorrect type in argument 2 (invalid types)
	   expected union __CONST_SOCKADDR_ARG [usertype] __addr
	   got struct sockaddr *<noident>

Since transparent unions are only applicable to function arguments, we
create a new function to check that the types are compatible
specifically in this context.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 evaluate.c                     | 28 +++++++++++++++++++++++++++-
 lib.c                          |  2 --
 lib.h                          |  1 -
 parse.c                        |  6 ++++--
 sparse.1                       |  8 --------
 symbol.h                       |  3 ++-
 validation/transparent-union.c | 25 +++++++++++++++++++++++++
 7 files changed, 58 insertions(+), 15 deletions(-)
 create mode 100644 validation/transparent-union.c

diff --git a/evaluate.c b/evaluate.c
index 2e6511b..f36c6c1 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1406,6 +1406,32 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 	return 1;
 }
 
+static int compatible_argument_type(struct expression *expr, struct symbol *target,
+	struct expression **rp, const char *where)
+{
+	const char *typediff;
+	struct symbol *source = degenerate(*rp);
+	struct symbol *t;
+	classify_type(target, &t);
+
+	if (t->type == SYM_UNION && t->transparent_union) {
+		struct symbol *member;
+		FOR_EACH_PTR(t->symbol_list, member) {
+			if (check_assignment_types(member, rp, &typediff))
+				return 1;
+		} END_FOR_EACH_PTR(member);
+	}
+
+	if (check_assignment_types(target, rp, &typediff))
+		return 1;
+
+	warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
+	info(expr->pos, "   expected %s", show_typename(target));
+	info(expr->pos, "   got %s", show_typename(source));
+	*rp = cast_to(*rp, target);
+	return 0;
+}
+
 static void mark_assigned(struct expression *expr)
 {
 	struct symbol *sym;
@@ -2172,7 +2198,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
 			static char where[30];
 			examine_symbol_type(target);
 			sprintf(where, "argument %d", i);
-			compatible_assignment_types(expr, target, p, where);
+			compatible_argument_type(expr, target, p, where);
 		}
 
 		i++;
diff --git a/lib.c b/lib.c
index bf3e91c..a52b88e 100644
--- a/lib.c
+++ b/lib.c
@@ -226,7 +226,6 @@ int Wparen_string = 0;
 int Wptr_subtraction_blows = 0;
 int Wreturn_void = 0;
 int Wshadow = 0;
-int Wtransparent_union = 0;
 int Wtypesign = 0;
 int Wundef = 0;
 int Wuninitialized = 1;
@@ -438,7 +437,6 @@ static const struct warning {
 	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
 	{ "return-void", &Wreturn_void },
 	{ "shadow", &Wshadow },
-	{ "transparent-union", &Wtransparent_union },
 	{ "typesign", &Wtypesign },
 	{ "undef", &Wundef },
 	{ "uninitialized", &Wuninitialized },
diff --git a/lib.h b/lib.h
index f09b338..197b549 100644
--- a/lib.h
+++ b/lib.h
@@ -120,7 +120,6 @@ extern int Wparen_string;
 extern int Wptr_subtraction_blows;
 extern int Wreturn_void;
 extern int Wshadow;
-extern int Wtransparent_union;
 extern int Wtypesign;
 extern int Wundef;
 extern int Wuninitialized;
diff --git a/parse.c b/parse.c
index 9cc5f65..bf62bdd 100644
--- a/parse.c
+++ b/parse.c
@@ -1207,8 +1207,10 @@ static struct token *attribute_designated_init(struct token *token, struct symbo
 
 static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct decl_state *ctx)
 {
-	if (Wtransparent_union)
-		warning(token->pos, "ignoring attribute __transparent_union__");
+	if (ctx->ctype.base_type && ctx->ctype.base_type->type == SYM_UNION)
+		ctx->ctype.base_type->transparent_union = 1;
+	else
+		warning(token->pos, "attribute __transparent_union__ applied to non-union type");
 	return token;
 }
 
diff --git a/sparse.1 b/sparse.1
index cd6be26..6e79202 100644
--- a/sparse.1
+++ b/sparse.1
@@ -297,14 +297,6 @@ Such declarations can lead to error-prone code.
 Sparse does not issue these warnings by default.
 .
 .TP
-.B \-Wtransparent\-union
-Warn about any declaration using the GCC extension
-\fB__attribute__((transparent_union))\fR.
-
-Sparse issues these warnings by default.  To turn them off, use
-\fB\-Wno\-transparent\-union\fR.
-.
-.TP
 .B \-Wtypesign
 Warn when converting a pointer to an integer type into a pointer to an integer
 type with different signedness.
diff --git a/symbol.h b/symbol.h
index 43c165b..ccb5dcb 100644
--- a/symbol.h
+++ b/symbol.h
@@ -174,7 +174,8 @@ struct symbol {
 					evaluated:1,
 					string:1,
 					designated_init:1,
-					forced_arg:1;
+					forced_arg:1,
+					transparent_union:1;
 			struct expression *array_size;
 			struct ctype ctype;
 			struct symbol_list *arguments;
diff --git a/validation/transparent-union.c b/validation/transparent-union.c
new file mode 100644
index 0000000..149c7d9
--- /dev/null
+++ b/validation/transparent-union.c
@@ -0,0 +1,25 @@
+struct a {
+	int field;
+};
+struct b {
+	int field;
+};
+
+typedef union {
+	struct a *a;
+	struct b *b;
+} transparent_arg __attribute__((__transparent_union__));
+
+static void foo(transparent_arg arg)
+{
+}
+
+static void bar(void)
+{
+	struct b arg = { 0 };
+	foo((struct a *) &arg);
+}
+
+/*
+ * check-name: Transparent union attribute.
+ */
-- 
1.9.0.6.g037df60.dirty

Re: [PATCH 1/2] evaluate: split out implementation of compatible_assignment_types

[Josh Triplett]

On Sat, Mar 01, 2014 at 11:41:38AM +0000, John Keeping wrote:
> This will allow us to reuse the logic when processing a transparent
> union by checking each member in turn without printing a warning unless
> none of the members match.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  evaluate.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index 6655615..2e6511b 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1307,10 +1307,9 @@ static int whitelist_pointers(struct symbol *t1, struct symbol *t2)
>  	return !Wtypesign;
>  }
>  
> -static int compatible_assignment_types(struct expression *expr, struct symbol *target,
> -	struct expression **rp, const char *where)
> +static int check_assignment_types(struct symbol *target, struct expression **rp,
> +	const char **typediff)
>  {
> -	const char *typediff;
>  	struct symbol *source = degenerate(*rp);
>  	struct symbol *t, *s;
>  	int tclass = classify_type(target, &t);
> @@ -1327,8 +1326,8 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  				return 1;
>  		} else if (!(sclass & TYPE_RESTRICT))
>  			goto Cast;
> -		typediff = "different base types";
> -		goto Err;
> +		*typediff = "different base types";
> +		return 0;
>  	}
>  
>  	if (tclass == TYPE_PTR) {
> @@ -1342,8 +1341,8 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  			goto Cast;
>  		}
>  		if (!(sclass & TYPE_PTR)) {
> -			typediff = "different base types";
> -			goto Err;
> +			*typediff = "different base types";
> +			return 0;
>  		}
>  		b1 = examine_pointer_target(t);
>  		b2 = examine_pointer_target(s);
> @@ -1356,19 +1355,19 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  			 * or mix address spaces [sparse].
>  			 */
>  			if (t->ctype.as != s->ctype.as) {
> -				typediff = "different address spaces";
> -				goto Err;
> +				*typediff = "different address spaces";
> +				return 0;
>  			}
>  			if (mod2 & ~mod1) {
> -				typediff = "different modifiers";
> -				goto Err;
> +				*typediff = "different modifiers";
> +				return 0;
>  			}
>  			goto Cast;
>  		}
>  		/* It's OK if the target is more volatile or const than the source */
> -		typediff = type_difference(&t->ctype, &s->ctype, 0, mod1);
> -		if (typediff)
> -			goto Err;
> +		*typediff = type_difference(&t->ctype, &s->ctype, 0, mod1);
> +		if (*typediff)
> +			return 0;
>  		return 1;
>  	}
>  
> @@ -1379,22 +1378,34 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  		/* XXX: need to turn into comparison with NULL */
>  		if (t == &bool_ctype && (sclass & TYPE_PTR))
>  			goto Cast;
> -		typediff = "different base types";
> -		goto Err;
> +		*typediff = "different base types";
> +		return 0;
>  	}
> -	typediff = "invalid types";
> -
> -Err:
> -	warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> -	info(expr->pos, "   expected %s", show_typename(target));
> -	info(expr->pos, "   got %s", show_typename(source));
> -	*rp = cast_to(*rp, target);
> +	*typediff = "invalid types";
>  	return 0;
> +
>  Cast:
>  	*rp = cast_to(*rp, target);
>  	return 1;
>  }
>  
> +static int compatible_assignment_types(struct expression *expr, struct symbol *target,
> +	struct expression **rp, const char *where)
> +{
> +	const char *typediff;
> +	struct symbol *source = degenerate(*rp);
> +
> +	if (!check_assignment_types(target, rp, &typediff)) {
> +		warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> +		info(expr->pos, "   expected %s", show_typename(target));
> +		info(expr->pos, "   got %s", show_typename(source));
> +		*rp = cast_to(*rp, target);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static void mark_assigned(struct expression *expr)
>  {
>  	struct symbol *sym;
> -- 
> 1.9.0.6.g037df60.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Support GCC's transparent unions

[Josh Triplett]

On Sat, Mar 01, 2014 at 11:41:39AM +0000, John Keeping wrote:
> This stops warnings in code using socket operations with a modern glibc,
> which otherwise result in warnings of the form:
> 
> 	warning: incorrect type in argument 2 (invalid types)
> 	   expected union __CONST_SOCKADDR_ARG [usertype] __addr
> 	   got struct sockaddr *<noident>
> 
> Since transparent unions are only applicable to function arguments, we
> create a new function to check that the types are compatible
> specifically in this context.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  evaluate.c                     | 28 +++++++++++++++++++++++++++-
>  lib.c                          |  2 --
>  lib.h                          |  1 -
>  parse.c                        |  6 ++++--
>  sparse.1                       |  8 --------
>  symbol.h                       |  3 ++-
>  validation/transparent-union.c | 25 +++++++++++++++++++++++++
>  7 files changed, 58 insertions(+), 15 deletions(-)
>  create mode 100644 validation/transparent-union.c
> 
> diff --git a/evaluate.c b/evaluate.c
> index 2e6511b..f36c6c1 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1406,6 +1406,32 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  	return 1;
>  }
>  
> +static int compatible_argument_type(struct expression *expr, struct symbol *target,
> +	struct expression **rp, const char *where)
> +{
> +	const char *typediff;
> +	struct symbol *source = degenerate(*rp);
> +	struct symbol *t;
> +	classify_type(target, &t);
> +
> +	if (t->type == SYM_UNION && t->transparent_union) {
> +		struct symbol *member;
> +		FOR_EACH_PTR(t->symbol_list, member) {
> +			if (check_assignment_types(member, rp, &typediff))
> +				return 1;
> +		} END_FOR_EACH_PTR(member);
> +	}
> +
> +	if (check_assignment_types(target, rp, &typediff))
> +		return 1;
> +
> +	warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> +	info(expr->pos, "   expected %s", show_typename(target));
> +	info(expr->pos, "   got %s", show_typename(source));
> +	*rp = cast_to(*rp, target);
> +	return 0;
> +}
> +
>  static void mark_assigned(struct expression *expr)
>  {
>  	struct symbol *sym;
> @@ -2172,7 +2198,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
>  			static char where[30];
>  			examine_symbol_type(target);
>  			sprintf(where, "argument %d", i);
> -			compatible_assignment_types(expr, target, p, where);
> +			compatible_argument_type(expr, target, p, where);
>  		}
>  
>  		i++;
> diff --git a/lib.c b/lib.c
> index bf3e91c..a52b88e 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -226,7 +226,6 @@ int Wparen_string = 0;
>  int Wptr_subtraction_blows = 0;
>  int Wreturn_void = 0;
>  int Wshadow = 0;
> -int Wtransparent_union = 0;
>  int Wtypesign = 0;
>  int Wundef = 0;
>  int Wuninitialized = 1;
> @@ -438,7 +437,6 @@ static const struct warning {
>  	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
>  	{ "return-void", &Wreturn_void },
>  	{ "shadow", &Wshadow },
> -	{ "transparent-union", &Wtransparent_union },
>  	{ "typesign", &Wtypesign },
>  	{ "undef", &Wundef },
>  	{ "uninitialized", &Wuninitialized },
> diff --git a/lib.h b/lib.h
> index f09b338..197b549 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -120,7 +120,6 @@ extern int Wparen_string;
>  extern int Wptr_subtraction_blows;
>  extern int Wreturn_void;
>  extern int Wshadow;
> -extern int Wtransparent_union;
>  extern int Wtypesign;
>  extern int Wundef;
>  extern int Wuninitialized;
> diff --git a/parse.c b/parse.c
> index 9cc5f65..bf62bdd 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1207,8 +1207,10 @@ static struct token *attribute_designated_init(struct token *token, struct symbo
>  
>  static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct decl_state *ctx)
>  {
> -	if (Wtransparent_union)
> -		warning(token->pos, "ignoring attribute __transparent_union__");
> +	if (ctx->ctype.base_type && ctx->ctype.base_type->type == SYM_UNION)
> +		ctx->ctype.base_type->transparent_union = 1;
> +	else
> +		warning(token->pos, "attribute __transparent_union__ applied to non-union type");
>  	return token;
>  }
>  
> diff --git a/sparse.1 b/sparse.1
> index cd6be26..6e79202 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -297,14 +297,6 @@ Such declarations can lead to error-prone code.
>  Sparse does not issue these warnings by default.
>  .
>  .TP
> -.B \-Wtransparent\-union
> -Warn about any declaration using the GCC extension
> -\fB__attribute__((transparent_union))\fR.
> -
> -Sparse issues these warnings by default.  To turn them off, use
> -\fB\-Wno\-transparent\-union\fR.
> -.
> -.TP
>  .B \-Wtypesign
>  Warn when converting a pointer to an integer type into a pointer to an integer
>  type with different signedness.
> diff --git a/symbol.h b/symbol.h
> index 43c165b..ccb5dcb 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -174,7 +174,8 @@ struct symbol {
>  					evaluated:1,
>  					string:1,
>  					designated_init:1,
> -					forced_arg:1;
> +					forced_arg:1,
> +					transparent_union:1;
>  			struct expression *array_size;
>  			struct ctype ctype;
>  			struct symbol_list *arguments;
> diff --git a/validation/transparent-union.c b/validation/transparent-union.c
> new file mode 100644
> index 0000000..149c7d9
> --- /dev/null
> +++ b/validation/transparent-union.c
> @@ -0,0 +1,25 @@
> +struct a {
> +	int field;
> +};
> +struct b {
> +	int field;
> +};
> +
> +typedef union {
> +	struct a *a;
> +	struct b *b;
> +} transparent_arg __attribute__((__transparent_union__));
> +
> +static void foo(transparent_arg arg)
> +{
> +}
> +
> +static void bar(void)
> +{
> +	struct b arg = { 0 };
> +	foo((struct a *) &arg);
> +}
> +
> +/*
> + * check-name: Transparent union attribute.
> + */
> -- 
> 1.9.0.6.g037df60.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Support GCC's transparent unions

[Ramsay Jones]

On 01/03/14 11:41, John Keeping wrote:
> This stops warnings in code using socket operations with a modern glibc,
> which otherwise result in warnings of the form:
> 
> 	warning: incorrect type in argument 2 (invalid types)
> 	   expected union __CONST_SOCKADDR_ARG [usertype] __addr
> 	   got struct sockaddr *<noident>
> 
> Since transparent unions are only applicable to function arguments, we
> create a new function to check that the types are compatible
> specifically in this context.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  evaluate.c                     | 28 +++++++++++++++++++++++++++-
>  lib.c                          |  2 --
>  lib.h                          |  1 -
>  parse.c                        |  6 ++++--
>  sparse.1                       |  8 --------
>  symbol.h                       |  3 ++-
>  validation/transparent-union.c | 25 +++++++++++++++++++++++++
>  7 files changed, 58 insertions(+), 15 deletions(-)
>  create mode 100644 validation/transparent-union.c


I applied these patches to 'sparse 0.5.0' (there is a simple textual
conflict with commit 2667c2d in chrisl repo) and tested them on the
git.git repo on Linux. They worked just fine and sparse only issues
a single warning on Linux now. Yay! :-D

I haven't tested yet, but I expect this to also fix two similar
warnings on Cygwin (relating to the wait() 'syscall').

So, FWIW:

    Tested-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

[I started to write a patch for this years ago, but just didn't finish
it. Yours looks *much* better BTW]

HTH

ATB,
Ramsay Jones


> diff --git a/evaluate.c b/evaluate.c
> index 2e6511b..f36c6c1 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1406,6 +1406,32 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>  	return 1;
>  }
>  
> +static int compatible_argument_type(struct expression *expr, struct symbol *target,
> +	struct expression **rp, const char *where)
> +{
> +	const char *typediff;
> +	struct symbol *source = degenerate(*rp);
> +	struct symbol *t;
> +	classify_type(target, &t);
> +
> +	if (t->type == SYM_UNION && t->transparent_union) {
> +		struct symbol *member;
> +		FOR_EACH_PTR(t->symbol_list, member) {
> +			if (check_assignment_types(member, rp, &typediff))
> +				return 1;
> +		} END_FOR_EACH_PTR(member);
> +	}
> +
> +	if (check_assignment_types(target, rp, &typediff))
> +		return 1;
> +
> +	warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> +	info(expr->pos, "   expected %s", show_typename(target));
> +	info(expr->pos, "   got %s", show_typename(source));
> +	*rp = cast_to(*rp, target);
> +	return 0;
> +}
> +
>  static void mark_assigned(struct expression *expr)
>  {
>  	struct symbol *sym;
> @@ -2172,7 +2198,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
>  			static char where[30];
>  			examine_symbol_type(target);
>  			sprintf(where, "argument %d", i);
> -			compatible_assignment_types(expr, target, p, where);
> +			compatible_argument_type(expr, target, p, where);
>  		}
>  
>  		i++;
> diff --git a/lib.c b/lib.c
> index bf3e91c..a52b88e 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -226,7 +226,6 @@ int Wparen_string = 0;
>  int Wptr_subtraction_blows = 0;
>  int Wreturn_void = 0;
>  int Wshadow = 0;
> -int Wtransparent_union = 0;
>  int Wtypesign = 0;
>  int Wundef = 0;
>  int Wuninitialized = 1;
> @@ -438,7 +437,6 @@ static const struct warning {
>  	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
>  	{ "return-void", &Wreturn_void },
>  	{ "shadow", &Wshadow },
> -	{ "transparent-union", &Wtransparent_union },
>  	{ "typesign", &Wtypesign },
>  	{ "undef", &Wundef },
>  	{ "uninitialized", &Wuninitialized },
> diff --git a/lib.h b/lib.h
> index f09b338..197b549 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -120,7 +120,6 @@ extern int Wparen_string;
>  extern int Wptr_subtraction_blows;
>  extern int Wreturn_void;
>  extern int Wshadow;
> -extern int Wtransparent_union;
>  extern int Wtypesign;
>  extern int Wundef;
>  extern int Wuninitialized;
> diff --git a/parse.c b/parse.c
> index 9cc5f65..bf62bdd 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1207,8 +1207,10 @@ static struct token *attribute_designated_init(struct token *token, struct symbo
>  
>  static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct decl_state *ctx)
>  {
> -	if (Wtransparent_union)
> -		warning(token->pos, "ignoring attribute __transparent_union__");
> +	if (ctx->ctype.base_type && ctx->ctype.base_type->type == SYM_UNION)
> +		ctx->ctype.base_type->transparent_union = 1;
> +	else
> +		warning(token->pos, "attribute __transparent_union__ applied to non-union type");
>  	return token;
>  }
>  
> diff --git a/sparse.1 b/sparse.1
> index cd6be26..6e79202 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -297,14 +297,6 @@ Such declarations can lead to error-prone code.
>  Sparse does not issue these warnings by default.
>  .
>  .TP
> -.B \-Wtransparent\-union
> -Warn about any declaration using the GCC extension
> -\fB__attribute__((transparent_union))\fR.
> -
> -Sparse issues these warnings by default.  To turn them off, use
> -\fB\-Wno\-transparent\-union\fR.
> -.
> -.TP
>  .B \-Wtypesign
>  Warn when converting a pointer to an integer type into a pointer to an integer
>  type with different signedness.
> diff --git a/symbol.h b/symbol.h
> index 43c165b..ccb5dcb 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -174,7 +174,8 @@ struct symbol {
>  					evaluated:1,
>  					string:1,
>  					designated_init:1,
> -					forced_arg:1;
> +					forced_arg:1,
> +					transparent_union:1;
>  			struct expression *array_size;
>  			struct ctype ctype;
>  			struct symbol_list *arguments;
> diff --git a/validation/transparent-union.c b/validation/transparent-union.c
> new file mode 100644
> index 0000000..149c7d9
> --- /dev/null
> +++ b/validation/transparent-union.c
> @@ -0,0 +1,25 @@
> +struct a {
> +	int field;
> +};
> +struct b {
> +	int field;
> +};
> +
> +typedef union {
> +	struct a *a;
> +	struct b *b;
> +} transparent_arg __attribute__((__transparent_union__));
> +
> +static void foo(transparent_arg arg)
> +{
> +}
> +
> +static void bar(void)
> +{
> +	struct b arg = { 0 };
> +	foo((struct a *) &arg);
> +}
> +
> +/*
> + * check-name: Transparent union attribute.
> + */
> 

Re: [PATCH 1/2] evaluate: split out implementation of compatible_assignment_types

[Christopher Li]

On Sat, Mar 1, 2014 at 3:41 AM, John Keeping <john@keeping.me.uk> wrote:
> This will allow us to reuse the logic when processing a transparent
> union by checking each member in turn without printing a warning unless
> none of the members match.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>

The series looks fine on my first casual look.

Will get back to you on that.

Chris

Re: [PATCH 2/2] Support GCC's transparent unions

[Christopher Li]

On Sat, Mar 1, 2014 at 3:41 AM, John Keeping <john@keeping.me.uk> wrote:
> This stops warnings in code using socket operations with a modern glibc,
> which otherwise result in warnings of the form:
>
>         warning: incorrect type in argument 2 (invalid types)
>            expected union __CONST_SOCKADDR_ARG [usertype] __addr
>            got struct sockaddr *<noident>
>
> Since transparent unions are only applicable to function arguments, we
> create a new function to check that the types are compatible
> specifically in this context.

Can you please keep the option to warning about the transparent union?
You can change the default to off. While you are there, please make the second
patch base on the chrisl sparse repository.

> +static int compatible_argument_type(struct expression *expr, struct symbol *target,
> +       struct expression **rp, const char *where)
> +{
> +       const char *typediff;
> +       struct symbol *source = degenerate(*rp);
> +       struct symbol *t;
> +       classify_type(target, &t);
> +
> +       if (t->type == SYM_UNION && t->transparent_union) {
> +               struct symbol *member;
> +               FOR_EACH_PTR(t->symbol_list, member) {
> +                       if (check_assignment_types(member, rp, &typediff))
> +                               return 1;
> +               } END_FOR_EACH_PTR(member);
> +       }
> +
> +       if (check_assignment_types(target, rp, &typediff))
> +               return 1;
> +
> +       warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> +       info(expr->pos, "   expected %s", show_typename(target));
> +       info(expr->pos, "   got %s", show_typename(source));
> +       *rp = cast_to(*rp, target);
> +       return 0;
> +}

I found this compatible_argument_type() hard to read. There are code
copy/paste from the function compatible_assignment_types().

I think a better way  is:
static int compatible_argument_type(...)
{
     struct symbol *t;
     classify_type(target, &t);

     if (t->type == SYM_UNION && t->transparent_union)
            return compatible_assignment_transparent_union(...);
     return compatible_assignment_types(...);
}

Then you just need to complete the function
compatible_assignment_transparent_union().
Call compatible_assignment_types() if needed.

Chris

Re: [PATCH 2/2] Support GCC's transparent unions

[John Keeping]

On Tue, Mar 04, 2014 at 09:21:50AM -0800, Christopher Li wrote:
> On Sat, Mar 1, 2014 at 3:41 AM, John Keeping <john@keeping.me.uk> wrote:
> > This stops warnings in code using socket operations with a modern glibc,
> > which otherwise result in warnings of the form:
> >
> >         warning: incorrect type in argument 2 (invalid types)
> >            expected union __CONST_SOCKADDR_ARG [usertype] __addr
> >            got struct sockaddr *<noident>
> >
> > Since transparent unions are only applicable to function arguments, we
> > create a new function to check that the types are compatible
> > specifically in this context.
> 
> Can you please keep the option to warning about the transparent union?
> You can change the default to off. While you are there, please make the second
> patch base on the chrisl sparse repository.

Will do.

> > +static int compatible_argument_type(struct expression *expr, struct symbol *target,
> > +       struct expression **rp, const char *where)
> > +{
> > +       const char *typediff;
> > +       struct symbol *source = degenerate(*rp);
> > +       struct symbol *t;
> > +       classify_type(target, &t);
> > +
> > +       if (t->type == SYM_UNION && t->transparent_union) {
> > +               struct symbol *member;
> > +               FOR_EACH_PTR(t->symbol_list, member) {
> > +                       if (check_assignment_types(member, rp, &typediff))
> > +                               return 1;
> > +               } END_FOR_EACH_PTR(member);
> > +       }
> > +
> > +       if (check_assignment_types(target, rp, &typediff))
> > +               return 1;
> > +
> > +       warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> > +       info(expr->pos, "   expected %s", show_typename(target));
> > +       info(expr->pos, "   got %s", show_typename(source));
> > +       *rp = cast_to(*rp, target);
> > +       return 0;
> > +}
> 
> I found this compatible_argument_type() hard to read. There are code
> copy/paste from the function compatible_assignment_types().
> 
> I think a better way  is:
> static int compatible_argument_type(...)
> {
>      struct symbol *t;
>      classify_type(target, &t);
> 
>      if (t->type == SYM_UNION && t->transparent_union)
>             return compatible_assignment_transparent_union(...);
>      return compatible_assignment_types(...);
> }
> 
> Then you just need to complete the function
> compatible_assignment_transparent_union().
> Call compatible_assignment_types() if needed.

I'm not sure I understand what you mean here.  If I extract
compatible_assignment_transparent_union() then it is essentially the
same as compatible_argument_type() but without the check for
t->transparent_union.

Looking again, I can see that my implementation above is unnecessarily
complicated because the warning() block is identical to that in
compatible_assignment_types() and there's no way for typediff to escape
from the transparent_union look, so the last 8 lines can be replaced by:

    return compatible_assignment_types(target, rp, &typediff);

That also allows us to get rid of 'source', so we end up with:

static int compatible_argument_type(struct expression *expr, struct symbol *target,
       struct expression **rp, const char *where)
{
	struct symbol *t;
	classify_type(target, &t);

	if (t->type == SYM_UNION && t->transparent_union) {
		const char *typediff;
		struct symbol *member;
		FOR_EACH_PTR(t->symbol_list, member) {
			if (check_assignment_types(member, rp, &typediff))
				return 1;
		} END_FOR_EACH_PTR(member);
	}

	return compatible_assignment_types(expr, target, rp, where);
}


I'm not sure moving the contents of the if block into a separate
function improves things much at that point.  What do you think?

Re: [PATCH 2/2] Support GCC's transparent unions

[Christopher Li]

On Tue, Mar 4, 2014 at 9:52 AM, John Keeping <john@keeping.me.uk> wrote:
> Looking again, I can see that my implementation above is unnecessarily
> complicated because the warning() block is identical to that in
> compatible_assignment_types() and there's no way for typediff to escape
> from the transparent_union look, so the last 8 lines can be replaced by:
>
>     return compatible_assignment_types(target, rp, &typediff);
>
> That also allows us to get rid of 'source', so we end up with:
>
> static int compatible_argument_type(struct expression *expr, struct symbol *target,
>        struct expression **rp, const char *where)
> {
>         struct symbol *t;
>         classify_type(target, &t);
>
>         if (t->type == SYM_UNION && t->transparent_union) {
>                 const char *typediff;
>                 struct symbol *member;
>                 FOR_EACH_PTR(t->symbol_list, member) {
>                         if (check_assignment_types(member, rp, &typediff))
>                                 return 1;
>                 } END_FOR_EACH_PTR(member);
>         }
>
>         return compatible_assignment_types(expr, target, rp, where);
> }
>
>
> I'm not sure moving the contents of the if block into a separate
> function improves things much at that point.  What do you think?

That is better, if you submit code like that, I will not reject it.
The difference is relatively minor,  fall into the personal preference category.

Let me explain why I suggest that way.

The if statement for transparent union is a very rare and special case.
In other world, it is the cold and slow path.

People reading the compatible_argument_type() functions will most likely
have the normal assignment check in mind. It will go straight to the last
return statement. That is the hot and fast path.

And the implementation of transparent union aka the slow path is a lot
more complicate than the fast path. It is justifiable to move them into a
separate function. It also help the compiler inline compatible_argument_type()
if the slow path end up too big and complicate to inline.

From the code reading point of view, I found it helps to clearly separate the
simple hot path and the complicate slow path.

Again, it is your call. I will not enforce it.

Chris

Re: [PATCH 2/2] Support GCC's transparent unions

[Christopher Li]

On Tue, Mar 4, 2014 at 9:52 AM, John Keeping <john@keeping.me.uk> wrote:
>> Can you please keep the option to warning about the transparent union?
>> You can change the default to off. While you are there, please make the second
>> patch base on the chrisl sparse repository.
>
> Will do.
>

Ping. Any update?

I am still waiting on your patch.

Chris