(patch) pragma pack lossage

Mumit Khan khan@xraylith.wisc.EDU
Tue Jun 29 13:33:00 GMT 1999


Background: 

All targets that support SYSV pragmas now also support #pragma pack(<n>). 
This code is handled by c-pragma.c:handle_pragma_token() that sets the
value of maximum_field_alignment used for aligning and layout out the 
fields.

In addition, some targets also support stacking these pragmas packing,
and is turned on by HANDLE_PRAGMA_PACK_PUSH_POP. When this is defined, 
c-pragma.h also defines INSERT_PACK_ATTRIBUTES to be 
insert_pack_attributes(), which is called from c-common.c:decl_attributes().

Now, this INSERT_PACK_ATTRIBUTES is only defined for push/pop targets,
and not for all the targets that support pragma pack(). This is truly
inconsistent; on top of that, insert_pack_attributes() is simply not
correct. It adds packed and aligned attributes to affected decls that
is not necessary; in fact, it breaks just about every structure with 
bitfields in it. It also doesn't executed when a pragma pack(<n>) is 
executed (see the first conditional, which fails whenver the stack is 
empty), so the bug only shows up when you're using push/pop instead of 
the global pack(<n>).

[ Sidebar: There is an issue of native vs gcc style bitfield packing; 
  eg., GCC style is incompatible with windows32 native, and Donn Terry 
  has just submitted a patch to support both the native/gcc styles. 
  The topic here is largely independent from that issue. ]

My vote to get rid of this code. If anyone thinks this code does the 
right thing, I want to know why and definitely want to see some 
testcases.

The second bug is simple: pragma pack(<n>) sets the *default* alignment
to be <n>, and if we're popping out of the alignment stack, set the
alignment to be the default, not to 0.

Anyone wishing to test this and want my rather extensive testcases, please 
drop me a line.

This patch, combined with Donn Terry's latest on bitfield packing, now
correctly build all the testcases that I have compiled so far.

Tue Jun 29 13:52:44 1999  Mumit Khan  <khan@xraylith.wisc.edu>

	* c-pragma.h (PRAGMA_INSERT_ATTRIBUTES): Delete macro.
	(insert_pack_attributes): Delete prototype.

	* c-pragma.c (default_alignment): New static variable.
	(push_alignment): Initialize to current effective alignment.
	(pop_alignment): Use to set new alignment.
	(insert_pack_attributes): Delete function.
	(handle_pragma_token): Set default_alignment as well each time 
	a #pragma pack(<n>) is encountered.

Index: c-pragma.h
===================================================================
RCS file: /homes/khan/src/CVSROOT/gcc-2.95/gcc/c-pragma.h,v
retrieving revision 1.1.1.1
diff -u -3 -p -r1.1.1.1 c-pragma.h
--- c-pragma.h	1999/06/14 21:05:17	1.1.1.1
+++ c-pragma.h	1999/06/29 18:50:37
@@ -39,9 +39,6 @@ Boston, MA 02111-1307, USA.  */
 /* If we are supporting #pragma pack(push... then we automatically
    support #pragma pack(<n>)  */
 #define HANDLE_PRAGMA_PACK 1
-#define PRAGMA_INSERT_ATTRIBUTES(node, pattr, prefix_attr) \
-  insert_pack_attributes (node, pattr, prefix_attr)
-extern void insert_pack_attributes PROTO((tree, tree *, tree *));
 #endif /* HANDLE_PRAGMA_PACK_PUSH_POP */
 
 
Index: c-pragma.c
===================================================================
RCS file: /homes/khan/src/CVSROOT/gcc-2.95/gcc/c-pragma.c,v
retrieving revision 1.2
diff -u -3 -p -r1.2 c-pragma.c
--- c-pragma.c	1999/06/14 21:13:53	1.2
+++ c-pragma.c	1999/06/29 18:50:22
@@ -51,6 +51,13 @@ typedef struct align_stack
 
 static struct align_stack * alignment_stack = NULL;
 
+/* If we have a "global" #pragma pack(<n>) if effect when the first
+   #pragma push(pack,<n>) is encountered, this stores the the value of 
+   maximum_field_alignment in effect. When the final pop_alignment() 
+   happens, we restore the value to this, not to a value of 0 for
+   maximum_field_alignment. Value is in bits. */
+static int default_alignment;
+
 static int  push_alignment PROTO((int, tree));
 static int  pop_alignment  PROTO((tree));
 
@@ -94,6 +101,12 @@ Alignment must be a small power of two, 
       entry->num_pushes = 1;
       entry->id         = id;
       entry->prev       = alignment_stack;
+
+      /* The current value of maximum_field_alignment is not necessarily 
+	 0 since there may be a #pragma pack(<n>) in effect; remember it 
+	 so that we can restore it after the final #pragma pop(). */
+      if (alignment_stack == NULL)
+	default_alignment = maximum_field_alignment;
       
       alignment_stack = entry;
 
@@ -142,7 +155,7 @@ pop_alignment (id)
       entry = alignment_stack->prev;
 
       if (entry == NULL)
-	maximum_field_alignment = 0;
+	maximum_field_alignment = default_alignment;
       else
 	maximum_field_alignment = entry->alignment * 8;
 
@@ -153,67 +166,6 @@ pop_alignment (id)
 
   return 1;
 }
-
-/* Generate 'packed' and 'aligned' attributes for decls whilst a
-   #pragma pack(push... is in effect.  */
-void
-insert_pack_attributes (node, attributes, prefix)
-     tree node;
-     tree * attributes;
-     tree * prefix;
-{
-  tree a;
-  int field_alignment;
-
-  /* If we are not packing, then there is nothing to do.  */
-  if (maximum_field_alignment == 0
-      || alignment_stack == NULL)
-    return;
-
-  /* We are only interested in fields.  */
-  if (TREE_CODE_CLASS (TREE_CODE (node)) != 'd'
-      || TREE_CODE (node) != FIELD_DECL)
-    return;
-  
-  field_alignment = TYPE_ALIGN (TREE_TYPE (node));
-  if (field_alignment <= 0 || field_alignment > maximum_field_alignment)
-    field_alignment = maximum_field_alignment;
-
-  /* Add a 'packed' attribute.  */
-  * attributes = tree_cons (get_identifier ("packed"), NULL, * attributes);
-  
-  /* If the alignment is > 8 then add an alignment attribute as well.  */
-  if (field_alignment > 8)
-    {
-      /* If the aligned attribute is already present then do not override it.  */
-      for (a = * attributes; a; a = TREE_CHAIN (a))
-	{
-	  tree name = TREE_PURPOSE (a);
-	  if (strcmp (IDENTIFIER_POINTER (name), "aligned") == 0)
-	    break;
-	}
-      
-      if (a == NULL)
-	for (a = * prefix; a; a = TREE_CHAIN (a))
-	  {
-	    tree name = TREE_PURPOSE (a);
-	    if (strcmp (IDENTIFIER_POINTER (name), "aligned") == 0)
-	      break;
-	  }
-  
-      if (a == NULL)
-	{
-	  * attributes = tree_cons
-	      (get_identifier ("aligned"),
-	       tree_cons (NULL,
-			  build_int_2 (field_alignment / 8, 0),
-			  NULL),
-	       * attributes);
-	}
-    }
-
-  return;
-}
 #endif /* HANDLE_PRAGMA_PACK_PUSH_POP */
 
 /* Handle one token of a pragma directive.  TOKEN is the current token, and
@@ -263,6 +215,9 @@ handle_pragma_token (string, token)
 	  if (state == ps_right)
 	    {
 	      maximum_field_alignment = align * 8;
+#ifdef HANDLE_PRAGMA_PAC_PUSH_POP
+	      default_alignment = maximum_field_alignment;
+#endif
 	      ret_val = 1;
 	    }
 	  else

Regards,
Mumit


More information about the Gcc-patches mailing list