PR 49169: testing the alignment of a function

Richard Sandiford rdsandiford@googlemail.com
Wed Jun 29 08:40:00 GMT 2011


"H.J. Lu" <hjl.tools@gmail.com> writes:
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49545

Sorry for the breakage.  I should obviously have tested on x86_64 as well.

To recap, there are (at least) two concepts of what "the address of X"
can mean.  It can mean (a) the address at which X is actually located
or (b) the value to which symbol X resolves.  The problem is that these
two are different for things like Thumb and MIPS16 functions.

The DECL_ALIGN on a function should be (a).  For example, if we have:

  void __attribute__((aligned(16))) foo (void) { ... }

then foo() really ought to start on a 16-byte boundary, even for Thumb.
However, most parts of GCC want (b).  In particular, if we want to know
the alignment of an ADDR_EXPR, or the alignment of a tree or RTL memory
reference, the address is going to come from symbol resolution.

As far as I can tell, all uses of get_object_alignment* want (b).
In particular, get_object_alignment* isn't used for __alignof__,
which might have been one case where (a) made more sense.
(Any thoughts on what __alignof__ should mean here?  Either way,
it's a separate patch.)

So DECL_ALIGN gives (a) rather than (b).  But as the PR shows,
there are cases where we know (and need to know) that (b) >= 2 bytes,
namely whenever TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn.
This patch uses that to decide whether &function has 1 or 2 bytes of
alignment.

Note that this isn't just about ARM and MIPS.  Some targets define
FUNCTION_BOUNDARY in a way that depends on optimisation flags,
whereas get_object_alignment previously treated it as an ABI property.
It's definitely arguable that those targets are buggy and should be
using align_functions instead, but the docs aren't really clear.

That's why I'm still not going for a target hook at this stage.
It's just too hard to say in general whether a port's current
FUNCTION_BOUNDARY is guaranteed by the ABI or not, so I think
the default definition of any hook would still be the condition
that I've used here.  I think it makes sense to leave it like
this until someone is motivated to guarantee greater alignment.

Tested on x86_64-linux-gnu and arm-linux-gnueabi.  OK to install?

Richard


gcc/
	PR tree-optimization/49545
	* builtins.c (get_object_alignment_1): Update function comment.
	Do not use DECL_ALIGN for functions, but test
	TARGET_PTRMEMFUNC_VBIT_LOCATION instead.
	* fold-const.c (get_pointer_modulus_and_residue): Don't check
	for functions here.
	* tree-ssa-ccp.c (get_value_from_alignment): Likewise.

gcc/testsuite/
	* gcc.dg/torture/pr49169.c: Restrict to ARM and MIPS targets.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	2011-06-28 14:06:19.000000000 +0100
+++ gcc/builtins.c	2011-06-29 09:07:00.000000000 +0100
@@ -264,8 +264,15 @@ called_as_built_in (tree node)
   return is_builtin_name (name);
 }
 
-/* Return the alignment in bits of EXP, an object.
-   Don't return more than MAX_ALIGN no matter what.  */
+/* Compute values M and N such that M divides (address of EXP - N) and
+   such that N < M.  Store N in *BITPOSP and return M.
+
+   Note that the address (and thus the alignment) computed here is based
+   on the address to which a symbol resolves, whereas DECL_ALIGN is based
+   on the address at which an object is actually located.  These two
+   addresses are not always the same.  For example, on ARM targets,
+   the address &foo of a Thumb function foo() has the lowest bit set,
+   whereas foo() itself starts on an even address.  */
 
 unsigned int
 get_object_alignment_1 (tree exp, unsigned HOST_WIDE_INT *bitposp)
@@ -287,7 +294,21 @@ get_object_alignment_1 (tree exp, unsign
     exp = DECL_INITIAL (exp);
   if (DECL_P (exp)
       && TREE_CODE (exp) != LABEL_DECL)
-    align = DECL_ALIGN (exp);
+    {
+      if (TREE_CODE (exp) == FUNCTION_DECL)
+	{
+	  /* Function addresses can encode extra information besides their
+	     alignment.  However, if TARGET_PTRMEMFUNC_VBIT_LOCATION
+	     allows the low bit to be used as a virtual bit, we know
+	     that the address itself must be 2-byte aligned.  */
+	  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)
+	    align = 2 * BITS_PER_UNIT;
+	  else
+	    align = BITS_PER_UNIT;
+	}
+      else
+	align = DECL_ALIGN (exp);
+    }
   else if (CONSTANT_CLASS_P (exp))
     {
       align = TYPE_ALIGN (TREE_TYPE (exp));
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	2011-06-28 14:06:19.000000000 +0100
+++ gcc/fold-const.c	2011-06-29 08:59:02.000000000 +0100
@@ -9216,8 +9216,7 @@ get_pointer_modulus_and_residue (tree ex
   *residue = 0;
 
   code = TREE_CODE (expr);
-  if (code == ADDR_EXPR
-      && TREE_CODE (TREE_OPERAND (expr, 0)) != FUNCTION_DECL)
+  if (code == ADDR_EXPR)
     {
       unsigned int bitalign;
       bitalign = get_object_alignment_1 (TREE_OPERAND (expr, 0), residue);
Index: gcc/tree-ssa-ccp.c
===================================================================
--- gcc/tree-ssa-ccp.c	2011-06-28 14:06:19.000000000 +0100
+++ gcc/tree-ssa-ccp.c	2011-06-29 08:59:02.000000000 +0100
@@ -520,10 +520,6 @@ get_value_from_alignment (tree expr)
     val = bit_value_binop (PLUS_EXPR, TREE_TYPE (expr),
 			   TREE_OPERAND (base, 0), TREE_OPERAND (base, 1));
   else if (base
-	   /* ???  While function decls have DECL_ALIGN their addresses
-	      may encode extra information in the lower bits on some
-	      targets (PR47239).  Simply punt for function decls for now.  */
-	   && TREE_CODE (base) != FUNCTION_DECL
 	   && ((align = get_object_alignment (base, BIGGEST_ALIGNMENT))
 		> BITS_PER_UNIT))
     {
Index: gcc/testsuite/gcc.dg/torture/pr49169.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr49169.c	2011-06-29 08:58:51.000000000 +0100
+++ gcc/testsuite/gcc.dg/torture/pr49169.c	2011-06-29 09:00:41.000000000 +0100
@@ -1,3 +1,5 @@
+/* { dg-do compile { target { arm*-*-* || mips*-*-* } } } */
+
 #include <stdlib.h>
 #include <stdint.h>
 



More information about the Gcc-patches mailing list