code_pack issues

View: New views
4 Messages — Rating Filter:   Alert me  

code_pack issues

by Michael Ballbach :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

I've been working on a project that uses code_pack: it #include's a
generated HID descriptor in a code_pack section, and this was failing at
link time due to the debug info (in the object file) being off in offset
because of it being non word aligned. The attached patch (against svn
trunk) fixes that. It's a bit interesting, it'll end up adding line
entries for non-word aligned addresses, which wouldn't have ever
happened before, but which I think is 'more obviously correct'. The
patch to coff.c is the only part of the attached patch for this issue.

In addition, listing was broken when you had labels between non-aligned
words (with no code generating statement attached); it looks like the
listing code had been refactored in svn and this cropped up. The bug
manifests as an infinite loop while assembling. I fixed this, and in the
patch there's a new test, code_pack2.asm, which acts as a regression
test for this. The second part of the lst.c patch is for this issue,
along with making sure the address the list outputs for a label on its
own line is correct when not word aligned.

I also changed the listing for code_pack sections to display
byte-by-byte all the time. I started down that road because there was
another listing bug grabbing the wrong byte out of a word when writing
the list, though I can't remember which one... So if you don't take the
listing changes, you should probably look at the listing for the
code_pack2.asm file and make sure it's OK. This part of the patch also
monkeys with how the spacing is written to the byte section of a listing
line. This is the first part of that lst.c patch.

Finally, the code_pack.o in the testsuite would need to be 'blessed'
from the patched assembler before the testsuite will verify, because
'technically' the current object file has some incorrect line number
address pairings.

Thanks for everyone's work on gputils and take care,
--
Michael Ballbach, N0ZTQ
ballbach@... -- PGP KeyID: 0xA05D5555
http://www.rten.net/

diff -uNr -x entries gputils-svn.orig/gputils/gpasm/coff.c gputils-svn/gputils/gpasm/coff.c
--- gputils-svn.orig/gputils/gpasm/coff.c 2008-11-28 23:41:12.000000000 -0500
+++ gputils-svn/gputils/gpasm/coff.c 2008-11-28 23:47:30.000000000 -0500
@@ -361,6 +361,10 @@
   int i;
   int origin;
   static gp_boolean show_bad_debug = true;
+  gp_boolean emitted_pack_byte;
+
+  /* note if we're doing code_pack work */
+  emitted_pack_byte = state.obj.section && state.obj.section->emitted_pack_byte;
 
   if ((!state.obj.enabled) || (state.obj.section == NULL))
     return;
@@ -381,7 +385,7 @@
     return;
   }
 
-  for (i = 0; i < emitted; i++) {
+  for (i = 0; i < emitted + (emitted_pack_byte ? 1 : 0); i++) {
     
     new = gp_coffgen_addlinenum(state.obj.section);
     if (state.debug_info) {
@@ -391,7 +395,14 @@
       new->symbol = state.src->file_symbol;
       new->line_number = state.src->line_number;
     }
-    new->address = origin + (i << _16bit_core);
+
+    /* when emitting non-word aligned data, we must modify
+     * the origin address if our initial emission was a byte
+     * in an existing word. subsequent bytes/words then need
+     * to subtract 2, to compensate for the fact that the
+     * origin was off by a whole word. */
+    new->address = origin + (i << _16bit_core)
+        - (emitted_pack_byte ? (i == 0 ? 1 : 2) : 0);
   }
 
   return;
diff -uNr -x entries gputils-svn.orig/gputils/gpasm/lst.c gputils-svn/gputils/gpasm/lst.c
--- gputils-svn.orig/gputils/gpasm/lst.c 2008-11-28 23:41:12.000000000 -0500
+++ gputils-svn/gputils/gpasm/lst.c 2008-11-28 23:33:20.000000000 -0500
@@ -218,36 +218,56 @@
   char buf[BUFSIZ];
   unsigned int i;
   unsigned int lst_bytes = 0;
+  size_t m_spacing, m_prev_len = strlen(m), m_after_len;
 
-  if ((byte_org & 1) != 0) {
-    /* not word-aligned */
-    /* list first byte */
-    unsigned char emit_byte = (unsigned char)(i_memory_get(state.i_memory,
-        (byte_org >> 1)) >> 8);
-    snprintf(buf, sizeof(buf), "%02X", emit_byte);
-    strncat(m, buf, sizeof_m);
-    ++lst_bytes;
+  if (bytes_emitted == 0)
+    return 0;
+
+  /* when in a byte packed section, print byte by byte */
+  if (state.obj.new_sec_flags & STYP_BPACK) {
+    gp_boolean started_odd = (byte_org & 1) != 0;
+
+    if (started_odd) {
+      /* not word-aligned */
+      /* list first byte */
+      unsigned char emit_byte = (unsigned char)(i_memory_get(state.i_memory,
+          (byte_org >> 1)) >> 8);
+      snprintf(buf, sizeof(buf), "%02X ", emit_byte);
+      strncat(m, buf, sizeof_m);
+      ++lst_bytes;
+    } else if (bytes_emitted == 1) {
+      /* word-aligned */
+      /* but only one byte */
+      unsigned char emit_byte = (unsigned char)(i_memory_get(state.i_memory,
+          (byte_org >> 1)) & 0xff);
+      snprintf(buf, sizeof(buf), "%02X ", emit_byte);
+      strncat(m, buf, sizeof_m);
+      ++lst_bytes;
+    }
     /* list whole words */
-    for (i = 0; (i < ((bytes_emitted-1) >> 1)) && (i < 1); ++i) {
+    for (i = 0; (i < ((bytes_emitted-started_odd) >> 1)) && (i < 1); ++i) {
       unsigned int emit_word = i_memory_get(state.i_memory,
-          ((byte_org+1) >> 1) + i) & 0xffff;
-      snprintf(buf, sizeof(buf), "%02X %02X", emit_word & 0x00ff,
+          ((byte_org+started_odd) >> 1) + i) & 0xffff;
+      snprintf(buf, sizeof(buf), "%02X %02X ", emit_word & 0x00ff,
           emit_word >> 8);
       strncat(m, buf, sizeof_m);
       lst_bytes += 2;
     }
-    /* list extra byte if odd */
-    if (((byte_org+bytes_emitted) & 1) != 0) {
-      snprintf(buf, sizeof(buf), "%02X ", i_memory_get(state.i_memory,
-          ((byte_org + bytes_emitted - 2) >> 1)) & 0x00ff);
+    if (i != 1 && started_odd && bytes_emitted > 1) {
+      /* we have space for an extra byte if we had no whole word and we started odd */
+      snprintf(buf, sizeof(buf), "%02X", i_memory_get(state.i_memory,
+          ((byte_org + 1) >> 1)) & 0xff00 >> 8);
+      strncat(m, buf, sizeof_m);
+      ++lst_bytes;
+    } else if (!started_odd && i == 1 && bytes_emitted > 2) {
+      /* we have space for another byte, word aligned */
+      snprintf(buf, sizeof(buf), "%02X", i_memory_get(state.i_memory,
+          (byte_org + 2) >> 1) & 0xff);
       strncat(m, buf, sizeof_m);
       ++lst_bytes;
-    }
-    else {
-      strncat(m, "   ", sizeof_m);
     }
   }
-  else {    /* word-aligned */
+  else {    /* non-code pack section */
     /* list full words as bytes */
     for (i = 0; (i < (bytes_emitted >> 1)) && (i < 2); ++i) {
       unsigned int emit_word = i_memory_get(state.i_memory,
@@ -270,6 +290,13 @@
     }
   }
 
+  /* append appropriate spacing */
+  m_after_len = strlen(m);
+  m_spacing = m_after_len - m_prev_len;
+  for(; m_spacing < 10; m_spacing++) {
+    m[m_after_len++] = ' ';
+  }
+  m[m_after_len] = '\0';
   return lst_bytes;
 }
 
@@ -307,10 +334,22 @@
     if (state.obj.section)
       byte_org -= (state.obj.section->emitted_pack_byte ? 1 : 0);
     bytes_emitted = (state.org << 1) - byte_org;
-    if (state.obj.section)
-      bytes_emitted -= (state.obj.section->have_pack_byte ? 1 : 0);
+    
+    /* deal with offset changes for non-word aligned data */
+    if (state.obj.section) {
+      /* do we have a pending byte for a full word? */
+      if (state.obj.section->have_pack_byte) {
+        /* did we emit some bytes? if so we have to subtract a half word */
+        if (bytes_emitted > 0 && state.obj.section->have_pack_byte)
+          bytes_emitted--;
+        /* is this a label or something with no instructions? then our
+         * org must be modified. */
+        else if (bytes_emitted == 0)
+          byte_org--;
+      }
+    }
     emitted = (bytes_emitted >> 1);
-    if (((byte_org & 1) == 0) && ((bytes_emitted & 1) != 0))
+    if (bytes_emitted > 0 && ((byte_org & 1) == 0) && ((bytes_emitted & 1) != 0))
       emitted += 1;
     snprintf(m, sizeof(m), "%04X ", byte_org >> (1 - _16bit_core));
 
diff -uNr -x entries gputils-svn.orig/gputils/gpasm/testsuite/gpasm.project/objasm/code_pack2.asm gputils-svn/gputils/gpasm/testsuite/gpasm.project/objasm/code_pack2.asm
--- gputils-svn.orig/gputils/gpasm/testsuite/gpasm.project/objasm/code_pack2.asm 1969-12-31 19:00:00.000000000 -0500
+++ gputils-svn/gputils/gpasm/testsuite/gpasm.project/objasm/code_pack2.asm 2008-11-28 23:47:56.000000000 -0500
@@ -0,0 +1,67 @@
+; this is my test file for the code_pack gpasm patch
+
+;
+; the idea is to test as many permutations of packed data as possible,
+; and to make sure that the label addresses generated are always correct.
+;
+
+            list p=18f2455
+
+blech       code_pack   0x100
+
+label0
+            db 1
+label1
+            db 2
+label2
+            db 3, 4
+label4
+            db 5
+label5
+            db 6, 7, 8
+label8
+            db 9, 0xa, 0xb
+label0xb
+            db 0xc
+label0xc
+            db 0xd, 0xe, 0xf, 0x10
+label0x10
+            db 0x11, 0x12, 0x13, 0x14, 0x15
+label0x15  
+            db 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b
+label0x1b
+            db 0x1c
+label0x1c
+            db 0x1d, 0x1e
+label0x1e
+            db 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24
+label0x24
+            db 0x25
+label0x25
+            db 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c
+label0x2c
+            db 0x2d
+
+blech2      code_pack   0x200
+
+blabel0 db 1
+blabel1 db 2
+blabel2 db 3, 4
+blabel4 db 5
+blabel5 db 6, 7, 8
+blabel8 db 9, 0xa, 0xb
+blabel0xb db 0xc
+blabel0xc db 0xd, 0xe, 0xf, 0x10
+blabel0x10 db 0x11, 0x12, 0x13, 0x14, 0x15
+blabel0x15 db 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b
+blabel0x1b db 0x1c
+blabel0x1c db 0x1d, 0x1e
+blabel0x1e db 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24
+blabel0x24 db 0x25
+blabel0x25 db 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c
+blabel0x2c db 0x2d
+
+blech3      code
+            movlw   0
+            end
+
Files gputils-svn.orig/gputils/gpasm/testsuite/gpasm.project/objfiles/code_pack2.o and gputils-svn/gputils/gpasm/testsuite/gpasm.project/objfiles/code_pack2.o differ
Files gputils-svn.orig/gputils/gpasm/testsuite/gpasm.project/objfiles/code_pack.o and gputils-svn/gputils/gpasm/testsuite/gpasm.project/objfiles/code_pack.o differ


attachment0 (196 bytes) Download Attachment

Re: code_pack issues

by David Barnett-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

These are great changes. I don't have any objections to listing the
code_pack sections byte-by-byte if that works for everyone. I can't imagine
it would cause any problems.

Would I be right in assuming that the infinite loop bug is the same as our
bug #1988473?(
https://sourceforge.net/tracker/?func=detail&atid=431665&aid=1988473&group_id=41924
)

Thanks a bunch for the effort!
David

Re: code_pack issues

by Michael Ballbach :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Nov 30, 2008 at 09:35:34AM -0500, David Barnett wrote:
> These are great changes. I don't have any objections to listing the
> code_pack sections byte-by-byte if that works for everyone. I can't
> imagine it would cause any problems.

For comparison, it's what MPASM does. The only way we differ is they use
15 characters for this 'field' in the output, and we use only 10.

> Would I be right in assuming that the infinite loop bug is the same as
> our bug #1988473?(
> https://sourceforge.net/tracker/?func=detail&atid=431665&aid=1988473&group_id=41924
> )

Yes, definitely. I would have looked at this earlier had I known it was
an issue, I'm sorry to say that while I subscribe to the list I am not
very attentive when I am busy with other things. Do I need to sign-up in
there or something? I would do so if you wanted to assign bugs like that
to me in the future.

--
Michael Ballbach, N0ZTQ
ballbach@... -- PGP KeyID: 0xA05D5555
http://www.rten.net/


attachment0 (196 bytes) Download Attachment

Re: code_pack issues

by David Barnett-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Nov 30, 2008 at 2:31 PM, Michael Ballbach <ballbach@...> wrote:

> On Sun, Nov 30, 2008 at 09:35:34AM -0500, David Barnett wrote:
> > Would I be right in assuming that the infinite loop bug is the same as
> > our bug #1988473?(
> >
> https://sourceforge.net/tracker/?func=detail&atid=431665&aid=1988473&group_id=41924
> > )
>
> Yes, definitely. I would have looked at this earlier had I known it was
> an issue, I'm sorry to say that while I subscribe to the list I am not
> very attentive when I am busy with other things. Do I need to sign-up in
> there or something? I would do so if you wanted to assign bugs like that
> to me in the future.

No, that's quite alright. I just wanted to double-check I should close that
ticket when I commit the patch. You've helped a bunch already.

David