aliasing problem with va_arg

aliasing problem with va_arg

[Jim Wilson]

This testcase gives incorrect code for a sparc-sun-solaris2 target when
compiled with -O2.

#include <stdarg.h>
extern double bar (double);
double foo (va_list ap) {
  double d = va_arg (ap, double);
  return bar (d);
}

The problem is that a SImode store created by the va_arg expansion is being
moved after the DFmode load of the result.  This movement comes from
DIFFERENT_ALIAS_SETS_P which sees that the two MEMs have different
MEM_ALIAS_SET values, and hence assumes that they can't overlap.  This macro
already knows that va_arg is unsafe, but it handles va_arg by checking to see
if this is a stdarg or varargs function.  That isn't sufficient, because we
can call va_arg from a function that takes a va_list parameter.  It is
also overkill since it means there is no aliasing within a stdarg/varargs
function, even though it is only MEMs from va_arg expansion that are unsafe.

Long term, I'd suggest changing how we implement va_arg.  Instead of a macro
that expands to complicated non-ISO C code, we should instead have a target
independent macro that expands to a call to a built-in function, e.g.
builtin_va_arg.  builtin_va_arg is then implemented by calling a target
dependent function that emits appropriate RTL.  When this code generates
RTL, it can set MEM_ALIAS_SET appropriately, which probably means setting it
to zero.  It would take a lot of work to convert all of the ports to a new
va_arg scheme though.  I did this once for irix6 as an experiment.  I can
send the patches if anyone is interested.  They would need a lot of work
before they could be checked in.

Short term, I am not sure what to do.  We can't easily search the parameter
list for va_list types, because that is a macro that could expand to any
type.  We could probably add some GNU C extension used to mark functions
that call va_arg, similar to the ... extension used for marking vararg
functions.  That seems like an ugly hack though.

Perhaps we can add code to dynamically enable/disable alias set computations.
E.g. add builtin functions __builtin_disable_aliasing, and
__builtin_enable_aliasing, modify va_arg to call them at the beginning and
end, and then modify get_alias_set to always return zero if
__builtin_disable_aliasing has been called.  We could then get rid of
the varargs/stdarg function checks in DIFFERENT_ALIAS_SETS_P, while
fixing va_list functions at the same time.

Jim

Re: aliasing problem with va_arg

[Mark Mitchell]

>>>>> "Jim" == Jim Wilson <wilson@cygnus.com> writes:

    Jim> hence assumes that they can't overlap.  This macro already
    Jim> knows that va_arg is unsafe, but it handles va_arg by
    Jim> checking to see if this is a stdarg or varargs function.
    Jim> That isn't sufficient, because we can call va_arg from a
    Jim> function that takes a va_list parameter.  It is also overkill

Rats, rats, rats.  Obviously, I did not think of this case.

    Jim> since it means there is no aliasing within a stdarg/varargs
    Jim> function, even though it is only MEMs from va_arg expansion
    Jim> that are unsafe.

    Jim> Long term, I'd suggest changing how we implement va_arg.
    Jim> Instead of a macro that expands to complicated non-ISO C
    Jim> code, we should instead have a target independent macro that
    Jim> expands to a call to a built-in function

Right, as per our discussion on Tuesday.  Is it really as hard as you
say to do this?  This is clearly the right fix.  For now, we don't
have to give good alias sets to the things we're loading; just 0 will
do for safety sake.

How hard would fixing the macros be?  Would you mind sending me the
preprocessed sun-solaris2 source for your example, so that I could
look at it with a cross-compiler?  (I can see va-sparc.h, but it would
be simpler for me if you would preprocess the file for me.)  I, too,
am skeptical that the macros can be fixed, but I would like to take a
look just to see.

    Jim> Perhaps we can add code to dynamically enable/disable alias
    Jim> set computations.  E.g. add builtin functions
    Jim> __builtin_disable_aliasing, and __builtin_enable_aliasing,
    Jim> modify va_arg to call them at the beginning and end, and then
    Jim> modify get_alias_set to always return zero if
    Jim> __builtin_disable_aliasing has been called.  We could then
    Jim> get rid of the varargs/stdarg function checks in
    Jim> DIFFERENT_ALIAS_SETS_P, while fixing va_list functions at the
    Jim> same time.

This seems sort-of reasonable to me, but I'd rather hold off on this
until we consider the other alternatives.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: aliasing problem with va_arg

[Jim Wilson]

	Right, as per our discussion on Tuesday.  Is it really as hard as you
	say to do this?  This is clearly the right fix.  For now, we don't
	have to give good alias sets to the things we're loading; just 0 will
	do for safety sake.

It shouldn't be hard for most targets.  Primarily it will be time consuming,
because there are 30 different ports that will need to be fixed.  I'd rather
see them all fixed than to just fix a few, because if we don't fix them
all now, we probably never will, and that could lead to long term maintenance
problems.

There are a few targets where making the change will be hard.  The MIPS
for instance supports many different macros, big endian vs little endian,
hard-float vs soft-float, 32 bit vs 64 bit, plus the effects of supporting
multiple ABIs (o32, n32, n64, EABI).  I'd expect it to get a little tricky
to get them all right.

	How hard would fixing the macros be?  Would you mind sending me the
	preprocessed sun-solaris2 source for your example, so that I could
	look at it with a cross-compiler?

I didn't even think of fixing va-sparc.h.  I just assumed it couldn't be.
It is possible that we might be able to do something there.

FYI It is possible to preprocess this with a cross-compiler.  I did
"make xgcc cpp cc1 stmp-int-hdrs" and then "./xgcc -B./ tmp.c -E > tmp.i".
I don't bother to build or use a native compiler while looking at this problem.

I thought of another idea, add a new type qualifier, e.g. __alias, __norestrct,
which has the effect of making a type that can alias any other type.  We
could then make get_alias_set return zero for types with this qualifier,
and modify the va-sparc.h file to use it.

A copy of the preprocessed file follows.

Jim

# 1 "tmp.c"
# 1 "include/stdarg.h" 1 3
 




























# 1 "include/va-sparc.h" 1 3
 


 








 

typedef void * __gnuc_va_list;



 





 









# 47 "include/va-sparc.h" 3



void va_end (__gnuc_va_list);		 

 

enum __va_type_classes {
  __no_type_class = -1,
  __void_type_class,
  __integer_type_class,
  __char_type_class,
  __enumeral_type_class,
  __boolean_type_class,
  __pointer_type_class,
  __reference_type_class,
  __offset_type_class,
  __real_type_class,
  __complex_type_class,
  __function_type_class,
  __method_type_class,
  __record_type_class,
  __union_type_class,
  __array_type_class,
  __string_type_class,
  __set_type_class,
  __file_type_class,
  __lang_type_class
};




 




 




# 134 "include/va-sparc.h" 3





 

 



# 159 "include/va-sparc.h" 3



 



# 30 "include/stdarg.h" 2 3

# 168 "include/stdarg.h" 3








 
 














 










typedef __gnuc_va_list va_list;




# 240 "include/stdarg.h" 3






# 1 "tmp.c" 2

extern double bar (double);
double foo (va_list ap) {
  double d = __extension__	(*({((__builtin_classify_type (*(  double *) 0) >= __record_type_class || (__builtin_classify_type (*(  double *) 0) == __real_type_class && sizeof (  double ) == 16))	? (( ap ) = (char *)( ap ) + (((sizeof (   double  * ) + sizeof (int) - 1) / sizeof (int)) * sizeof (int)) ,	*(  double  **) (void *) ((char *)( ap ) - (((sizeof (   double  * ) + sizeof (int) - 1) / sizeof (int)) * sizeof (int)) )) : (((sizeof (   double  ) + sizeof (int) - 1) / sizeof (int)) * sizeof (int))  == 8	? ({ union {char __d[sizeof (  double )]; int __i[2];} __u;	__u.__i[0] = ((int *) (void *) ( ap ))[0];	__u.__i[1] = ((int *) (void *) ( ap ))[1];	( ap ) = (char *)( ap ) + 8;	(  double  *) (void *) __u.__d; })	: (( ap ) = (char *)( ap ) + (((sizeof (   double  ) + sizeof (int) - 1) / sizeof (int)) * sizeof (int)) ,	((  double  *) (void *) ((char *)( ap ) - (((sizeof (   double  ) + sizeof (int) - 1) / sizeof (int)) * sizeof (int)) ))));})) ;
  return bar (d);
}

Re: aliasing problem with va_arg

[Mark Mitchell]

    Mark> 	How hard would fixing the macros be?  Would you mind
    Mark> sending me the preprocessed sun-solaris2 source for your
    Mark> example, so that I could look at it with a cross-compiler?

    Jim> I didn't even think of fixing va-sparc.h.  I just assumed it
    Jim> couldn't be.  It is possible that we might be able to do
    Jim> something there.

I think I've figured out how to fix the SPARC macros, at least.  To be
ANSI/ISO-compliant, without compiler magic (like
__builtin_va_arg_incr, etc.), the macros must not access the storage
except by access through pointers of type `char*' or of the type
actually there.  So, it's easy to see what's going wrong in the SPARC
macros:

    ? ({ union {char __d[sizeof (TYPE)]; int __i[2];} __u;	\
	 __u.__i[0] = ((int *) (void *) (pvar))[0];		\
	 __u.__i[1] = ((int *) (void *) (pvar))[1];		\
	 (pvar) = (char *)(pvar) + 8;				\
	 (TYPE *) (void *) __u.__d; })				\

tries to treat `pvar' as an `int*', and indirect through it.  Thus,
the storage at `((int*) pvar)[0]' and `((int*) pvar)[1]' is treated
both as an `int' (here) and as a `double' by the client code.  But,
all this code is trying to do is treat `pvar' as a pointer to an
8-byte type.  So, why not just write:

  ((pvar) = (char*) (pvar) + 8,
   (TYPE*) (void*) ((char*) (pvar) - 8))

which is type-correct?  I think this fixes the bug, but my SPARC
assembler is rusty.  Here's the generated code for your test case:

  #include <stdarg.h>

  extern double bar (double);

  double foo (va_list ap) {
    double d = va_arg (ap, double);
    return bar (d);
  }

gets:

  foo:
	  !#PROLOGUE# 0
	  save	%sp, -112, %sp
	  !#PROLOGUE# 1
	  call	bar, 0
	  ldd	[%i0], %o0
	  ret
	  restore

instead of the (bogus):

  foo:
	  !#PROLOGUE# 0
	  save	%sp, -120, %sp
	  !#PROLOGUE# 1
	  ld	[%i0+4], %o2
	  ld	[%i0], %o3
	  st	%o2, [%fp-20]
	  ldd	[%fp-24], %o0
	  call	bar, 0
	  st	%o3, [%fp-24]
	  ret
	  restore

Note that the second version (besides being incorrect) is vastly
inferior; it dumps the arguments into the stack before loading them
back into %o0,%o01 while with my proposed change the redundant
loads/stores are eliminated.  The SH port is the other port that seems
to use this `union' trickery.  I didn't look at those macros at all.

Does this analysis seem right to you?  If so, I suggest you modify
va-sparc.h accordingly.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: aliasing problem with va_arg

[Jim Wilson]

The sparc ABI does not align double-word arguments to a double-word boundary.
The sparc ldd instruction requires double-word alignment.  Thus we can't use
an ldd instruction to access a double-word value from the argument list because
it might not be properly aligned.  In any other case, a double-word value
will be double-word aligned.

We do want to use the ldd instruction for performance reasons, so the sparc
port is careful to make sure doubles are word-aligned.  We have to handle the
case of arguments specially though.  There is code in assign_parms in
function.c that will allocate a new stack slot and copy a parameter into it
if the place where it was passed was not sufficiently aligned.
      /* If we can't trust the parm stack slot to be aligned enough
         for its ultimate type, don't use that slot after entry.
         We'll make another stack slot, if we need one.  */

For stdarg/varargs, we make this work by explicitly copying one word at a time
into a double-word aligned temporary buffer, and then copying the double-word
out of the temporary buffer.

If there was a syntax for declaring a pointer to unaligned data, we could
perhaps solve the problem by using that in the va_arg macro.

Jim

Re: aliasing problem with va_arg

[Mark Mitchell]

>>>>> "Jim" == Jim Wilson <wilson@cygnus.com> writes:

    Jim> The sparc ABI does not align double-word arguments to a
    Jim> double-word boundary.  The sparc ldd instruction requires
    Jim> double-word alignment.  Thus we can't use an ldd instruction

Thanks.  That explains why the macro was doing what it was.  One other
ANSI C solution to this problem is to use `memcpy'.  Obviously, the
performance of even the builtin memcpy may not be ideal, especially if
it can't figure out that the storage will be word-aligned.

But, the short answer will be that this oddity of the SPARC ABI makes
efficient va_arg macros that are compliant ISO C just about
impossible; you can't access the memory with the type that it really
has, so you can only use `char'.  I suppose inline assembly is another
option.

I suggest that you proceed with your partially implemented
__builtin_va_arg support.  I think that your assertion that all 30
ports will have to be changed is false; only those with non-compliant
va_arg macros need to be changed.  It doesn't look to me like the MIPS
port does the same kind of trickery as the SPARC port, so I don't see
(at first glance) that it's va_arg macros will cause problems.

It looks like the PA port also does something odd with va_arg
involving treating a `char*' as an `int*' so it too probably needs
fixing.  I suggest that we finish up the machine-independent builtin
support, which seems better than doing other weird hacks because it
can't really be that hard and is something we know we want to do, and
then provide the machine-dependent implementations for it on the PA
and SPARC ports, if the PA macros cannot be fixed.

At that point we can also remove the bits in alias.c that disable
alias analysis for varargs functions.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: aliasing problem with va_arg

[Jim Wilson]

	I suggest that you proceed with your partially implemented
	__builtin_va_arg support.

I am a relatively inactive egcs developer at this time.  If I can't do
something in a few hours, it isn't worthwhile for me to start, as I won't
be able to finish it.  I am just tossing this out as an idea, in case there
is someone else interested in working on this project.  It will likely be a
long time before I have a chance to work on it myself.

I've seen two good ideas on ways to fix the problem within va-sparc.h.  I am
going to try those ideas next.

Jim

Re: aliasing problem with va_arg

[Jim Wilson]

I dug out the experimental va_arg code I wrote about a year and a half ago.
It would need some cleaning up before it would be useful.

Jim

I managed to move the varargs support from the preprocessor level to the
tree->RTL generator with little trouble.  I didn't bother to try to get
all details of ANSI C right, or to gracefully handle user errors, but the
patches are good enough that I can bootstrap the irix6 gcc port.

--

Some interesting problems I ran into along the way...

va_arg takes a type as the second parameter, thus it is not possible to
handle it as an ordinary function call.  It needs explicit support in the
parser, just like sizeof does.

The stdarg va_start function can be passed a variable of any type as its
second argument.  There is no way to declare such a function, so this
requires a little special support.  The current code pretends that it is
a stdarg function with one named argument; this allows us to type check the
first argument while accepting anything for the second argument.

The type used for va_list must be target dependent.  (Some C library
functions take a va_list parameter, e.g. vfprintf.  In order for these
functions to work when called from gcc, the va_list type used by gcc must
be the same type as the va_list type used by the compiler that compiled
the C library.)  This means that varargs.h/stdarg.h can't be completely
generic.  We either need to have a target dependent fragment that defines
this type, or else have gcc define another predefined type (like __SIZE_TYPE__)
and then assume in these files that this type will be defined by the
preprocessor.  I have not tried to address this problem in these patches.

I suspect the type used for the varargs va_dcl macro is not important, but
it is something that should be watched for.  For instance, on a 64 bit target,
that has 32 bit ints, it might need to be long long instead of int.  If this
type matters, this is another part of the varargs.h file that will be target
dependent, or else require a gcc assumption.

--

These patches are for the FSF gcc development sources as of 97-03-14,
and only handle the irix6 N32 varargs/stdarg conventions.

Diffs for these generated files have not been included:
	c-gperf.h
	c-parse.c
	c-parse.h
	c-parse.y
	objc-parse.c
	objc-parse.y

diff -pr clean-ss-970314/c-decl.c tmp/c-decl.c
*** clean-ss-970314/c-decl.c	Sun Feb  2 04:13:50 1997
--- tmp/c-decl.c	Thu Mar 20 22:18:10 1997
*************** init_decl_processing ()
*** 3184,3189 ****
--- 3184,3213 ----
  						    endlink)),
  		    BUILT_IN_ALLOCA, "alloca");
    builtin_function ("__builtin_ffs", int_ftype_int, BUILT_IN_FFS, NULL_PTR);
+ 
+   builtin_function ("__builtin_varargs_start",
+ 		    build_function_type (void_type_node,
+ 					 tree_cons (NULL_TREE, ptr_type_node,
+ 						    endlink)),
+ 		    BUILT_IN_VARARGS_START, NULL_PTR);
+ 
+   /* ??? The second parameter here can actually be any type.  This will
+      have to be handled somehow.  Perhaps by not specifying the parameter
+      types here, and then checking them in expand_builtin.  Perhaps by adding
+      a special match-anything type.  This works by pretending that it is a
+      stdarg function with one named argument.  */
+   builtin_function ("__builtin_stdarg_start",
+ 		    build_function_type (void_type_node,
+ 					 tree_cons (NULL_TREE, ptr_type_node,
+ 						    NULL_TREE)),
+ 		    BUILT_IN_STDARG_START, NULL_PTR);
+ 
+   builtin_function ("__builtin_va_end",
+ 		    build_function_type (void_type_node,
+ 					 tree_cons (NULL_TREE, ptr_type_node,
+ 						    endlink)),
+ 		    BUILT_IN_VA_END, NULL_PTR);
+ 
    /* Define alloca, ffs as builtins.
       Declare _exit just to mark it as volatile.  */
    if (! flag_no_builtin && !flag_no_nonansi_builtin)
diff -pr clean-ss-970314/c-parse.gperf tmp/c-parse.gperf
*** clean-ss-970314/c-parse.gperf	Wed Oct  9 04:24:35 1996
--- tmp/c-parse.gperf	Thu Mar 20 15:52:22 1997
*************** __asm, ASM_KEYWORD, NORID
*** 21,26 ****
--- 21,27 ----
  __asm__, ASM_KEYWORD, NORID
  __attribute, ATTRIBUTE, NORID
  __attribute__, ATTRIBUTE, NORID
+ __builtin_va_arg, VA_ARG, NORID
  __complex, TYPESPEC, RID_COMPLEX
  __complex__, TYPESPEC, RID_COMPLEX
  __const, TYPE_QUAL, RID_CONST
diff -pr clean-ss-970314/c-parse.in tmp/c-parse.in
*** clean-ss-970314/c-parse.in	Tue Jan  7 13:58:08 1997
--- tmp/c-parse.in	Thu Mar 20 16:46:28 1997
*************** void yyerror ();
*** 139,145 ****
  
  /* the reserved words */
  /* SCO include files test "ASM", so use something else. */
! %token SIZEOF ENUM STRUCT UNION IF ELSE WHILE DO FOR SWITCH CASE DEFAULT
  %token BREAK CONTINUE RETURN GOTO ASM_KEYWORD TYPEOF ALIGNOF
  %token ATTRIBUTE EXTENSION LABEL
  %token REALPART IMAGPART
--- 139,145 ----
  
  /* the reserved words */
  /* SCO include files test "ASM", so use something else. */
! %token SIZEOF VA_ARG ENUM STRUCT UNION IF ELSE WHILE DO FOR SWITCH CASE DEFAULT
  %token BREAK CONTINUE RETURN GOTO ASM_KEYWORD TYPEOF ALIGNOF
  %token ATTRIBUTE EXTENSION LABEL
  %token REALPART IMAGPART
*************** unary_expr:
*** 503,508 ****
--- 503,510 ----
  		{ $$ = build_unary_op (REALPART_EXPR, $2, 0); }
  	| IMAGPART cast_expr %prec UNARY
  		{ $$ = build_unary_op (IMAGPART_EXPR, $2, 0); }
+ 	| VA_ARG '(' expr_no_commas ',' typename ')'
+ 		{ $$ = build_va_arg ($3, groktypename ($5)); }
  	;
  
  sizeof:
diff -pr clean-ss-970314/c-tree.h tmp/c-tree.h
*** clean-ss-970314/c-tree.h	Sun Jan 19 12:04:55 1997
--- tmp/c-tree.h	Thu Mar 20 15:55:36 1997
*************** extern void incomplete_type_error		PROTO
*** 323,328 ****
--- 323,329 ----
  extern tree common_type                         PROTO((tree, tree));
  extern int comptypes				PROTO((tree, tree));
  extern int self_promoting_args_p		PROTO((tree));
+ extern tree build_va_arg			PROTO((tree, tree));
  extern tree c_sizeof                            PROTO((tree));
  extern tree c_sizeof_nowarn                     PROTO((tree));
  extern tree c_size_in_bytes                     PROTO((tree));
diff -pr clean-ss-970314/c-typeck.c tmp/c-typeck.c
*** clean-ss-970314/c-typeck.c	Sun Mar  2 10:20:26 1997
--- tmp/c-typeck.c	Thu Mar 20 16:43:56 1997
*************** signed_or_unsigned_type (unsignedp, type
*** 793,798 ****
--- 793,805 ----
    return type;
  }
  
+ tree
+ build_va_arg (expr, type)
+      tree expr, type;
+ {
+   return build1 (VA_ARG_EXPR, type, expr);
+ }
+ 
  /* Compute the value of the `sizeof' operator.  */
  
  tree
diff -pr clean-ss-970314/expr.c tmp/expr.c
*** clean-ss-970314/expr.c	Sun Jan  5 04:26:02 1997
--- tmp/expr.c	Thu Mar 20 22:23:03 1997
*************** expand_expr (exp, target, tmode, modifie
*** 7262,7267 ****
--- 7262,7299 ----
  	return op0;
        return const0_rtx;
  
+     case VA_ARG_EXPR:
+       {
+ 	/* ??? This code must be target dependent.  */
+ 	/* Get AP.  */
+ 	op0 = expand_expr (TREE_OPERAND (exp, 0), NULL_RTX, VOIDmode, 0);
+ 
+ 	/* Compute new value for AP.  */
+ 	temp = expand_binop (GET_MODE (op0), add_optab, op0, GEN_INT (7),
+ 			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ 	temp = expand_binop (GET_MODE (op0), and_optab, temp, GEN_INT (-8),
+ 			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ 	temp = expand_binop (GET_MODE (op0), add_optab, temp,
+ 			     GEN_INT (((TREE_INT_CST_LOW (TYPE_SIZE (type))
+ 					/ BITS_PER_UNIT)
+ 				       + 7)
+ 				      / 8 * 8),
+ 			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ 	/* Store new value back into AP.  */
+ 	emit_move_insn (op0, temp);
+ 
+ 	/* Compute address of argument from new AP value.  */
+ 	temp = expand_binop (GET_MODE (op0), add_optab, temp,
+ 			     GEN_INT (- (TREE_INT_CST_LOW (TYPE_SIZE (type))
+ 					 / BITS_PER_UNIT)),
+ 			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ 	/* Create MEM to reference argument.  */
+ 	temp = gen_rtx (MEM, TYPE_MODE (type), temp);
+ 	MEM_IN_STRUCT_P (temp) = 1;
+ 
+ 	return temp;
+       }
+ 
      default:
        return (*lang_expand_expr) (exp, original_target, tmode, modifier);
      }
*************** expand_builtin_return_addr (fndecl_code,
*** 8022,8027 ****
--- 8054,8101 ----
    return tem;
  }
  \f
+ static rtx
+ expand_builtin_next_arg (arglist)
+      tree arglist;
+ {
+   tree fntype = TREE_TYPE (current_function_decl);
+ 
+   if ((TYPE_ARG_TYPES (fntype) == 0
+        || (TREE_VALUE (tree_last (TYPE_ARG_TYPES (fntype))) == void_type_node))
+       && ! current_function_varargs)
+     {
+       error ("`va_start' used in function with fixed args");
+       return const0_rtx;
+     }
+ 
+   if (arglist)
+     {
+       tree last_parm = tree_last (DECL_ARGUMENTS (current_function_decl));
+       tree arg = TREE_VALUE (arglist);
+ 
+       /* Strip off all nops for the sake of the comparison.  This
+ 	 is not quite the same as STRIP_NOPS.  It does more.  
+ 	 We must also strip off INDIRECT_EXPR for C++ reference
+ 	 parameters.  */
+       while (TREE_CODE (arg) == NOP_EXPR
+ 	     || TREE_CODE (arg) == CONVERT_EXPR
+ 	     || TREE_CODE (arg) == NON_LVALUE_EXPR
+ 	     || TREE_CODE (arg) == INDIRECT_REF)
+ 	arg = TREE_OPERAND (arg, 0);
+       if (arg != last_parm)
+ 	warning ("second parameter of `va_start' not last named argument");
+     }
+   else if (! current_function_varargs)
+     /* Evidently an out of date version of <stdarg.h>; can't validate
+        va_start's second argument, but can still work as intended.  */
+     warning ("`__builtin_next_arg' called without an argument");
+ 
+   return expand_binop (Pmode, add_optab,
+ 		       current_function_internal_arg_pointer,
+ 		       current_function_arg_offset_rtx,
+ 		       NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ }
+ 
  /* Expand an expression EXP that calls a built-in function,
     with result going to TARGET if that's convenient
     (and in mode MODE if that's convenient).
*************** expand_builtin (exp, target, subtarget, 
*** 8339,8383 ****
  
        /* Return the address of the first anonymous stack arg.  */
      case BUILT_IN_NEXT_ARG:
!       {
! 	tree fntype = TREE_TYPE (current_function_decl);
! 
! 	if ((TYPE_ARG_TYPES (fntype) == 0
! 	     || (TREE_VALUE (tree_last (TYPE_ARG_TYPES (fntype)))
! 		 == void_type_node))
! 	    && ! current_function_varargs)
! 	  {
! 	    error ("`va_start' used in function with fixed args");
! 	    return const0_rtx;
! 	  }
! 
! 	if (arglist)
! 	  {
! 	    tree last_parm = tree_last (DECL_ARGUMENTS (current_function_decl));
! 	    tree arg = TREE_VALUE (arglist);
! 
! 	    /* Strip off all nops for the sake of the comparison.  This
! 	       is not quite the same as STRIP_NOPS.  It does more.  
! 	       We must also strip off INDIRECT_EXPR for C++ reference
! 	       parameters.  */
! 	    while (TREE_CODE (arg) == NOP_EXPR
! 		   || TREE_CODE (arg) == CONVERT_EXPR
! 		   || TREE_CODE (arg) == NON_LVALUE_EXPR
! 		   || TREE_CODE (arg) == INDIRECT_REF)
! 	      arg = TREE_OPERAND (arg, 0);
! 	    if (arg != last_parm)
! 	      warning ("second parameter of `va_start' not last named argument");
! 	  }
! 	else if (! current_function_varargs)
! 	  /* Evidently an out of date version of <stdarg.h>; can't validate
! 	     va_start's second argument, but can still work as intended.  */
! 	  warning ("`__builtin_next_arg' called without an argument");
!       }
! 
!       return expand_binop (Pmode, add_optab,
! 			   current_function_internal_arg_pointer,
! 			   current_function_arg_offset_rtx,
! 			   NULL_RTX, 0, OPTAB_LIB_WIDEN);
  
      case BUILT_IN_CLASSIFY_TYPE:
        if (arglist != 0)
--- 8413,8419 ----
  
        /* Return the address of the first anonymous stack arg.  */
      case BUILT_IN_NEXT_ARG:
!       return expand_builtin_next_arg (arglist);
  
      case BUILT_IN_CLASSIFY_TYPE:
        if (arglist != 0)
*************** expand_builtin (exp, target, subtarget, 
*** 9094,9099 ****
--- 9130,9177 ----
  
  	return const0_rtx;
        }
+ 
+     case BUILT_IN_VARARGS_START:
+       {
+ 	/* ??? This code must be target dependent.  */
+ 	rtx temp;
+ 
+ 	/* Get AP.  */
+ 	op0 = expand_expr (TREE_VALUE (arglist), NULL_RTX, VOIDmode, 0);
+ 
+ 	/* Compute starting value for AP.  */
+ 	temp = expand_builtin_next_arg (NULL_TREE);
+ 	if (current_function_args_info.arg_words >= 8)
+ 	  temp = expand_binop (GET_MODE (temp), add_optab, temp, GEN_INT (-8),
+ 			       NULL_RTX, 0, OPTAB_LIB_WIDEN);
+ 	
+ 	/* Store starting value into AP.  */
+ 	emit_move_insn (op0, temp);
+ 
+ 	return const0_rtx;
+       }
+ 
+     case BUILT_IN_STDARG_START:
+       {
+ 	/* ??? This code must be target dependent.  */
+ 	rtx temp;
+ 
+ 	/* Get AP.  */
+ 	op0 = expand_expr (TREE_VALUE (arglist), NULL_RTX, VOIDmode, 0);
+ 
+ 	/* Compute starting value for AP.  */
+ 	temp = expand_builtin_next_arg (TREE_CHAIN (arglist));
+ 	
+ 	/* Store starting value into AP.  */
+ 	emit_move_insn (op0, temp);
+ 
+ 	return const0_rtx;
+       }
+ 
+     case BUILT_IN_VA_END:
+       /* ??? This code must be target dependent.  */
+       /* No action is necesary here.  */
+       return const0_rtx;
  
      default:			/* just do library call, if unknown builtin */
        error ("built-in function `%s' not currently supported",
diff -pr clean-ss-970314/ginclude/stdarg.h tmp/ginclude/stdarg.h
*** clean-ss-970314/ginclude/stdarg.h	Wed Dec 11 13:24:09 1996
--- tmp/ginclude/stdarg.h	Thu Mar 20 18:44:46 1997
***************
*** 1,3 ****
--- 1,18 ----
+ #if 1
+ 
+ /* ??? This type is OS (actually C library) dependent.  */
+ typedef char * __gnuc_va_list;
+ 
+ typedef __gnuc_va_list va_list;
+ 
+ #define va_start  __builtin_stdarg_start
+ 
+ #define va_arg  __builtin_va_arg
+ 
+ #define va_end  __builtin_va_end
+ 
+ #else
+ 
  /* stdarg.h for GNU.
     Note that the type used in va_arg is supposed to match the
     actual type **after default promotions**.
*************** typedef __gnuc_va_list va_list;
*** 176,178 ****
--- 191,195 ----
  
  #endif /* not _ANSI_STDARG_H_ */
  #endif /* not _STDARG_H */
+ 
+ #endif
diff -pr clean-ss-970314/ginclude/varargs.h tmp/ginclude/varargs.h
*** clean-ss-970314/ginclude/varargs.h	Sun Mar  2 14:37:21 1997
--- tmp/ginclude/varargs.h	Thu Mar 20 18:43:39 1997
***************
*** 1,3 ****
--- 1,23 ----
+ #if 1
+ 
+ /* ??? This type is OS (actually C library) dependent.  */
+ typedef char * __gnuc_va_list;
+ 
+ typedef __gnuc_va_list va_list;
+ 
+ #define va_alist __builtin_va_alist
+ 
+ /* ??? It would be nice to get rid of the ellipsis here.  */
+ #define va_dcl   int __builtin_va_alist; ...
+ 
+ #define va_start  __builtin_varargs_start
+ 
+ #define va_arg  __builtin_va_arg
+ 
+ #define va_end  __builtin_va_end
+ 
+ #else
+ 
  /* Record that this is varargs.h; this turns off stdarg.h.  */
  
  #ifndef _VARARGS_H
*************** typedef __gnuc_va_list va_list;
*** 194,197 ****
--- 214,219 ----
     undefined instead of _VA_LIST_.  */
  #ifdef _BSD_VA_LIST
  #undef _BSD_VA_LIST
+ #endif
+ 
  #endif
diff -pr clean-ss-970314/tree.def tmp/tree.def
*** clean-ss-970314/tree.def	Fri Nov  8 17:30:11 1996
--- tmp/tree.def	Thu Mar 20 15:50:12 1997
*************** DEFTREECODE (PREDECREMENT_EXPR, "predecr
*** 689,694 ****
--- 689,696 ----
  DEFTREECODE (PREINCREMENT_EXPR, "preincrement_expr", "e", 2)
  DEFTREECODE (POSTDECREMENT_EXPR, "postdecrement_expr", "e", 2)
  DEFTREECODE (POSTINCREMENT_EXPR, "postincrement_expr", "e", 2)
+ 
+ DEFTREECODE (VA_ARG_EXPR, "va_arg_expr", "e", 1)
  \f
  /* These types of expressions have no useful value,
     and always have side effects.  */
diff -pr clean-ss-970314/tree.h tmp/tree.h
*** clean-ss-970314/tree.h	Sun Feb 16 02:04:51 1997
--- tmp/tree.h	Thu Mar 20 18:48:04 1997
*************** enum built_in_function
*** 100,105 ****
--- 100,109 ----
    BUILT_IN_SETJMP,
    BUILT_IN_LONGJMP,
  
+   BUILT_IN_VARARGS_START,
+   BUILT_IN_STDARG_START,
+   BUILT_IN_VA_END,
+ 
    /* C++ extensions */
    BUILT_IN_NEW,
    BUILT_IN_VEC_NEW,

Re: aliasing problem with va_arg

[Jason Merrill]

>>>>> Jim Wilson <wilson@cygnus.com> writes:

 > If there was a syntax for declaring a pointer to unaligned data, we could
 > perhaps solve the problem by using that in the va_arg macro.

Here's what I did in frame.c:

  union unaligned {
    void *p;
    unsigned b2 __attribute__ ((mode (HI)));
    unsigned b4 __attribute__ ((mode (SI)));
    unsigned b8 __attribute__ ((mode (DI)));
  } __attribute__ ((packed));
  static inline void *
  read_pointer (void *p)
  { union unaligned *up = p; return up->p; }
  ..etc..

Jason

Re: aliasing problem with va_arg

[Jim Wilson]

I finally got a chance to take a better look at the problem, and I noticed
that your aliasing code allows type punning if it sees a direct union member
reference.  However, it does not allow type punning if we have a more
complicated tree.  This test case for instance gives incorrect code for irix6
and solaris2 because none of the references are recognized as union
accesses. They are array_refs of a union field, which the current code
doesn't handle.

double
sub (int i, int k, int j)
{
  union {
    int i[4];
    double d[2];
  } u;

  u.i[2] = i;
  u.i[3] = j;
  sub2 (u.d[1]);
}

If I fix this problem, then the va_arg problem goes away.  The stores
into the temp buffer are clean accesses to a union and now get alias set 0,
which means they can alias anything.  The reads from the temp buffer are
messy, but their alias set does not matter.  This will work provided that
either all stores or all reads to the temp buffer have alias set zero, and
this is true for the sparc va_arg macro.

Does this patch look OK to check in?

It is possible that there are other problems with the union type punning
check.  What happens if we put a pointer in a union, and then indirect
through it?  Will c_get_alias_set see a COMPONENT_REF or an INDIRECT_REF
of a COMPONENT_REF?  If it see an INDIRECT_REF only, then the MEM that
reads the pointer from the union may get the wrong alias set.  I am not
going to worry about such problems unless I see them though.

Wed Oct 28 15:29:56 1998  Jim Wilson  <wilson@cygnus.com>

	* c-common.c (c_get_alias_set): Handle ARRAY_REF of union field.

Index: c-common.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gcc/c-common.c,v
retrieving revision 1.106
diff -p -r1.106 c-common.c
*** c-common.c	1998/10/09 23:02:26	1.106
--- c-common.c	1998/10/28 23:29:52
*************** c_get_alias_set (t)
*** 3016,3023 ****
         the conservative assumption.  */
      return 0;
  
!   if (TREE_CODE (t) == COMPONENT_REF
!       && TREE_CODE (TREE_TYPE (TREE_OPERAND (t, 0))) == UNION_TYPE)
      /* Permit type-punning when accessing a union, provided the
         access is directly through the union.  For example, this code does
         not permit taking the address of a union member and then
--- 3016,3029 ----
         the conservative assumption.  */
      return 0;
  
!   if ((TREE_CODE (t) == COMPONENT_REF
!        && TREE_CODE (TREE_TYPE (TREE_OPERAND (t, 0))) == UNION_TYPE)
!       /* Also permit punning when accessing an array which is a union
! 	 member.  */
!       || (TREE_CODE (t) == ARRAY_REF
! 	  && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF
! 	  && (TREE_CODE (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)))
! 	      == UNION_TYPE)))
      /* Permit type-punning when accessing a union, provided the
         access is directly through the union.  For example, this code does
         not permit taking the address of a union member and then

Re: aliasing problem with va_arg

[Mark Mitchell]

>>>>> "Jim" == Jim Wilson <wilson@cygnus.com> writes:

    Jim> I finally got a chance to take a better look at the problem,
    Jim> and I noticed that your aliasing code allows type punning if
    Jim> it sees a direct union member reference.  However, it does

Right.  All uses of type-punning are implementation-defined; I defined
them in the manual to not work except for the one explict case you
mention here.

In general, things can get arbitrarily complicated: you can take the
address of things in the union and then pun through them, etc.  I
don't see any reason to make this work, especially since it will
completely trash type-based alias analysis.

Your patch is probably OK.  I am leery, though, of trying to extend
things too far, though; much better that people learn not to use
type-punning.  It's not safe, and it's not portable.  I understand
that you want to do this to fix the va-sparc.h problem, and I'm
sympathetic.  

I still think the right thing to do is really to do __builtin_va_arg
and friends; this might be a bit of work, but if you cannot find the
time, perhaps some other interested party can do the work, or arrange
for it to be done.  As you point out, it would be nice to allow alias
analysis in varargs functions; this will require (at least) also
fixing the HP macros.  Your fix will fix the SPARC problem, and that
may be the best thing to do for now, but it is not a long-term
solution.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: aliasing problem with va_arg

[Jim Wilson]

	I am leery, though, of trying to extend
	things too far, though; much better that people learn not to use
	type-punning.  It's not safe, and it's not portable. 

There is a lot of code that has been written specifically for gcc that
has occasionally found the need for type punning.  I know that there is
type punning code in libio/libstc++.  I suspect that there is type punning
code in glibc and the linux kernel.  These codes were never meant to be
compiled by any other compiler, so portability is not a concern.

We have been telling people for a long time that gcc supports type punning for
anything inside a union.  They won't like being told that their code is
now suddenly broken, so we will need to be careful now we approach this.
We need to support existing code, and educate people about better ways to
write code before we start breaking it.

Meanwhile, I am concerned that your definition of what is a union access for
type punning purposes is so narrow that end users might not understand it.
For instance, consider this example:

union { struct { short a, b; } s; int i; } u;
short sub (int i) { u.i = i; return u.s.b; }

The access to u.s.b is not given an alias set of zero, because it is not a
union member access, it is instead a structure member access.  That seems
confusing to me.  If we allow type punning of union members, then it seems
weak not to allow type punning of union members that are structures.
As a practical matter though, I don't expect much code to break because of
this problem, because the access to u.i does get an alias set of zero, and
that is sufficient to make this example work.

This is essentially the same problem I am handling in the sparc va_arg case,
except that I have replaced the array with a structure.

	Your patch is probably OK.  I understand that you want to do this
	to fix the va-sparc.h problem, and I'm sympathetic.  

I expanded the comment to make it clear that it was specifically for the
va-sparc.h va_arg, and then checked it in.  We can remove it later if the
va_arg support is rewritten, and we decide that this kind of type punning
inside unions is not necessary.

J

Re: aliasing problem with va_arg

[Jeffrey A Law]

  In message < 199810262036.MAA12026@rtl.cygnus.com >you write:
  > 	Right, as per our discussion on Tuesday.  Is it really as hard as you
  > 	say to do this?  This is clearly the right fix.  For now, we don't
  > 	have to give good alias sets to the things we're loading; just 0 will
  > 	do for safety sake.
  > 
  > It shouldn't be hard for most targets.  Primarily it will be time consuming
  > ,
  > because there are 30 different ports that will need to be fixed.  I'd rathe
  > r
  > see them all fixed than to just fix a few, because if we don't fix them
  > all now, we probably never will, and that could lead to long term maintenan
  > ce
  > problems.
  > 
  > There are a few targets where making the change will be hard.  The MIPS
  > for instance supports many different macros, big endian vs little endian,
  > hard-float vs soft-float, 32 bit vs 64 bit, plus the effects of supporting
  > multiple ABIs (o32, n32, n64, EABI).  I'd expect it to get a little tricky
  > to get them all right.
  > 
  > 	How hard would fixing the macros be?  Would you mind sending me the
  > 	preprocessed sun-solaris2 source for your example, so that I could
  > 	look at it with a cross-compiler?
  > 
  > I didn't even think of fixing va-sparc.h.  I just assumed it couldn't be.
  > It is possible that we might be able to do something there.
One possibility would be to have the va_arg macro expand in the normal fashion,
but also emit something similar to __builtin_saveregs -- ie a marker to
indicate that we're accessing stuff on a varargs list.

When we see the marker we turn off type based alias analysis, or just generate
alias set zero for all mems.

We then have a different marker emitted by va_end which returns us to normal
behavior.

That's easier than fixing the varargs stuff to work strictly with builtins. 

We could even have the builtins be named __builtin_va_arg (ap, type) and
__builtin_va_end (ap) as a step towards actually implementing them as builtins.

jeff

Re: aliasing problem with va_arg

[Andi Kleen]

In article < 21508.909509785@hurl.cygnus.com >,
Jeffrey A Law <law@cygnus.com> writes:
> One possibility would be to have the va_arg macro expand in the normal fashion,
> but also emit something similar to __builtin_saveregs -- ie a marker to
> indicate that we're accessing stuff on a varargs list.

> When we see the marker we turn off type based alias analysis, or just generate
> alias set zero for all mems.

> We then have a different marker emitted by va_end which returns us to normal
> behavior.

> That's easier than fixing the varargs stuff to work strictly with builtins. 

> We could even have the builtins be named __builtin_va_arg (ap, type) and
> __builtin_va_end (ap) as a step towards actually implementing them as builtins.

A __norestrict pointer qualifier and an appropiate cast in the va_* macros 
looks a lot cleaner and more general purpose for me - I could imagine that 
it would be very useful to fix other macros where the maintainers don't
have the possibility to change the compiler @)

-Andi

Re: aliasing problem with va_arg

[Jeffrey A Law]

  In message < k2btmwg31j.fsf@zero.aec.at >you write:
  > A __norestrict pointer qualifier and an appropiate cast in the va_* macros 
  > looks a lot cleaner and more general purpose for me - I could imagine that 
  > it would be very useful to fix other macros where the maintainers don't
  > have the possibility to change the compiler @)
That would probably work just as well.  The only benefit of my approach is it
take a stiny step towards making the varargs/stdarg stuff a builtin instead
of disgusting macro code.  It's something we want to do at some point, it's
just not clear when it'll become important enough for someone to actually
start working on it.

jeff