Patchwork Fix array sizes created by Java FE (PR libgcj/57074)

login
register
mail settings
Submitter Alan Modra
Date May 4, 2013, 5:55 a.m.
Message ID <20130504055505.GT5221@bubble.grove.modra.org>
Download mbox | patch
Permalink /patch/241428/
State New
Headers show

Comments

Alan Modra - May 4, 2013, 5:55 a.m.
I believe the real problem here is in place_block_symbol() and
output_object_block().  If DECL_INITIAL is given for an array, then
shouldn't we be taking the size from the initializer?

This patch fixes that problem, and ensures that we get an assembler
error if placement in the block changes.  I've bootstrapped with
Jakub's java change, and the libjava failures are gone.  However, the
checking code shows up failures in gcc.c-torture/execute/20010924-1.c
with "attempt to move .org backwards".  This isn't exactly a
regression as previously we generated bad code silently, but clearly
more work is needed to teach place_block_symbol() how to size
variables properly, or else something is going wrong elsewhere in the
compiler..

	PR libcgj/57074
	* varasm.c (place_block_symbol): Use DECL_INITIAL size for
	variables if available.
	(output_object_block): Likewise.  Use .org for each item in
	section anchor block rather than padding.
Eric Botcazou - May 4, 2013, 8:34 a.m.
> I believe the real problem here is in place_block_symbol() and
> output_object_block().  If DECL_INITIAL is given for an array, then
> shouldn't we be taking the size from the initializer?

This means that the size of the array and the size of the initializer don't 
agree, right?  IMO this should be fixed instead since this could run afoul of 
various optimizations using array_ref_up_bound for example.
Alan Modra - May 4, 2013, 10:20 a.m.
On Sat, May 04, 2013 at 10:34:52AM +0200, Eric Botcazou wrote:
> > I believe the real problem here is in place_block_symbol() and
> > output_object_block().  If DECL_INITIAL is given for an array, then
> > shouldn't we be taking the size from the initializer?
> 
> This means that the size of the array and the size of the initializer don't 
> agree, right?  IMO this should be fixed instead since this could run afoul of 
> various optimizations using array_ref_up_bound for example.

Good to hear.  I wasn't sure whether the sizes were even supposed to
agree.  Assuming Jakub's second patch fixes java for us (testing now),
that just leaves gcc.c-torture/execute/20010924-1.c which fails to
size a3 and a4 properly.  Both of these vars have DECL_SIZE_UNIT of 1.

struct {
  char a3c;
  char a3p[];
} a3 = {
  'o',
  "wx"
};

struct {
  char a4c;
  char a4p[];
} a4 = {
  '9',
  { 'e', 'b' }
};
Eric Botcazou - May 4, 2013, 10:31 a.m.
> Good to hear.  I wasn't sure whether the sizes were even supposed to
> agree.  Assuming Jakub's second patch fixes java for us (testing now),
> that just leaves gcc.c-torture/execute/20010924-1.c which fails to
> size a3 and a4 properly.  Both of these vars have DECL_SIZE_UNIT of 1.
> 
> struct {
>   char a3c;
>   char a3p[];
> } a3 = {
>   'o',
>   "wx"
> };
> 
> struct {
>   char a4c;
>   char a4p[];
> } a4 = {
>   '9',
>   { 'e', 'b' }
> };

Flexible array members (or related GNU extensions) are a specific issue, 
reported under PR middle-end/28865.
Alan Modra - May 4, 2013, 11:51 a.m.
On Sat, May 04, 2013 at 12:31:31PM +0200, Eric Botcazou wrote:
> > Good to hear.  I wasn't sure whether the sizes were even supposed to
> > agree.  Assuming Jakub's second patch fixes java for us (testing now),
> > that just leaves gcc.c-torture/execute/20010924-1.c which fails to
> > size a3 and a4 properly.  Both of these vars have DECL_SIZE_UNIT of 1.
> > 
> > struct {
> >   char a3c;
> >   char a3p[];
> > } a3 = {
> >   'o',
> >   "wx"
> > };
> > 
> > struct {
> >   char a4c;
> >   char a4p[];
> > } a4 = {
> >   '9',
> >   { 'e', 'b' }
> > };
> 
> Flexible array members (or related GNU extensions) are a specific issue, 
> reported under PR middle-end/28865.

Actually when I look more closely, it was me taking the size from
DECL_INITIAL that caused these to fail.  Interestingly, the var_decl
size is correct.  I was wrong to claim it wasn't.

	.org .LANCB1+0
	.type	a4, @object
	.size	a4, 1
a4:
	.byte	57
	.byte	101
	.byte	98
	.org .LANCB1+3
	.type	a3, @object
	.size	a3, 1
a3:
	.byte	111
	.string	"wx"
	.org .LANCB1+7

From that, I see the .size is wrong (assemble_variable_contents() uses
DECL_INITIAL), but the size gleaned from the .org differences
(effectively from var_decl DECL_SIZE_UNIT) is correct.

Patch

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 198274)
+++ gcc/varasm.c	(working copy)
@@ -6979,7 +6989,11 @@ 
     {
       decl = SYMBOL_REF_DECL (symbol);
       alignment = DECL_ALIGN (decl);
-      size = tree_low_cst (DECL_SIZE_UNIT (decl), 1);
+      if (DECL_INITIAL (decl)
+	  && DECL_INITIAL (decl) != error_mark_node)
+	size = int_size_in_bytes (TREE_TYPE (DECL_INITIAL (decl)));
+      else
+	size = tree_low_cst (DECL_SIZE_UNIT (decl), 1);
       if (flag_asan && asan_protect_global (decl))
 	{
 	  size += asan_red_zone_size (size);
@@ -7095,6 +7109,10 @@ 
   HOST_WIDE_INT offset;
   tree decl;
   rtx symbol;
+#if HAVE_GNU_AS
+  static int labelno;
+  char buf[30];
+#endif
 
   if (!block->objects)
     return;
@@ -7104,6 +7122,12 @@ 
   switch_to_section (block->sect);
   assemble_align (block->alignment);
 
+#if HAVE_GNU_AS
+  ASM_GENERATE_INTERNAL_LABEL (buf, "LANCB", labelno);
+  ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, buf);
+  ++labelno;
+#endif
+
   /* Define the values of all anchors relative to the current section
      position.  */
   FOR_EACH_VEC_SAFE_ELT (block->anchors, i, symbol)
@@ -7114,7 +7138,14 @@ 
   FOR_EACH_VEC_ELT (*block->objects, i, symbol)
     {
       /* Move to the object's offset, padding with zeros if necessary.  */
+#if HAVE_GNU_AS
+      fprintf (asm_out_file, "\t.org ");
+      assemble_name_raw (asm_out_file, buf);
+      fprintf (asm_out_file, "+" HOST_WIDE_INT_PRINT_DEC "\n",
+	       SYMBOL_REF_BLOCK_OFFSET (symbol));
+#else
       assemble_zeros (SYMBOL_REF_BLOCK_OFFSET (symbol) - offset);
+#endif
       offset = SYMBOL_REF_BLOCK_OFFSET (symbol);
       if (CONSTANT_POOL_ADDRESS_P (symbol))
 	{
@@ -7144,7 +7175,11 @@ 
 	  HOST_WIDE_INT size;
 	  decl = SYMBOL_REF_DECL (symbol);
 	  assemble_variable_contents (decl, XSTR (symbol, 0), false);
-	  size = tree_low_cst (DECL_SIZE_UNIT (decl), 1);
+	  if (DECL_INITIAL (decl)
+	      && DECL_INITIAL (decl) != error_mark_node)
+	    size = int_size_in_bytes (TREE_TYPE (DECL_INITIAL (decl)));
+	  else
+	    size = tree_low_cst (DECL_SIZE_UNIT (decl), 1);
 	  offset += size;
 	  if (flag_asan && asan_protect_global (decl))
 	    {