Re: OCTETS_PER_BYTE and Gas

[Nick Clifton]

Hi Dan,

> The assembler support structure provided by the GNU assembler has worked 
> quite well for this circumstance, with only a couple minor changes that 
> I would like to propose. 

Thanks for sending us this patch.  I have decided to apply the changes to 
the alignment functions, but not to the LEB128 encoding functions.  The 
reason for this is that the LEB128 functions currently pack only a single
octet into a byte, which is terribly wasteful for targets such as yours,
but this is how the DWARF spec appears to define the encoding operation.
Changing the functions to pack multiple octets into a single byte (where
possible) would be a much more extensive change, and probably contravene
the DWARF standard.


> Without these changes, the assembler would crash with some strange bugs.

I would be interested to know more about these crashes.

Note - we already support two targets where OCTETS_PER_BYTE_POWER is 2 
(tic4x and tic54x), so I would hope that in general we have the octets
vs bytes thing sorted out.


Attached is the patch that I am going to apply.

Cheers
  Nick

gas/ChangeLog
2016-02-18  Dan Gisselquist  
Nick Clifton  

* read.c (finish_bundle): Avoid recording a negative alignment.
(do_align): Use unsigned values for n, len and max.  Only create
a frag if the alignment requirement is greater than the minimum
byte alignment.  Avoid recording a negative alignment.
(s_align): Use unsigned values where appropriate.
(bss_alloc): Use an unsigned value for the alignment.
(sizeof_sleb128): Add a comment noting that we encode one octet
per byte, regardless of the value of OCTETS_PER_BYTE_POWER.
(emit_leb129_expr): Abort if the emitted encoding was longer than
expected.
* read.h (output_leb128): Update prototype.
(sizeof_leb128): Update prototype.
(bss_alloc): Update prototype.
* write.c (record_alignment): Use an unsigned value for the
alignment.  Do not record alignments less than the minimum
alignment for a byte.
* write.h (record_alignment): Update prototype.

diff --git a/gas/read.c b/gas/read.c
index 5c04789..f8fff78 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -238,7 +238,6 @@ static unsigned int bundle_lock_depth;
 #endif
 
 static void do_s_func (int end_p, const char *default_prefix);
-static void do_align (int, char *, int, int);
 static void s_align (int, int);
 static void s_altmacro (int);
 static void s_bad_end (int);
@@ -685,7 +685,8 @@ finish_bundle (fragS *frag, unsigned int size)
   /* We do this every time rather than just in s_bundle_align_mode
  so that we catch any affected section without needing hooks all
  over for all paths that do section changes.  It's cheap enough.  */
-  record_alignment (now_seg, bundle_align_p2 - OCTETS_PER_BYTE_POWER);
+  if (bundle_align_p2 > OCTETS_PER_BYTE_POWER)
+record_alignment (now_seg, bundle_align_p2 - OCTETS_PER_BYTE_POWER);
 }
 
 /* Assemble one instruction.  This takes care of the bundle features
@@ -736,6 +737,75 @@ single instruction is %u bytes long but .bundle_align_mode limit is %u"),
 
 #endif  /* HANDLE_BUNDLE */
 
+static bfd_boolean
+in_bss (void)
+{
+  flagword flags = bfd_get_section_flags (stdoutput, now_seg);
+
+  return (flags & SEC_ALLOC) && !(flags & (SEC_LOAD | SEC_HAS_CONTENTS));
+}
+
+/* Guts of .align directive:
+   N is the power of two to which to align.  A value of zero is accepted but
+ignored: the default alignment of the section will be at least this.
+   FILL may be NULL, or it may point to the bytes of the fill pattern.
+   LEN is the length of whatever FILL points to, if anything.  If LEN is zero
+but FILL is not NULL then LEN is treated as if it were one.
+   MAX is the maximum number of characters to skip when doing the alignment,
+or 0 if there is no maximum.  */
+
+static void
+do_align (unsigned int n, char *fill, unsigned int len, unsigned int max)
+{
+  if (now_seg == absolute_section || in_bss ())
+{
+  if (fill != NULL)
+	while (len-- > 0)
+	  if (*fill++ != '\0')
+	{
+	  if (now_seg == absolute_section)
+		as_warn (_("ignoring fill value in absolute section"));
+	  else
+		as_warn (_("ignoring fill value in section `%s'"),
+			 segment_name (now_seg));
+	  break;
+	}
+  fill = NULL;
+  len = 0;
+}
+
+#ifdef md_flush_pending_output
+  md_flush_pending_output ();
+#endif
+
+#ifdef md_do_align
+  md_do_align (n, fill, len, max, just_record_alignment);
+#endif
+
+  /* Only make a frag if we HAVE to...  */
+  if ((n > OCTETS_PER_BYTE_POWER) && !need_pass_2)
+{
+  if (fill == NULL)
+	{
+	  if (subseg_text_p (now_seg))
+	frag_align_code (n, max);
+	  else
+	frag_align (n, 0, max);
+	}
+  else if (len <= 1)
+	frag_align (n, *fill, max);
+  else
+	frag_align_pattern (n, fill, len, max);
+}
+
+#ifdef md_do_align
+ 

OCTETS_PER_BYTE and Gas

[Dan]

To those who maintain the GNU assembler, thank you.

I am currently working on a back-end implementation of the assembler to
an embedded system where OCTETS_PER_BYTE=4.  From a C standpoint, you
might think of it as a system where sizeof(char)==sizeof(int), and both
are 32-bits.

The embedded system is quite minimal in its support, therefore I don't
expect to run the assembler from the embedded system itself.  I'm just
building a cross-assembler to support this target from an i86_64
machine.  

The assembler support structure provided by the GNU assembler has worked
quite well for this circumstance, with only a couple minor changes that
I would like to propose.  Without these changes, the assembler would
crash with some strange bugs.  Therefore, I thought I'd push these
upstream.

Admittedly, my full implementation requires lots of other changes
elsewhere, such as in the testsuite, configuration files, and more.
These changes, though, should be applicable to anyone else working in
this type of situation.

Below is the diff for gas/read.c:

--- binutils-2.25-original/gas/read.c   2014-10-14 03:32:03.0
-0400
+++ binutils-2.25/gas/read.c2016-02-05 06:48:11.911995367 -0500
@@ -684,7 +684,8 @@
   /* We do this every time rather than just in s_bundle_align_mode
  so that we catch any affected section without needing hooks all
  over for all paths that do section changes.  It's cheap enough.
*/
-  record_alignment (now_seg, bundle_align_p2 - OCTETS_PER_BYTE_POWER);
+  if (bundle_align_p2 > OCTETS_PER_BYTE_POWER)
+record_alignment (now_seg, bundle_align_p2 -
OCTETS_PER_BYTE_POWER);
 }
 
 /* Assemble one instruction.  This takes care of the bundle features
@@ -1394,6 +1395,9 @@
 static void
 do_align (int n, char *fill, int len, int max)
 {
+  if (n < OCTETS_PER_BYTE_POWER)
+n = OCTETS_PER_BYTE_POWER;
+
   if (now_seg == absolute_section)
 {
   if (fill != NULL)
@@ -1415,7 +1419,7 @@
 #endif
 
   /* Only make a frag if we HAVE to...  */
-  if (n != 0 && !need_pass_2)
+  if ((n >= OCTETS_PER_BYTE_POWER) && !need_pass_2)
 {
   if (fill == NULL)
{
@@ -1434,7 +1438,8 @@
  just_record_alignment: ATTRIBUTE_UNUSED_LABEL
 #endif
 
-  record_alignment (now_seg, n - OCTETS_PER_BYTE_POWER);
+  if (n > OCTETS_PER_BYTE_POWER)
+record_alignment (now_seg, n - OCTETS_PER_BYTE_POWER);
 }
 
 /* Handle the .align pseudo-op.  A positive ARG is a default alignment
@@ -4927,6 +4932,8 @@
   while (!(((value == 0) && ((byte & 0x40) == 0))
   || ((value == -1) && ((byte & 0x40) != 0;
 
+  if (OCTETS_PER_BYTE_POWER > 0)
+size = (size +
(1< 0)
+size = (size +
(1< 0)
+size = (size +
(1< 0)
+size = (size +
(1<