Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses

[Richard Biener]

On Mon, Jul 17, 2017 at 12:48 PM, Eric Botcazou  wrote:
>> So this isn't about global
>>
>>  void *x[] = { &((struct Y *)0x12)->foo }
>>
>> but for a local one where supposedly variable indexing is valid (don't
>> we gimplify that)?
>
> Yes, it's local (it's OK if global because we do a full RTL expansion).
> Everything is valid, constant and passes initializer_constant_valid_p.
>
>> And
>>
>> +case INDIRECT_REF:
>> +  /* This deals with absolute addresses.  */
>> +  offset += tree_to_shwi (TREE_OPERAND (target, 0));
>> +  x = gen_rtx_MEM (QImode,
>> +  gen_rtx_SYMBOL_REF (Pmode, "origin of addresses"));
>>
>> simply translates 0x12 to &* + 0x12 (where origin == 0 somehow?).
>>
>> I suppose returing directly here and sth like
>>
>> value->base = gen_rtx_SYMBOL_REF (Pmode, "origin of addresses");
>> value->offset = offset + tree_to_shwi (...);
>> return;
>>
>> would be clearer.
>
> That's a matter of consistency, the LABEL_DECL case does something equivalent:
>
>   x = gen_rtx_MEM (FUNCTION_MODE,
>gen_rtx_LABEL_REF (Pmode, force_label_rtx (target)));

Ah, I see.

>> Or even
>>
>> value->base = tree-to-rtx (TREE_OPERAND (target, 0));
>> value->offset = offset;
>
> The callers expect the base to be SYMBOL_REF or LABEL_REF though.

Ok.  I suppose I'm mostly concerned about that magic "origin of addresses".

Patch is ok in its original form.  As said, handling MEM_REF would be nice,
FEs start to generate that (well C++ does).

Thanks,
Richard.

> --
> Eric Botcazou

Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses

[Eric Botcazou]

> So this isn't about global
> 
>  void *x[] = { &((struct Y *)0x12)->foo }
> 
> but for a local one where supposedly variable indexing is valid (don't
> we gimplify that)?

Yes, it's local (it's OK if global because we do a full RTL expansion).
Everything is valid, constant and passes initializer_constant_valid_p.

> And
> 
> +case INDIRECT_REF:
> +  /* This deals with absolute addresses.  */
> +  offset += tree_to_shwi (TREE_OPERAND (target, 0));
> +  x = gen_rtx_MEM (QImode,
> +  gen_rtx_SYMBOL_REF (Pmode, "origin of addresses"));
> 
> simply translates 0x12 to &* + 0x12 (where origin == 0 somehow?).
> 
> I suppose returing directly here and sth like
> 
> value->base = gen_rtx_SYMBOL_REF (Pmode, "origin of addresses");
> value->offset = offset + tree_to_shwi (...);
> return;
> 
> would be clearer.

That's a matter of consistency, the LABEL_DECL case does something equivalent:

  x = gen_rtx_MEM (FUNCTION_MODE,
   gen_rtx_LABEL_REF (Pmode, force_label_rtx (target)));

> Or even
> 
> value->base = tree-to-rtx (TREE_OPERAND (target, 0));
> value->offset = offset;

The callers expect the base to be SYMBOL_REF or LABEL_REF though.

-- 
Eric Botcazou

Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses

[Richard Biener]

On Mon, Jul 17, 2017 at 10:51 AM, Eric Botcazou  wrote:
>> Apart from the MEM construction where I simply trust you this looks
>> ok.  Mind adding MEM_REF support for this case as well?
>
> Do you mean MEM_REF ?  Is that possible?

Yes.

>> Btw, why's simply output_constant_def (TREE_OPERAND (target, 0), 1);
>> not correct?
>
> If you do that, you get a symbol in the constant pool whose value (address) is
> arbitrary; here what we want is a fixed value.  That being said, given that
> the contents of the contant pool is hashed, there is very likely not much
> difference in the end, although that would be conceptually incorrect.
>
>> Isn't this about &*0x1?
>
> Yes, it's not the address of a constant, it's the address of an object whose
> base address is absolute, so &(abs_address)->field[index].  This kind of thing
> is not folded by build_fold_addr_expr.

So this isn't about global

 void *x[] = { &((struct Y *)0x12)->foo }

but for a local one where supposedly variable indexing is valid (don't
we gimplify
that)?

And

+case INDIRECT_REF:
+  /* This deals with absolute addresses.  */
+  offset += tree_to_shwi (TREE_OPERAND (target, 0));
+  x = gen_rtx_MEM (QImode,
+  gen_rtx_SYMBOL_REF (Pmode, "origin of addresses"));

simply translates 0x12 to &* + 0x12 (where origin == 0 somehow?).

I suppose returing directly here and sth like

value->base = gen_rtx_SYMBOL_REF (Pmode, "origin of addresses");
value->offset = offset + tree_to_shwi (...);
return;

would be clearer.  Or even

value->base = tree-to-rtx (TREE_OPERAND (target, 0));
value->offset = offset;

?


> --
> Eric Botcazou

Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses

[Eric Botcazou]

> Apart from the MEM construction where I simply trust you this looks
> ok.  Mind adding MEM_REF support for this case as well?

Do you mean MEM_REF ?  Is that possible?

> Btw, why's simply output_constant_def (TREE_OPERAND (target, 0), 1);
> not correct?

If you do that, you get a symbol in the constant pool whose value (address) is 
arbitrary; here what we want is a fixed value.  That being said, given that 
the contents of the contant pool is hashed, there is very likely not much 
difference in the end, although that would be conceptually incorrect.

> Isn't this about &*0x1?

Yes, it's not the address of a constant, it's the address of an object whose 
base address is absolute, so &(abs_address)->field[index].  This kind of thing 
is not folded by build_fold_addr_expr.

-- 
Eric Botcazou

Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses

[Richard Biener]

On Fri, Jul 7, 2017 at 1:08 PM, Eric Botcazou  wrote:
> Hi,
>
> this fixes the following ICE in decode_addr_const:
>
> +===GNAT BUG DETECTED==+
> | 8.0.0 20170704 (experimental) [trunk revision 249942] (x86_64-suse-linux)
> GCC error:|
> | in decode_addr_const, at varasm.c:2880
>
> stemming from a CONSTRUCTOR containing absolute addresses hidden behind a
> COMPONENT_REF or similar references.
>
> Fixed by adding support for INDIRECT_REF  to decode_addr_const.
>
> Tested on x86_64-suse-linux, OK for the mainline?

Apart from the MEM construction where I simply trust you this looks
ok.  Mind adding
MEM_REF support for this case as well?

Btw, why's simply output_constant_def (TREE_OPERAND (target, 0), 1);
not correct?

Isn't this about &*0x1?

Thanks,
Richard.

>
> 2017-07-07  Eric Botcazou  
>
> * varasm.c (decode_addr_const): Deal with INDIRECT_REF .
>
>
> 2017-07-07  Eric Botcazou  
>
> * gnat.dg/aggr22.ad[sb]: New test.
>
> --
> Eric Botcazou