Re: ADDR_SPACE_CONVERT_EXPR always expanded to 0?

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Mar 8, 2013 at 5:03 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> Richard Biener wrote:
>> On Fri, Mar 8, 2013 at 3:19 PM, Richard Biener
>> <richard.guenther@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 ;)

Richard.

> Johann
>