* aliasing problem with va_arg @ 1998-10-23 17:45 Jim Wilson 1998-10-23 18:12 ` Mark Mitchell 0 siblings, 1 reply; 15+ messages in thread From: Jim Wilson @ 1998-10-23 17:45 UTC (permalink / raw) To: egcs; +Cc: mark, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-23 17:45 aliasing problem with va_arg Jim Wilson @ 1998-10-23 18:12 ` Mark Mitchell 1998-10-26 20:42 ` Jim Wilson 0 siblings, 1 reply; 15+ messages in thread From: Mark Mitchell @ 1998-10-23 18:12 UTC (permalink / raw) To: wilson; +Cc: egcs, wilson >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-23 18:12 ` Mark Mitchell @ 1998-10-26 20:42 ` Jim Wilson 1998-10-27 3:02 ` Mark Mitchell 1998-10-27 16:33 ` Jeffrey A Law 0 siblings, 2 replies; 15+ messages in thread From: Jim Wilson @ 1998-10-26 20:42 UTC (permalink / raw) To: mark; +Cc: egcs 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); } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-26 20:42 ` Jim Wilson @ 1998-10-27 3:02 ` Mark Mitchell 1998-10-27 14:55 ` Jim Wilson ` (2 more replies) 1998-10-27 16:33 ` Jeffrey A Law 1 sibling, 3 replies; 15+ messages in thread From: Mark Mitchell @ 1998-10-27 3:02 UTC (permalink / raw) To: wilson; +Cc: egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-27 3:02 ` Mark Mitchell @ 1998-10-27 14:55 ` Jim Wilson 1998-10-27 16:33 ` Mark Mitchell 1998-10-27 16:33 ` Jim Wilson [not found] ` <199810271907.LAA04580.cygnus.egcs@rtl.cygnus.com> 2 siblings, 1 reply; 15+ messages in thread From: Jim Wilson @ 1998-10-27 14:55 UTC (permalink / raw) To: mark; +Cc: egcs, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-27 14:55 ` Jim Wilson @ 1998-10-27 16:33 ` Mark Mitchell 1998-10-28 12:45 ` Jim Wilson 0 siblings, 1 reply; 15+ messages in thread From: Mark Mitchell @ 1998-10-27 16:33 UTC (permalink / raw) To: wilson; +Cc: egcs, wilson >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-27 16:33 ` Mark Mitchell @ 1998-10-28 12:45 ` Jim Wilson 0 siblings, 0 replies; 15+ messages in thread From: Jim Wilson @ 1998-10-28 12:45 UTC (permalink / raw) To: mark; +Cc: egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-27 3:02 ` Mark Mitchell 1998-10-27 14:55 ` Jim Wilson @ 1998-10-27 16:33 ` Jim Wilson [not found] ` <199810271907.LAA04580.cygnus.egcs@rtl.cygnus.com> 2 siblings, 0 replies; 15+ messages in thread From: Jim Wilson @ 1998-10-27 16:33 UTC (permalink / raw) To: egcs; +Cc: mark, 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, ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <199810271907.LAA04580.cygnus.egcs@rtl.cygnus.com>]
* Re: aliasing problem with va_arg [not found] ` <199810271907.LAA04580.cygnus.egcs@rtl.cygnus.com> @ 1998-10-28 1:58 ` Jason Merrill 1998-10-29 0:50 ` Jim Wilson 0 siblings, 1 reply; 15+ messages in thread From: Jason Merrill @ 1998-10-28 1:58 UTC (permalink / raw) To: Jim Wilson, egcs, Mark Mitchell >>>>> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-28 1:58 ` Jason Merrill @ 1998-10-29 0:50 ` Jim Wilson 1998-10-29 0:50 ` Mark Mitchell 0 siblings, 1 reply; 15+ messages in thread From: Jim Wilson @ 1998-10-29 0:50 UTC (permalink / raw) To: Mark Mitchell; +Cc: egcs, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-29 0:50 ` Jim Wilson @ 1998-10-29 0:50 ` Mark Mitchell 1998-10-30 2:40 ` Jim Wilson 0 siblings, 1 reply; 15+ messages in thread From: Mark Mitchell @ 1998-10-29 0:50 UTC (permalink / raw) To: wilson; +Cc: egcs, wilson >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-29 0:50 ` Mark Mitchell @ 1998-10-30 2:40 ` Jim Wilson 0 siblings, 0 replies; 15+ messages in thread From: Jim Wilson @ 1998-10-30 2:40 UTC (permalink / raw) To: mark; +Cc: egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-26 20:42 ` Jim Wilson 1998-10-27 3:02 ` Mark Mitchell @ 1998-10-27 16:33 ` Jeffrey A Law 1998-10-28 6:43 ` Andi Kleen 1 sibling, 1 reply; 15+ messages in thread From: Jeffrey A Law @ 1998-10-27 16:33 UTC (permalink / raw) To: Jim Wilson; +Cc: mark, egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-27 16:33 ` Jeffrey A Law @ 1998-10-28 6:43 ` Andi Kleen 1998-10-28 12:45 ` Jeffrey A Law 0 siblings, 1 reply; 15+ messages in thread From: Andi Kleen @ 1998-10-28 6:43 UTC (permalink / raw) To: law; +Cc: Jim Wilson, mark, egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: aliasing problem with va_arg 1998-10-28 6:43 ` Andi Kleen @ 1998-10-28 12:45 ` Jeffrey A Law 0 siblings, 0 replies; 15+ messages in thread From: Jeffrey A Law @ 1998-10-28 12:45 UTC (permalink / raw) To: Andi Kleen; +Cc: Jim Wilson, mark, egcs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~1998-10-30 2:40 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 1998-10-23 17:45 aliasing problem with va_arg Jim Wilson 1998-10-23 18:12 ` Mark Mitchell 1998-10-26 20:42 ` Jim Wilson 1998-10-27 3:02 ` Mark Mitchell 1998-10-27 14:55 ` Jim Wilson 1998-10-27 16:33 ` Mark Mitchell 1998-10-28 12:45 ` Jim Wilson 1998-10-27 16:33 ` Jim Wilson [not found] ` <199810271907.LAA04580.cygnus.egcs@rtl.cygnus.com> 1998-10-28 1:58 ` Jason Merrill 1998-10-29 0:50 ` Jim Wilson 1998-10-29 0:50 ` Mark Mitchell 1998-10-30 2:40 ` Jim Wilson 1998-10-27 16:33 ` Jeffrey A Law 1998-10-28 6:43 ` Andi Kleen 1998-10-28 12:45 ` Jeffrey A Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).