diff mbox

Fix ICE on unaligned record field

Message ID 20150225154303.GE20953@virgil.suse
State New
Headers show

Commit Message

Martin Jambor Feb. 25, 2015, 3:43 p.m. UTC 
Hi Eric and Richard,

On Tue, Jan 06, 2015 at 06:07:12PM +0100, Eric Botcazou wrote:
> Martin,
> 
> > I suppose that could be done by something like the following, which I
> > have tested only very mildly so far, in particular I have not double
> > checked that get_inner_reference is cfun-agnostic.
> 
> The patch introduces no regressions on x86-64/Linux and makes the testcase 
> (gnat.dg/specs/pack12.ads attached to the first message) pass.
> 
> Do you plan to install it (along with the testcase)?
> 

for various reasons I was not able to do it earlier, but today I have
re-bootstrapped the following (the only change is the added testcase)
on x86_64-linux and it passes OK.  Should I commit it to trunk then?

Thanks,

Martin


2015-02-25  Martin Jambor  <mjambor@suse.cz>
	    Eric Botcazou  <ebotcazou@adacore.com>

gcc/
	* tree-sra.c (ipa_sra_check_caller_data): New type.
	(has_caller_p): Removed.
	(ipa_sra_check_caller): New function.
	(ipa_sra_preliminary_function_checks): Use it.

gcc/changelog/
	* gnat.dg/specs/pack12.ads: New test.

Comments

Eric Botcazou Feb. 28, 2015, 2:29 p.m. UTC | #1 
> for various reasons I was not able to do it earlier, but today I have
> re-bootstrapped the following (the only change is the added testcase)
> on x86_64-linux and it passes OK.  Should I commit it to trunk then?

Yes, that would be kind of you, thanks in advance.
H.J. Lu March 3, 2015, 6:34 p.m. UTC | #2 
On Wed, Feb 25, 2015 at 7:43 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi Eric and Richard,
>
> On Tue, Jan 06, 2015 at 06:07:12PM +0100, Eric Botcazou wrote:
>> Martin,
>>
>> > I suppose that could be done by something like the following, which I
>> > have tested only very mildly so far, in particular I have not double
>> > checked that get_inner_reference is cfun-agnostic.
>>
>> The patch introduces no regressions on x86-64/Linux and makes the testcase
>> (gnat.dg/specs/pack12.ads attached to the first message) pass.
>>
>> Do you plan to install it (along with the testcase)?
>>
>
> for various reasons I was not able to do it earlier, but today I have
> re-bootstrapped the following (the only change is the added testcase)
> on x86_64-linux and it passes OK.  Should I commit it to trunk then?
>
> Thanks,
>
> Martin
>
>
> 2015-02-25  Martin Jambor  <mjambor@suse.cz>
>             Eric Botcazou  <ebotcazou@adacore.com>
>
> gcc/
>         * tree-sra.c (ipa_sra_check_caller_data): New type.
>         (has_caller_p): Removed.
>         (ipa_sra_check_caller): New function.
>         (ipa_sra_preliminary_function_checks): Use it.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65305
diff mbox

Patch

diff --git a/gcc/testsuite/gnat.dg/specs/pack12.ads b/gcc/testsuite/gnat.dg/specs/pack12.ads
new file mode 100644
index 0000000..c5e962c
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/specs/pack12.ads
@@ -0,0 +1,21 @@ 
+-- { dg-do compile }
+-- { dg-options "-O2" }
+
+package Pack12 is
+
+  type Rec1 is record
+    B : Boolean;
+    N : Natural;
+  end record;
+
+  type Rec2 is record
+    B : Boolean;
+    R : Rec1;
+  end record;
+  pragma Pack (Rec2);
+
+  type Rec3 is tagged record
+    R : Rec2;
+  end record;
+
+end Pack12;
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 023b817..a6cddaf 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -5009,13 +5009,54 @@  modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
   return cfg_changed;
 }
 
-/* If NODE has a caller, return true.  */
+/* Means of communication between ipa_sra_check_caller and
+   ipa_sra_preliminary_function_checks.  */
+
+struct ipa_sra_check_caller_data
+{
+  bool has_callers;
+  bool bad_arg_alignment;
+};
+
+/* If NODE has a caller, mark that fact in DATA which is pointer to
+   ipa_sra_check_caller_data.  Also check all aggregate arguments in all known
+   calls if they are unit aligned and if not, set the appropriate flag in DATA
+   too. */
 
 static bool
-has_caller_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
+ipa_sra_check_caller (struct cgraph_node *node, void *data)
 {
-  if (node->callers)
-    return true;
+  if (!node->callers)
+    return false;
+
+  struct ipa_sra_check_caller_data *iscc;
+  iscc = (struct ipa_sra_check_caller_data *) data;
+  iscc->has_callers = true;
+
+  for (cgraph_edge *cs = node->callers; cs; cs = cs->next_caller)
+    {
+      gimple call_stmt = cs->call_stmt;
+      unsigned count = gimple_call_num_args (call_stmt);
+      for (unsigned i = 0; i < count; i++)
+	{
+	  tree arg = gimple_call_arg (call_stmt, i);
+	  if (is_gimple_reg (arg))
+	      continue;
+
+	  tree offset;
+	  HOST_WIDE_INT bitsize, bitpos;
+	  machine_mode mode;
+	  int unsignedp, volatilep = 0;
+	  get_inner_reference (arg, &bitsize, &bitpos, &offset, &mode,
+			       &unsignedp, &volatilep, false);
+	  if (bitpos % BITS_PER_UNIT)
+	    {
+	      iscc->bad_arg_alignment = true;
+	      return true;
+	    }
+	}
+    }
+
   return false;
 }
 
@@ -5070,14 +5111,6 @@  ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
-  if (!node->call_for_symbol_thunks_and_aliases (has_caller_p, NULL, true))
-    {
-      if (dump_file)
-	fprintf (dump_file,
-		 "Function has no callers in this compilation unit.\n");
-      return false;
-    }
-
   if (cfun->stdarg)
     {
       if (dump_file)
@@ -5096,6 +5129,25 @@  ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
+  struct ipa_sra_check_caller_data iscc;
+  memset (&iscc, 0, sizeof(iscc));
+  node->call_for_symbol_thunks_and_aliases (ipa_sra_check_caller, &iscc, true);
+  if (!iscc.has_callers)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "Function has no callers in this compilation unit.\n");
+      return false;
+    }
+
+  if (iscc.bad_arg_alignment)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "A function call has an argument with non-unit alignemnt.\n");
+      return false;
+    }
+
   return true;
 }