| Message ID | 2319634.9b0vxstqzg@polaris |
|---|---|
| State | New |
| Headers | show |
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
> 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.
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
> 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.
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
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 (); }