* [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
@ 2017-07-07 11:10 Eric Botcazou
2017-07-17 8:14 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2017-07-07 11:10 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]
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 <INTEGER_CST> to decode_addr_const.
Tested on x86_64-suse-linux, OK for the mainline?
2017-07-07 Eric Botcazou <ebotcazou@adacore.com>
* varasm.c (decode_addr_const): Deal with INDIRECT_REF <INTEGER_CST>.
2017-07-07 Eric Botcazou <ebotcazou@adacore.com>
* gnat.dg/aggr22.ad[sb]: New test.
--
Eric Botcazou
[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 557 bytes --]
Index: varasm.c
===================================================================
--- varasm.c (revision 249942)
+++ varasm.c (working copy)
@@ -2876,6 +2876,13 @@ decode_addr_const (tree exp, struct addr
x = output_constant_def (target, 1);
break;
+ 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"));
+ break;
+
default:
gcc_unreachable ();
}
[-- Attachment #3: aggr22.adb --]
[-- Type: text/x-adasrc, Size: 262 bytes --]
-- { dg-do compile }
package body Aggr22 is
type Ptr is access all Integer;
type Arr is array (Positive range <>) of Ptr;
procedure Proc is
A : Arr (1 .. 33);
begin
A := (1 => null, 2 .. 32 => My_Rec.I'Access, 33 => null);
end;
end Aggr22;
[-- Attachment #4: aggr22.ads --]
[-- Type: text/x-adasrc, Size: 267 bytes --]
with System;
package Aggr22 is
type Rec is record
C : Character;
I : aliased Integer;
end record;
My_Rec : aliased Rec;
pragma Import (Ada, My_Rec);
for My_Rec'Address use System'To_Address (16#40086000#);
procedure Proc;
end Aggr22;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
2017-07-07 11:10 [patch] Fix ICE on CONSTRUCTOR containing absolute addresses Eric Botcazou
@ 2017-07-17 8:14 ` Richard Biener
2017-07-17 8:51 ` Eric Botcazou
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2017-07-17 8:14 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Fri, Jul 7, 2017 at 1:08 PM, Eric Botcazou <ebotcazou@adacore.com> 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 <INTEGER_CST> 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 <ebotcazou@adacore.com>
>
> * varasm.c (decode_addr_const): Deal with INDIRECT_REF <INTEGER_CST>.
>
>
> 2017-07-07 Eric Botcazou <ebotcazou@adacore.com>
>
> * gnat.dg/aggr22.ad[sb]: New test.
>
> --
> Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
2017-07-17 8:14 ` Richard Biener
@ 2017-07-17 8:51 ` Eric Botcazou
2017-07-17 10:33 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2017-07-17 8:51 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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 <INTEGER_CST, INTEGER_CST>? 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
2017-07-17 8:51 ` Eric Botcazou
@ 2017-07-17 10:33 ` Richard Biener
2017-07-17 10:49 ` Eric Botcazou
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2017-07-17 10:33 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Mon, Jul 17, 2017 at 10:51 AM, Eric Botcazou <ebotcazou@adacore.com> 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 <INTEGER_CST, INTEGER_CST>? 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 &*<origin> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
2017-07-17 10:33 ` Richard Biener
@ 2017-07-17 10:49 ` Eric Botcazou
2017-07-17 13:05 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2017-07-17 10:49 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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 &*<origin> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix ICE on CONSTRUCTOR containing absolute addresses
2017-07-17 10:49 ` Eric Botcazou
@ 2017-07-17 13:05 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2017-07-17 13:05 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Mon, Jul 17, 2017 at 12:48 PM, Eric Botcazou <ebotcazou@adacore.com> 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 &*<origin> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-17 13:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-07 11:10 [patch] Fix ICE on CONSTRUCTOR containing absolute addresses Eric Botcazou
2017-07-17 8:14 ` Richard Biener
2017-07-17 8:51 ` Eric Botcazou
2017-07-17 10:33 ` Richard Biener
2017-07-17 10:49 ` Eric Botcazou
2017-07-17 13:05 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).