Patchwork Mark static const strings as read-only.

login
register
mail settings
Submitter Tom de Vries
Date Oct. 10, 2011, 3:15 p.m.
Message ID <4E930BF4.1050008@mentor.com>
Download mbox | patch
Permalink /patch/118754/
State New
Headers show

Comments

Tom de Vries - Oct. 10, 2011, 3:15 p.m.
On 10/10/2011 03:44 PM, Eric Botcazou wrote:
>> The patch makes sure that the src_mem computed here in
>> expand_builtin_memcpy is marked as MEM_READONLY_P:
>> ...
>>       src_mem = get_memory_rtx (src, len);
>> ...
>>
>> build and reg-tested on i686, arm, and mips.
>>
>> OK for trunk?
> 
> Do you get the same result by calling gen_const_mem from build_constant_desc 
> instead of gen_rtx_MEM?  If so, this would be better I think.
> 

I tried the patch that you describe:
...
...

The src_mem in expand_builtin_memcpy is generated in get_memory_rtx:
...
static rtx
get_memory_rtx (tree exp, tree len)
{
  tree orig_exp = exp;
  rtx addr, mem;
  HOST_WIDE_INT off;

  /* When EXP is not resolved SAVE_EXPR, MEM_ATTRS can be still derived
     from its expression, for expr->a.b only <variable>.a.b is recorded.  */
  if (TREE_CODE (exp) == SAVE_EXPR && !SAVE_EXPR_RESOLVED_P (exp))
    exp = TREE_OPERAND (exp, 0);

  addr = expand_expr (orig_exp, NULL_RTX, ptr_mode, EXPAND_NORMAL);
  mem = gen_rtx_MEM (BLKmode, memory_address (BLKmode, addr));
...

With the patch for build_constant_desc, a mem with MEM_READONLY_P set (/u) is
generated in expand_expr_constant during expand_expr:
...
(mem/s/u/c:BLK (symbol_ref/f:SI ("*$LC0") [flags 0x2]  <var_decl 0xf7dc7900
*$LC0>) [1 S4 A32])
...

but the expand_expr returns this:
...
(symbol_ref/f:SI ("*$LC0") [flags 0x2]  <var_decl 0xf7dc7900 *$LC0>)
...

The subsequent gen_rtx_MEM generates this:
...
(mem:BLK (lo_sum:SI (reg:SI 195)
        (symbol_ref/f:SI ("*$LC0") [flags 0x2]  <var_decl 0xf7dc7900 *$LC0>)) [0
A8])
...

and after set_mem_attributes it looks like this:
...
(mem/s/c:BLK (lo_sum:SI (reg:SI 195)
        (symbol_ref/f:SI ("*$LC0") [flags 0x2]  <var_decl 0xf7dc7900 *$LC0>)) [0
S4 A32])
...

So, the patch for build_constant_desc does not have the desired effect.

Thanks,
- Tom
Eric Botcazou - Oct. 10, 2011, 3:50 p.m.
> So, the patch for build_constant_desc does not have the desired effect.

OK, too bad that we need to play this back-and-forth game with MEMs.  So the 
original patch is OK (with TREE_READONLY (base) on the next line to mimic what 
is done just above and without the gcc/ prefix in the ChangeLog).  If you have 
some available cycles, you can test and install the build_constant_desc change 
in the same commit, otherwise I'll do it myself.
Tom de Vries - Oct. 12, 2011, 8:13 a.m.
On 10/10/2011 05:50 PM, Eric Botcazou wrote:
>> So, the patch for build_constant_desc does not have the desired effect.
> 
> OK, too bad that we need to play this back-and-forth game with MEMs.  So the 
> original patch is OK (with TREE_READONLY (base) on the next line to mimic what 
> is done just above and without the gcc/ prefix in the ChangeLog).  If you have 
> some available cycles, you can test and install the build_constant_desc change 
> in the same commit, otherwise I'll do it myself.
> 

I'll include the build_constant_desc change in a bootstrap/reg-test on x86_64.

Thanks,
- Tom

Patch

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (revision 179662)
+++ gcc/varasm.c (working copy)
@@ -3119,7 +3119,7 @@  build_constant_desc (tree exp)
   SET_SYMBOL_REF_DECL (symbol, decl);
   TREE_CONSTANT_POOL_ADDRESS_P (symbol) = 1;

-  rtl = gen_rtx_MEM (TYPE_MODE (TREE_TYPE (exp)), symbol);
+  rtl = gen_const_mem (TYPE_MODE (TREE_TYPE (exp)), symbol);
   set_mem_attributes (rtl, exp, 1);
   set_mem_alias_set (rtl, 0);
   set_mem_alias_set (rtl, const_alias_set);