On Mon, Mar 11, 2013 at 5:30 PM, Georg-Johann Lay <a...@gjlay.de> wrote:
> Richard Biener wrote:
>> On Fri, Mar 8, 2013 at 5:03 PM, Georg-Johann Lay <a...@gjlay.de> wrote:
>>> Richard Biener wrote:
>>>> On Fri, Mar 8, 2013 at 3:19 PM, Richard Biener
>>>> <richard.guent...@gmail.com> wrote:
>>>>> On Fri, Mar 8, 2013 at 2:57 PM, Georg-Johann Lay wrote:
>>>>>> While implementing PR56263 (Strict address-space checking for AVR), I
>>>>>> encountered the problem that pointer casts with address spaces are always
>>>>>> expanded as const_int 0.
>>>>>>
>>>>>> The problem occurs if the attached patch that implements PR56263 and the
>>>>>> following code is compiled as
>>>>>>
>>>>>> $ avr-gcc -Os flash-cast.c -S -mstrict-addr-space-subsets
>>>>>>
>>>>>>
>>>>>> #define PROGMEM __attribute__((__progmem__))
>>>>>>
>>>>>> #define PSTR(s) \
>>>>>> (__extension__ \
>>>>>> ({ \
>>>>>> static const char __c[] PROGMEM = (s); \
>>>>>> &__c[0]; \
>>>>>> }))
>>>>>>
>>>>>> extern void print (const __memx char*, ...);
>>>>>>
>>>>>> const __flash char *p;
>>>>>>
>>>>>> void f (const char *c)
>>>>>> {
>>>>>> c = (const char*) p;
>>>>>>
>>>>>> print ((const __flash char*) PSTR ("Hallo flash"));
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>> The corresponding .expand dump reads:
>>>>>>
>>>>>>
>>>>>> f (const char * c)
>>>>>> {
>>>>>> static const char __c[12] = "Hallo flash";
>>>>>> const <address-space-1> char * _2;
>>>>>> const <address-space-7> char * _3;
>>>>>>
>>>>>> ;; basic block 2, loop depth 0
>>>>>> ;; pred: ENTRY
>>>>>> _2 = (const <address-space-1> char *) (&__c[0]);
>>>>>> _3 = (const <address-space-7> char *) _2;
>>>>>> print (_3); [tail call]
>>>>>> return;
>>>>>> ;; succ: EXIT
>>>>>>
>>>>>> }
>>>>>>
>>>>>>
>>>>>> but the expression is always emit as (const_int 0).
>>>>>>
>>>>>>
>>>>>> Trying to track this issue, I ended up in build1_stat which enters the
>>>>>> default
>>>>>> case for "code" (ADDR_SPACE_CONVERT_EXPR at that time) and returns the
>>>>>> following tree:
>>>>>>
>>>>>>
>>>>>> (gdb) p t
>>>>>> $62 = (tree) 0xb7bb4578
>>>>>> (gdb) pt
>>>>>> <addr_space_convert_expr 0xb7bb4578
>>>>>> type <pointer_type 0xb7bc3a80
>>>>>> type <integer_type 0xb7bc3a20 char readonly sizes-gimplified
>>>>>> address-space-1 string-flag QI
>>>>>> size <integer_cst 0xb7b3a1a4 constant 8>
>>>>>> unit size <integer_cst 0xb7b3a1b8 constant 1>
>>>>>> align 8 symtab 0 alias set -1 canonical type 0xb7bc3a20
>>>>>> precision 8
>>>>>> min <integer_cst 0xb7b3a1e0 -128> max <integer_cst 0xb7b3a208 127>
>>>>>> pointer_to_this <pointer_type 0xb7bc3a80>>
>>>>>> unsigned HI
>>>>>> size <integer_cst 0xb7b3a08c constant 16>
>>>>>> unit size <integer_cst 0xb7b3a0a0 constant 2>
>>>>>> align 8 symtab 0 alias set -1 canonical type 0xb7bc3a80>
>>>>>> readonly constant
>>>>>> arg 0 <addr_expr 0xb7bb449c
>>>>>> type <pointer_type 0xb7b4f2a0 type <integer_type 0xb7b4f240 char>
>>>>>> public unsigned HI size <integer_cst 0xb7b3a08c 16> unit size
>>>>>> <integer_cst 0xb7b3a0a0 2>
>>>>>> align 8 symtab 0 alias set -1 canonical type 0xb7b4f2a0
>>>>>> pointer_to_this <pointer_type 0xb7b4f840>>
>>>>>> readonly constant
>>>>>> arg 0 <array_ref 0xb7b465a0 type <integer_type 0xb7b4f240 char>
>>>>>> readonly arg 0 <var_decl 0xb7bcd05c __c>
>>>>>> arg 1 <integer_cst 0xb7b3a460 constant 0>
>>>>>> flash-cast.c:18:128>
>>>>>> flash-cast.c:18:124>>
>>>>>>
>>>>>> Problem is the arg 1 <integer_cst 0xb7b3a460 constant 0> at the end
>>>>>> which leads
>>>>>> to the expansion of 0.
>>>>>>
>>>>>> The call chain is:
>>>>>>
>>>>>> #0 build1_stat (code=ADDR_SPACE_CONVERT_EXPR, type=0xb7bc3a80,
>>>>>> node=0xb7bb449c) at ../../../gcc.gnu.org/trunk/gcc/tree.c:3848
>>>>>> (gdb) bt
>>>>>> #0 build1_stat (code=ADDR_SPACE_CONVERT_EXPR, type=0xb7bc3a80,
>>>>>> node=0xb7bb449c) at ../../../gcc.gnu.org/trunk/gcc/tree.c:3848
>>>>>> #1 0x08286be0 in gimple_assign_rhs_to_tree (stmt=0xb7bc2c30) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/cfgexpand.c:86
>>>>>> #2 0x0837047f in expand_expr_real_1 (exp=0xb7b972f8, target=0x0,
>>>>>> tmode=VOIDmode, modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:9274
>>>>>> #3 0x083762f2 in expand_expr_real (exp=0xb7b972f8, target=0x0,
>>>>>> tmode=VOIDmode,
>>>>>> modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:7863
>>>>>> #4 0x08376a7e in expand_expr (exp=0xb7b972f8, target=0x0, mode=VOIDmode,
>>>>>> modifier=EXPAND_STACK_PARM) at ../../../gcc.gnu.org/trunk/gcc/expr.h:444
>>>>>> #5 0x0836ae96 in expand_expr_real_2 (ops=0xbfffca10, target=0x0,
>>>>>> tmode=VOIDmode, modifier=EXPAND_STACK_PARM) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:8150
>>>>>> #6 0x08376234 in expand_expr_real_1 (exp=0xb7bb4564, target=0x0,
>>>>>> tmode=VOIDmode, modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:10491
>>>>>> #7 0x083762f2 in expand_expr_real (exp=0xb7bb4564, target=0x0,
>>>>>> tmode=VOIDmode,
>>>>>> modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:7863
>>>>>> #8 0x083704a6 in expand_expr_real_1 (exp=0xb7b97320, target=0x0,
>>>>>> tmode=VOIDmode, modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:9274
>>>>>> #9 0x083762f2 in expand_expr_real (exp=0xb7b97320, target=0x0,
>>>>>> tmode=VOIDmode,
>>>>>> modifier=EXPAND_STACK_PARM, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:7863
>>>>>> #10 0x0826ae1a in expand_expr (exp=0xb7b97320, target=0x0, mode=VOIDmode,
>>>>>> modifier=EXPAND_STACK_PARM) at ../../../gcc.gnu.org/trunk/gcc/expr.h:444
>>>>>> #11 0x0826c215 in store_one_arg (arg=0xbfffd130, argblock=0x0, flags=0,
>>>>>> variable_size=0, reg_parm_stack_space=0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/calls.c:4500
>>>>>> #12 0x08274d79 in expand_call (exp=0xb7b46640, target=0x0, ignore=1) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/calls.c:3040
>>>>>> #13 0x08375088 in expand_expr_real_1 (exp=0xb7b46640, target=0x0,
>>>>>> tmode=VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/expr.c:10209
>>>>>> #14 0x0828cb86 in expand_call_stmt (stmt=0xb7fde140) at
>>>>>> ../../../gcc.gnu.org/trunk/gcc/cfgexpand.c:2114
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>
>>>>>> The problem comes apparent in #5 expand_expr_real_2
>>>>>>
>>>>>> if (targetm.addr_space.subset_p (as_to, as_from)
>>>>>> || targetm.addr_space.subset_p (as_from, as_to))
>>>>>> {
>>>>>> op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
>>>>>> op0 = targetm.addr_space.convert (op0, treeop0_type, type);
>>>>>> gcc_assert (op0);
>>>>>> return op0;
>>>>>> }
>>>>>>
>>>>>> where op0 is const0_rtx and targetm.addr_space.convert cannot recover
>>>>>> from that.
>>>>>>
>>>>>> Maybe someone can help me fixing the root cause?
>>>>> Doesn't make sense - in the above code treeop0 should be the
>>>>>
>>>>> arg 0 <addr_expr 0xb7bb449c
>>>>>
>>>>> not the zero integer constant.
>>>> The first conversion expands to
>>>>
>>>> (symbol_ref:HI ("__c.1466") [flags 0x202] <var_decl 0x7ffff6dfaa18 __c>)
>>>>
>>>> then we try to convert that to another address-space and the target hook
>>>> returns
>>>>
>>>> (insn 6 0 7 (set (reg/f:HI 46)
>>>> (symbol_ref:HI ("__c.1466") [flags 0x202] <var_decl
>>>> 0x7ffff6dfaa18 __c>)) t.c:18 -1
>>>> (nil))
>>>>
>>>> (insn 7 6 0 (set (reg:PSI 45)
>>>> (zero_extend:PSI (reg/f:HI 46))) t.c:18 -1
>>>> (nil))
>>>>
>>>> (reg:PSI 45)
>>>>
>>>> so the issue must be elsewhere (in case the above is correct).
>>> This looks correct. Without the patch you'll have always
>>> -mno-strict-addr-space-subsets where the code is good.
>>>
>>>> -mstrict-addr-space-subsets
>>>>
>>>> doesn't exist for me.
>>> You must apply the patch "strict-addr-spaces.diff" from the initial mail.
>>> This
>>> adds the -mstrict-addr-space-subsets option so the user can pick strictness
>>> of
>>> address space checking (PR56263).
>>>
>>> For a quick change you can change avr.c from
>>>
>>> static bool
>>> avr_addr_space_subset_p (addr_space_t subset ATTRIBUTE_UNUSED,
>>> addr_space_t superset ATTRIBUTE_UNUSED)
>>> {
>>> return true;
>>> }
>>>
>>> to
>>>
>>> static bool
>>> avr_addr_space_subset_p (addr_space_t subset ATTRIBUTE_UNUSED,
>>> addr_space_t superset)
>>> {
>>> return superset == ADDR_SPACE_MEMX;
>>> }
>>>
>>>
>>> The C code contains 2 casts: The first casts a pointer to generic space in
>>> PSTR ("Hallo world") to a 16-bit __flash pointer. The second cast is to a
>>> 24-bit __memx pointer which is for the print() prototype.
>>>
>>> PSTR is a common idiom in avr applications to put a string literal into
>>> flash
>>> (progmem) memory.
>>>
>>
>> Well, then you hit undefined behavior:
>>
>> /* Ask target code to handle conversion between pointers
>> to overlapping address spaces. */
>> if (targetm.addr_space.subset_p (as_to, as_from)
>> || targetm.addr_space.subset_p (as_from, as_to))
>> {
>> op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
>> op0 = targetm.addr_space.convert (op0, treeop0_type, type);
>> gcc_assert (op0);
>> return op0;
>> }
>>
>> /* For disjoint address spaces, converting anything but
>> a null pointer invokes undefined behaviour. We simply
>> always return a null pointer here. */
>> return CONST0_RTX (mode);
>>
>> thus your input program is not valid, or your strict addr-space
>> implementation
>> is bogus.
>>
>> Choose ;)
>
> *sigh*
>
> The point is that targetm.addr_space.convert is *not* called, at least for the
> small test case in my initial post. That is, the backend has no means to
> produce a warning. Moreover, there is no location information to use
> warning_at...
>
> Making addr-spaces disjoint was just a means to trigger the desired diagnose,
> but one more time the middle end is myopic. Why not let decide the back-end
> what to return if address spaces are no subsets?
>
> Well, how would you implement warnings on address space casts so that:
>
> - The diagnose has proper location information
> - It's purely in the back end by means of some target hooks
> - No warnings are bypassed
> - Distinguish between explicit casts by the user or implicit casts,
> e.g. calling a function with a non-matching address space pointer.
>
> A quick try shows that TARGET_CONVERT_TO_TYPE could do the job. locations look
> reasonable, but I have no idea how to test the last point with implicit or
> explicit casts.
>
> Using some type of promotion and diagnose inside the hook?
Well, one way would be to fold in the
if (targetm.addr_space.subset_p (as_to, as_from)
|| targetm.addr_space.subset_p (as_from, as_to))
tests into the addr_space.convert target hook - that is, make it
addr_space.convert
responsibility to handle the case where no conversion is possible (and return
CONST0_RTX). That way you'd have a convenient place to emit the warning.
Or emit the warning from the middle-end - if that's easily possible (I suppose
only the target knows to assign nice user-visible names to the two
address-spaces).
A quick grep shows not many targets would be affected, AVR, m32c, rl78 and spu.
You should work with the maintainers of those targets to see which approach
would be the best.
Richard.
> Johann
