This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] convert braced initializers to strings (PR 71625)


On 08/18/18 12:40, Richard Sandiford wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> Hi everybody,
>>
>> On 08/16/18 08:36, Bernd Edlinger wrote:
>>> Jeff Law wrote:
>>>> I wonder if the change to how we set up the initializers is ultimately
>>>> changing the section those go into and ultimately causing an overflow of
>>>> the .sdata section.
>>>
>>>
>>> Yes, that is definitely the case.
>>> Due to the -fmerge-all-constants option used
>>> named arrays with brace initializer look like string initializers
>>> and can go into the merge section if there are no embedded nul chars.
>>> But the string constants can now be huge.
>>>
>>> See my other patch about string merging:
>>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
>>>
>>>
>>> Can this section overflow?
>>>
>>
>>
>> could someone try out if this (untested) patch fixes the issue?
>>
>>
>> Thanks,
>> Bernd.
>>
>>
>> 2018-08-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* expmed.c (simple_mem_bitfield_p): Do shift right signed.
>> 	* config/alpha/alpha.h (CONSTANT_ADDRESS_P): Avoid signed
>> 	integer overflow.
>>
>> Index: gcc/config/alpha/alpha.h
>> ===================================================================
>> --- gcc/config/alpha/alpha.h	(revision 263611)
>> +++ gcc/config/alpha/alpha.h	(working copy)
>> @@ -678,7 +678,7 @@ enum reg_class {
>>   
>>   #define CONSTANT_ADDRESS_P(X)   \
>>     (CONST_INT_P (X)		\
>> -   && (unsigned HOST_WIDE_INT) (INTVAL (X) + 0x8000) < 0x10000)
>> +   && (UINTVAL (X) + 0x8000) < 0x10000)
>>   
>>   /* The macros REG_OK_FOR..._P assume that the arg is a REG rtx
>>      and check its validity for a certain class.
>> Index: gcc/expmed.c
>> ===================================================================
>> --- gcc/expmed.c	(revision 263611)
>> +++ gcc/expmed.c	(working copy)
>> @@ -579,8 +579,12 @@ static bool
>>   simple_mem_bitfield_p (rtx op0, poly_uint64 bitsize, poly_uint64 bitnum,
>>   		       machine_mode mode, poly_uint64 *bytenum)
>>   {
>> +  poly_int64 ibit = bitnum;
>> +  poly_int64 ibyte;
>> +  if (!multiple_p (ibit, BITS_PER_UNIT, &ibyte))
>> +    return false;
>> +  *bytenum = ibyte;
>>     return (MEM_P (op0)
>> -	  && multiple_p (bitnum, BITS_PER_UNIT, bytenum)
>>   	  && known_eq (bitsize, GET_MODE_BITSIZE (mode))
>>   	  && (!targetm.slow_unaligned_access (mode, MEM_ALIGN (op0))
>>   	      || (multiple_p (bitnum, GET_MODE_ALIGNMENT (mode))
> 
> Do we have a genuinely negative bit offset here?  Seems like the callers
> would need to be updated if so, since the code is consistent in treating
> the offset as unsigned.
> 

Aehm, yes.

The test case plural.i contains this:

static const yytype_int8 yypgoto[] =
{
      -10, -10, -1
};

static const yytype_uint8 yyr1[] =
{
        0, 16, 17, 18, 18, 18, 18, 18, 18, 18,
       18, 18, 18, 18
};

   yyn = yyr1[yyn];

   yystate = yypgoto[yyn - 16] + *yyssp;


There will probably a reason why yyn can never be 0
in yyn = yyr1[yyn]; but it is not really obvious.

In plural.i.228t.optimized we have:

  pretmp_400 = yypgoto[-16];
   _385 = (int) pretmp_400;
   goto <bb 69>; [100.00%]

which is expanded to:

(insn 927 926 928 59 (set (reg/f:DI 369)
         (symbol_ref:DI ("yypgoto") [flags 0x6] <var_decl 0x7f76af1d1c60 yypgoto>)) -1
      (nil))
(insn 928 927 929 59 (set (reg:DI 372)
         (const_int 1 [0x1])) -1
      (nil))
(insn 929 928 930 59 (set (reg:DI 371)
         (ashift:DI (reg:DI 372)
             (const_int 61 [0x3d]))) -1
      (expr_list:REG_EQUAL (const_int 2305843009213693952 [0x2000000000000000])
         (nil)))
(insn 930 929 931 59 (set (reg/f:DI 370)
         (plus:DI (reg/f:DI 369)
             (reg:DI 371))) -1
      (nil))
(insn 931 930 932 59 (set (reg:DI 375)
         (plus:DI (reg/f:DI 370)
             (const_int 1 [0x1]))) -1
      (nil))
(insn 932 931 933 59 (set (reg:DI 376)
         (mem/u/c:DI (and:DI (plus:DI (reg/f:DI 370)
                     (const_int -16 [0xfffffffffffffff0]))
                 (const_int -8 [0xfffffffffffffff8])) [0  S8 A64])) -1
      (nil))
(insn 933 932 934 59 (set (reg:DI 377)
         (ashift:DI (reg:DI 376)
             (minus:DI (const_int 64 [0x40])
                 (ashift:DI (and:DI (reg:DI 375)
                         (const_int 7 [0x7]))
                     (const_int 3 [0x3]))))) -1
      (nil))
(insn 934 933 935 59 (set (reg:DI 374)
         (ashiftrt:DI (reg:DI 377)
             (const_int 56 [0x38]))) -1
      (nil))


Certainly there is also a target bug, because

(define_insn "*movdi"
   [(set (match_operand:DI 0 "nonimmediate_operand"
                                 "=r,r,r,r,r,r,r,r, m, *f,*f, Q, r,*f")
         (match_operand:DI 1 "input_operand"
                                 "rJ,K,L,T,s,n,s,m,rJ,*fJ, Q,*f,*f, r"))]
   "register_operand (operands[0], DImode)
    || reg_or_0_operand (operands[1], DImode)"
   "@
    mov %r1,%0
    lda %0,%1($31)
    ldah %0,%h1($31)
    #
    #
    #
    lda %0,%1
    ldq%A1 %0,%1
    stq%A0 %r1,%0
    fmov %R1,%0
    ldt %0,%1
    stt %R1,%0
    ftoit %1,%0
    itoft %1,%0"

overload 7 of 14, has constraint op0 = r, op1 = s.
and assembles to lda %0,%1 so s matches symbol_ref+any_integer
I think even without the bug in the expansion, that could
happen if I write
volatile x = 0; if (x) { char *y = &yypgoto[0x20000000000000]; }

I think there should be a splitter for large offsets that gets enabled
after reload and splits the mov r,s+x into mov r,s; add r,x or something
appropriate.

So let's see, how does insn 929 get expanded?

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(Revision 263644)
+++ emit-rtl.c	(Arbeitskopie)
@@ -4023,6 +4023,8 @@
    insn = as_a <rtx_insn *> (rtx_alloc (INSN));
  
    INSN_UID (insn) = cur_insn_uid++;
+  if (INSN_UID (insn) == 929)
+    asm ("int3");
    PATTERN (insn) = pattern;
    INSN_CODE (insn) = -1;
    REG_NOTES (insn) = NULL;

gdb --args ../gcc-build-alpha-linux-gnu/gcc/cc1 -fpreprocessed plural.i -quiet -dumpbase plural.i -mlong-double-128 -mieee "-mfp-rounding-mode=d" -auxbase plural -g -O2 -Wall -Werror -Wundef -Wwrite-strings -Wstrict-prototypes -Wold-style-definition "-std=gnu11" -fgnu89-inline -fmerge-all-constants -fno-stack-protector -frounding-math -fno-math-errno "-ftls-model=initial-exec" -o plural.s

#0  make_insn_raw (pattern=pattern@entry=0x7ffff6e60c30) at ../../gcc-trunk/gcc/emit-rtl.c:4031
#1  0x00000000007fde49 in emit_insn (x=x@entry=0x7ffff6e60c30) at ../../gcc-trunk/gcc/emit-rtl.c:5108
#2  0x0000000000a92623 in expand_binop_directly (icode=CODE_FOR_ashldi3, mode=mode@entry=E_DImode,
     binoptab=binoptab@entry=ashl_optab, op0=op0@entry=0x7ffff6e60be8, op1=op1@entry=0x7ffff6ecf860,
     target=target@entry=0x7ffff6e60bd0, unsignedp=unsignedp@entry=0,
     methods=methods@entry=OPTAB_WIDEN, last=last@entry=0x7ffff6e618c0)
     at ../../gcc-trunk/gcc/optabs.c:1114
#3  0x0000000000a8fe62 in expand_binop (mode=mode@entry=E_DImode, binoptab=<optimized out>,
     binoptab@entry=ashl_optab, op0=op0@entry=0x7ffff6e60be8, op1=<optimized out>,
     target=target@entry=0x7ffff6e60bd0, unsignedp=unsignedp@entry=0, methods=methods@entry=OPTAB_WIDEN)
     at ../../gcc-trunk/gcc/optabs.c:1186
#4  0x0000000000f1421b in alpha_emit_set_const_1 (target=target@entry=0x7ffff6e60bd0,
     mode=mode@entry=E_DImode, c=c@entry=2305843009213693952, n=n@entry=2,
     no_output=no_output@entry=false) at ../../gcc-trunk/gcc/config/alpha/alpha.c:1925
#5  0x0000000000f147a0 in alpha_emit_set_const (target=0x7ffff6e60bd0, mode=E_DImode,
     c=2305843009213693952, n=3, no_output=<optimized out>)
     at ../../gcc-trunk/gcc/config/alpha/alpha.c:2048
#6  0x0000000000f1545a in alpha_split_const_mov (mode=mode@entry=E_DImode,
     operands=operands@entry=0x7fffffffc7b0) at ../../gcc-trunk/gcc/config/alpha/alpha.c:2214
#7  0x0000000000f15536 in alpha_expand_mov (mode=mode@entry=E_DImode,
     operands=operands@entry=0x7fffffffc7b0) at ../../gcc-trunk/gcc/config/alpha/alpha.c:2263
#8  0x00000000010f0f67 in gen_movdi (operand0=0x7ffff6e60bd0, operand1=0x7ffff6da8580)
     at ../../gcc-trunk/gcc/config/alpha/alpha.md:4062
#9  0x0000000000833893 in operator() (a1=0x7ffff6da8580, a0=0x7ffff6e60bd0, this=<optimized out>)
     at ../../gcc-trunk/gcc/recog.h:301
#10 emit_move_insn_1 (x=x@entry=0x7ffff6e60bd0, y=y@entry=0x7ffff6da8580)
     at ../../gcc-trunk/gcc/expr.c:3694
#11 0x0000000000833b86 in emit_move_insn (x=x@entry=0x7ffff6e60bd0, y=y@entry=0x7ffff6da8580)
     at ../../gcc-trunk/gcc/expr.c:3790
#12 0x0000000000810aa0 in copy_to_mode_reg (mode=mode@entry=E_DImode, x=0x7ffff6da8580)
     at ../../gcc-trunk/gcc/explow.c:632
#13 0x0000000000a8b69a in maybe_legitimize_operand (op=0x7fffffffca40, opno=<optimized out>,
     icode=CODE_FOR_adddi3) at ../../gcc-trunk/gcc/optabs.c:7168
#14 maybe_legitimize_operands (icode=icode@entry=CODE_FOR_adddi3, opno=opno@entry=0,
     nops=nops@entry=3, ops=ops@entry=0x7fffffffca10) at ../../gcc-trunk/gcc/optabs.c:7292
#15 0x0000000000a8bcde in maybe_gen_insn (icode=icode@entry=CODE_FOR_adddi3, nops=nops@entry=3,
     ops=ops@entry=0x7fffffffca10) at ../../gcc-trunk/gcc/optabs.c:7311
#16 0x0000000000a925ff in expand_binop_directly (icode=CODE_FOR_adddi3, mode=mode@entry=E_DImode,
     binoptab=binoptab@entry=add_optab, op0=op0@entry=0x7ffff6e60b40, op1=op1@entry=0x7ffff6da8580,
     target=target@entry=0x0, unsignedp=unsignedp@entry=1, methods=methods@entry=OPTAB_LIB_WIDEN,
     last=last@entry=0x7ffff6e61880) at ../../gcc-trunk/gcc/optabs.c:1098
#17 0x0000000000a8fe62 in expand_binop (mode=mode@entry=E_DImode, binoptab=<optimized out>,
     op0=op0@entry=0x7ffff6e60b40, op1=<optimized out>, target=target@entry=0x0,
     unsignedp=unsignedp@entry=1, methods=OPTAB_LIB_WIDEN) at ../../gcc-trunk/gcc/optabs.c:1186
#18 0x0000000000a923e3 in expand_simple_binop (mode=mode@entry=E_DImode, code=code@entry=PLUS,
     op0=op0@entry=0x7ffff6e60b40, op1=<optimized out>, target=target@entry=0x0,
     unsignedp=unsignedp@entry=1, methods=<optimized out>, methods@entry=OPTAB_LIB_WIDEN)
     at ../../gcc-trunk/gcc/optabs.c:917
#19 0x0000000000f12d6f in alpha_legitimize_address_1 (x=0x7ffff6e60b40, x@entry=0x7ffff6e60ba0,
     scratch=scratch@entry=0x0, mode=<optimized out>) at ../../gcc-trunk/gcc/config/alpha/alpha.c:1159
#20 0x0000000000f133cb in alpha_legitimize_address (x=0x7ffff6e60ba0, oldx=<optimized out>,
     mode=<optimized out>) at ../../gcc-trunk/gcc/config/alpha/alpha.c:1177
#21 0x0000000000810922 in memory_address_addr_space (mode=E_QImode, x=0x7ffff6e60ba0,
     as=<optimized out>) at ../../gcc-trunk/gcc/explow.c:450
#22 0x00000000007f689d in change_address_1 (memref=memref@entry=0x7ffff6e60b88,
     mode=mode@entry=E_QImode, addr=addr@entry=0x7ffff6e60ba0, validate=validate@entry=1,
     inplace=inplace@entry=false) at ../../gcc-trunk/gcc/emit-rtl.c:2288
#23 0x00000000007fab5c in adjust_address_1 (memref=memref@entry=0x7ffff6e60b88,
     mode=mode@entry=E_QImode, offset=..., validate=validate@entry=1,
     adjust_address=adjust_address@entry=1, adjust_object=adjust_object@entry=1, size=...,
     size@entry=...) at ../../gcc-trunk/gcc/emit-rtl.c:2420
#24 0x000000000081942a in extract_bit_field_1 (str_rtx=str_rtx@entry=0x7ffff6e60b88,
     bitsize=bitsize@entry=..., bitnum=..., bitnum@entry=..., unsignedp=unsignedp@entry=0,
     target=target@entry=0x0, mode=mode@entry=E_QImode, tmode=<optimized out>, tmode@entry=E_QImode,
     reverse=reverse@entry=false, fallback_p=fallback_p@entry=true, alt_rtl=alt_rtl@entry=0x0)
     at ../../gcc-trunk/gcc/expmed.c:1805
#25 0x000000000081a063 in extract_bit_field (str_rtx=0x7ffff6e60b88, bitsize=..., bitnum=...,
     unsignedp=0, target=target@entry=0x0, mode=mode@entry=E_QImode, tmode=tmode@entry=E_QImode,
     reverse=false, alt_rtl=alt_rtl@entry=0x0) at ../../gcc-trunk/gcc/expmed.c:2097
#26 0x000000000082f828 in expand_expr_real_1 (exp=<optimized out>, target=<optimized out>,
     tmode=<optimized out>, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=<optimized out>)
     at ../../gcc-trunk/gcc/expr.c:10801
#27 0x000000000083047a in expand_expr_real_1 (exp=0x7ffff6df8f30, target=<optimized out>,
     tmode=E_SImode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=<optimized out>)
     at ../../gcc-trunk/gcc/expr.c:9861
#28 0x000000000083d9de in expand_expr (modifier=EXPAND_NORMAL, mode=E_SImode, target=0x0,
     exp=0x7ffff6df8f30) at ../../gcc-trunk/gcc/expr.h:279
#29 expand_expr_real_2 (ops=ops@entry=0x7fffffffd8a0, target=<optimized out>, tmode=E_SImode,
     modifier=modifier@entry=EXPAND_NORMAL) at ../../gcc-trunk/gcc/expr.c:8455
#30 0x000000000070ba16 in expand_gimple_stmt_1 (stmt=stmt@entry=0x7ffff6e0eb40)
     at ../../gcc-trunk/gcc/cfgexpand.c:3674
#31 0x000000000070d4ad in expand_gimple_stmt (stmt=stmt@entry=0x7ffff6e0eb40)
     at ../../gcc-trunk/gcc/cfgexpand.c:3734
#32 0x000000000070dbbe in expand_gimple_basic_block (bb=0x7ffff6deb478,
     disable_tail_calls=disable_tail_calls@entry=false) at ../../gcc-trunk/gcc/cfgexpand.c:5770
#33 0x0000000000713597 in (anonymous namespace)::pass_expand::execute (this=<optimized out>,
     fun=0x7ffff70ab4d0) at ../../gcc-trunk/gcc/cfgexpand.c:6373
#34 0x0000000000ab4e25 in execute_one_pass (pass=pass@entry=0x1b28700)
     at ../../gcc-trunk/gcc/passes.c:2446
#35 0x0000000000ab5738 in execute_pass_list_1 (pass=0x1b28700) at ../../gcc-trunk/gcc/passes.c:2535
#36 0x0000000000ab5795 in execute_pass_list (fn=0x7ffff70ab4d0, pass=<optimized out>)
     at ../../gcc-trunk/gcc/passes.c:2546
#37 0x000000000074efe8 in cgraph_node::expand (this=0x7ffff6cfb840)
     at ../../gcc-trunk/gcc/cgraphunit.c:2116
#38 0x00000000007508a1 in expand_all_functions () at ../../gcc-trunk/gcc/cgraphunit.c:2254
#39 symbol_table::compile (this=this@entry=0x7ffff6ecc000) at ../../gcc-trunk/gcc/cgraphunit.c:2605
#40 0x0000000000752c75 in symbol_table::finalize_compilation_unit (this=0x7ffff6ecc000)
     at ../../gcc-trunk/gcc/cgraphunit.c:2698
#41 0x0000000000b97d6d in compile_file () at ../../gcc-trunk/gcc/toplev.c:480
#42 0x000000000059cb5e in do_compile () at ../../gcc-trunk/gcc/toplev.c:2165
#43 toplev::main (this=this@entry=0x7fffffffdce0, argc=argc@entry=28, argv=argv@entry=0x7fffffffdde8)
     at ../../gcc-trunk/gcc/toplev.c:2300
#44 0x000000000059ecb7 in main (argc=28, argv=0x7fffffffdde8) at ../../gcc-trunk/gcc/main.c:39

(gdb) frame 26
#26 0x000000000082f828 in expand_expr_real_1 (exp=<optimized out>, target=<optimized out>,
     tmode=<optimized out>, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=<optimized out>)
     at ../../gcc-trunk/gcc/expr.c:10801
10801					     ext_mode, ext_mode, reversep, alt_rtl);
(gdb) list
10796		      reversep = TYPE_REVERSE_STORAGE_ORDER (type);
10797	
10798		    op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp,
10799					     (modifier == EXPAND_STACK_PARM
10800					      ? NULL_RTX : target),
10801					     ext_mode, ext_mode, reversep, alt_rtl);
10802	
10803		    /* If the result has a record type and the mode of OP0 is an
10804		       integral mode then, if BITSIZE is narrower than this mode
10805		       and this is for big-endian data, we must put the field
(gdb) p bitpos
$1 = {<poly_int_pod<1u, long>> = {coeffs = {-128}}, <No data fields>}
(gdb) frame 25
#25 0x000000000081a063 in extract_bit_field (str_rtx=0x7ffff6e60b88, bitsize=..., bitnum=...,
     unsignedp=0, target=target@entry=0x0, mode=mode@entry=E_QImode, tmode=tmode@entry=E_QImode,
     reverse=false, alt_rtl=alt_rtl@entry=0x0) at ../../gcc-trunk/gcc/expmed.c:2097
2097				      target, mode, tmode, reverse, true, alt_rtl);
(gdb) list
2092	      return extract_bit_field_1 (str_rtx, ibitsize, ibitnum, unsignedp,
2093					  target, mode, tmode, reverse, true, alt_rtl);
2094	    }
2095	
2096	  return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp,
2097				      target, mode, tmode, reverse, true, alt_rtl);
2098	}
2099	^L
2100	/* Use shifts and boolean operations to extract a field of BITSIZE bits
2101	   from bit BITNUM of OP0.  If OP0_MODE is defined, it is the mode of OP0,
(gdb) p/x bitnum
$3 = {<poly_int_pod<1u, unsigned long>> = {coeffs = {0xffffffffffffff80}}, <No data fields>}
(gdb) frame 24
#24 0x000000000081942a in extract_bit_field_1 (str_rtx=str_rtx@entry=0x7ffff6e60b88,
     bitsize=bitsize@entry=..., bitnum=..., bitnum@entry=..., unsignedp=unsignedp@entry=0,
     target=target@entry=0x0, mode=mode@entry=E_QImode, tmode=<optimized out>, tmode@entry=E_QImode,
     reverse=reverse@entry=false, fallback_p=fallback_p@entry=true, alt_rtl=alt_rtl@entry=0x0)
     at ../../gcc-trunk/gcc/expmed.c:1805
1805	      op0 = adjust_bitfield_address (op0, mode1, bytenum);
(gdb) list
1800	  /* Extraction of a full MODE1 value can be done with a load as long as
1801	     the field is on a byte boundary and is sufficiently aligned.  */
1802	  poly_uint64 bytenum;
1803	  if (simple_mem_bitfield_p (op0, bitsize, bitnum, mode1, &bytenum))
1804	    {
1805	      op0 = adjust_bitfield_address (op0, mode1, bytenum);
1806	      if (reverse)
1807		op0 = flip_storage_order (mode1, op0);
1808	      return convert_extracted_bit_field (op0, mode, tmode, unsignedp);
1809	    }
(gdb) p/x bytenum
$5 = {<poly_int_pod<1u, unsigned long>> = {coeffs = {0x1ffffffffffffff0}}, <No data fields>}

The byte offset is completely wrong now, due to the bitnum was initially a negative integer
and got converted to unsigned.  At the moment when that is converted to byte offsets it is
done wrong.  I think it is too much to change everything to signed arithmetics,
but at least when converting a bit pos to a byte pos, it should be done in signed arithmetics.

So yes, I think a negative offset can happen.  And it must be handled correctly.


Thanks
Bernd.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]