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?
Johann
