Patchwork fix memory spaces and references for C

login
register
mail settings
Submitter Mike Stump
Date May 23, 2013, 5:24 p.m.
Message ID <C8631A23-E710-4CB9-A2EC-C9805610E71F@comcast.net>
Download mbox | patch
Permalink /patch/245985/
State New
Headers show

Comments

Mike Stump - May 23, 2013, 5:24 p.m.
So, memory spaces and references are interacting badly in C.  The standard allows conversions during assignment that can change qualifiers.  The good news, all that code is already written and appears to work just fine.  The sad part, we don't use it.  The code that needs fixing is in convert_for_assignment:

   /* A type converts to a reference to it.                                                               
       This code doesn't fully support references, it's just for the                                       
       special case of va_start and va_copy.  */
    if (codel == REFERENCE_TYPE
        && comptypes (TREE_TYPE (type),
                      TREE_TYPE (rhs)) == 1)

This doesn't work, as the memory space qualifiers disqualify the two types from being compatible (this is correct and matches the standard).  Instead, we expand the conditional to include all cases we are prepared to handle:

	if (codel == REFERENCE_TYPE && coder != REFERENCE_TYPE)

and then, in the body, we use convert_for_assignment to handle the conversion, as it already does everything.

This is a smaller patch than maybe it should be.  Arguably not recursing is a better approach, but then we need to split into two functions, so that I can add the REFERENCE_TYPE back to the top.  Let me know if you prefer it split.

A user actually hit this in rather trivial code with memory spaces.

Tested on two platforms, one with memory spaces and one without.

Ok?
------------------------------
Joseph S. Myers - May 23, 2013, 9:20 p.m.
On Thu, 23 May 2013, Mike Stump wrote:

> This is a smaller patch than maybe it should be.  Arguably not recursing 
> is a better approach, but then we need to split into two functions, so 
> that I can add the REFERENCE_TYPE back to the top.  Let me know if you 
> prefer it split.
> 
> A user actually hit this in rather trivial code with memory spaces.
> 
> Tested on two platforms, one with memory spaces and one without.
> 
> Ok?

This patch is OK.

Patch

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index fe6d1f6..40ccf58 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -5294,11 +5294,9 @@  convert_for_assignment (location_t location, tree type, tree rhs,
   rhs = require_complete_type (rhs);
   if (rhs == error_mark_node)
     return error_mark_node;
-  /* A type converts to a reference to it.
-     This code doesn't fully support references, it's just for the
-     special case of va_start and va_copy.  */
-  if (codel == REFERENCE_TYPE
-      && comptypes (TREE_TYPE (type), TREE_TYPE (rhs)) == 1)
+  /* A non-reference type can convert to a reference.  This handles
+     va_start, va_copy and possibly port built-ins.  */
+  if (codel == REFERENCE_TYPE && coder != REFERENCE_TYPE)
     {
       if (!lvalue_p (rhs))
 	{
@@ -5310,16 +5308,11 @@  convert_for_assignment (location_t location, tree type, tree rhs,
       rhs = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (rhs)), rhs);
       SET_EXPR_LOCATION (rhs, location);
 
-      /* We already know that these two types are compatible, but they
-	 may not be exactly identical.  In fact, `TREE_TYPE (type)' is
-	 likely to be __builtin_va_list and `TREE_TYPE (rhs)' is
-	 likely to be va_list, a typedef to __builtin_va_list, which
-	 is different enough that it will cause problems later.  */
-      if (TREE_TYPE (TREE_TYPE (rhs)) != TREE_TYPE (type))
-	{
-	  rhs = build1 (NOP_EXPR, build_pointer_type (TREE_TYPE (type)), rhs);
-	  SET_EXPR_LOCATION (rhs, location);
-	}
+      rhs = convert_for_assignment (location, build_pointer_type (TREE_TYPE (type)),
+				    rhs, origtype, errtype, null_pointer_constant,
+				    fundecl, function, parmnum);
+      if (rhs == error_mark_node)
+	return error_mark_node;
 
       rhs = build1 (NOP_EXPR, type, rhs);
       SET_EXPR_LOCATION (rhs, location);