[gccsdk] Adding 'mkimage' to the autobuilder

Jeffrey Lee me at phlamethrower.co.uk
Sun Nov 28 07:16:20 PST 2010


On Wed, 29 Sep 2010, John Tytgat wrote:

> In message <Pine.WNT.4.62.1009292154140.2692 at c203>
>          Jeffrey Lee <me at phlamethrower.co.uk> wrote:
>
>> If you want I
>> can have a go at adding file writing support to unixlib's mmap()
>> implementation, but bear in mind that I'm not exactly an expert on how
>> mmap() is meant to behave.
>
> Seeing what you had to implement for mmap() for mkimage, we should move
> that to UnixLib.  A full implementation won't be possible but limited
> one (like this) should.  But we should return an error for all cases
> we don't have support for.  If you could do a first shot, I'm happy
> to review & commit.
>
> John.

It took me a while to get round to it, but attached is a patch which adds 
file writing support to UnixLib's mmap() implementation. Brief summary:

* Implemented within the existing DynamicArea-based solution
* msync() is supported for flushing changed areas to disc (although 
obviously it flushes all pages within the indicated region instead of just 
the modified ones)
* munmap() flushes changes too (even though none of the munmap() docs I've 
seen suggest that such a thing should happen, mkimage seems to rely on it)
* I've removed the check that was preventing mmap() from being used if the
main heap wasn't in a dynamic area. As far as I know there's no good 
reason for us to prevent additional dynamic areas from being used if the 
main heap isn't a DA.
* To keep things simple, I've disallowed the use of mremap() on mapped 
files. The old code didn't really support it properly anyway.

So far I've only tested this with mkimage, so if you know of any other 
software I can test then let me know.

Cheers,

- Jeffrey
-------------- next part --------------
Index: gcc4/recipe/files/gcc/libunixlib/include/sys/mman.h
===================================================================
--- gcc4/recipe/files/gcc/libunixlib/include/sys/mman.h	(revision 4819)
+++ gcc4/recipe/files/gcc/libunixlib/include/sys/mman.h	(working copy)
@@ -86,7 +86,6 @@
 /* Synchronize the region starting at ADDR and extending LEN bytes with the
    file it maps.  Filesystem operations on a file being mapped are
    unpredictable before this is done.  */
-/* Not supported.  */
 extern int msync (__caddr_t __addr, size_t __len) __THROW;
 
 /* Advise the system about particular usage patterns the program follows
Index: gcc4/recipe/files/gcc/libunixlib/sys/mman.c
===================================================================
--- gcc4/recipe/files/gcc/libunixlib/sys/mman.c	(revision 4819)
+++ gcc4/recipe/files/gcc/libunixlib/sys/mman.c	(working copy)
@@ -39,9 +39,12 @@
   int	    number;
   size_t    len;
   int	    prot;
+  int       flags;
+  int       fd;
+  off_t     offset;
 } mmap_info;
 
-#define IMMAP { 0, -1, 0, 0 }
+#define IMMAP { 0, -1, 0, 0, 0, -1, 0 }
 #define MAX_MMAPS 8
 static mmap_info mmaps[MAX_MMAPS] = {
   IMMAP, IMMAP, IMMAP, IMMAP, IMMAP, IMMAP, IMMAP, IMMAP
@@ -160,14 +163,6 @@
 
   PTHREAD_UNSAFE
 
-  /* Only support mmap on files for reading from zero offset.  */
-  if (fd != -1 && (prot != PROT_READ))
-    return (caddr_t) __set_errno (ENOSYS);
-
-  /* Non-zero offset only makes sense for mmap onto files.  */
-  if (offset != 0)
-    return (caddr_t) __set_errno (EINVAL);
-
   /* We don't support MAP_FIXED.  */
   if (flags & MAP_FIXED)
     return (caddr_t) __set_errno (((size_t)addr & (gbl->pagesize - 1)) ? EINVAL : ENOSYS);
@@ -176,18 +171,14 @@
   if (flags & MAP_COPY)
     return (caddr_t) __set_errno (ENOSYS);
 
-  /* We only support mmap when dynamic areas are enabled.  */
-  if (gbl->dynamic_num == -1)
-    return (caddr_t) __set_errno (ENOSYS);
+  /* We don't support MAP_PRIVATE. However, we will just use MAP_SHARED instead
+     rather than changing all the user code calling us. */
 
   /* We only support PROT_READ or PROT_WRITE.  */
   if (((prot & PROT_EXEC) == PROT_EXEC)
       || ((prot & (PROT_READ | PROT_WRITE)) == 0))
     return (caddr_t) __set_errno (ENOSYS);
 
-  /* We don't support MAP_PRIVATE. However, we will just use MAP_SHARED instead
-     rather than changing all the user code calling us. */
-
   /* find spare mmap_info index */
   for (i = 0; i < MAX_MMAPS; i++)
     {
@@ -200,74 +191,96 @@
     return (caddr_t) __set_errno (ENOMEM);
 
   len = page_align (gbl, len);
+
   /* Create the dynamic area.  */
   regs[0] = 0;
   regs[1] = -1;
   regs[2] = len;
   regs[3] = -1;
-  if (prot & PROT_READ)
-    {
-      if (prot & PROT_WRITE)
-	regs[4] = 0x80;
-      else
-	regs[4] = 0x81;
-    }
-  else
-    regs[4] = 0x82;
-
-  /* We allocate triple the requested memory as the virtual limit to help
-     with mremap which is used by realloc.  */
-  regs[5] = regs[2] * 3;
   regs[6] = 0;
   regs[7] = 0;
   /* The DA name could be the same name used in sys/_syslib.s so Virtualise
      can be enabled/disabled for the area. Using the name with 'X' appended
      is useful though to separate the areas from the more general purpose
      heap.  */
+  char namebuf[128];
+
+  if (&__dynamic_da_name)
+    regs[8] = (int) __dynamic_da_name;
+  else
   {
-    char namebuf[128];
+    regs[8] = (int)__get_program_name (__u->argv[0], namebuf, sizeof(namebuf) - sizeof(" MMap"));
+    strcat (namebuf, " MMap");
+  }
 
-    if (&__dynamic_da_name)
-      regs[8] = (int) __dynamic_da_name;
+  /* Check whether we're mapping a file or just memory */
+  if (fd != -1)
+  {
+    /* Always use read/write access, otherwise we won't be able to read/write the file data */
+    regs[4] = 0x80;
+
+    /* Assume that mmap'd files won't be enlarged very often */
+    regs[5] = regs[2];
+  }
+  else
+  {
+    /* Non-zero offset only makes sense for mmap onto files.  */
+    if (offset != 0)
+      return (caddr_t) __set_errno (EINVAL);
+
+    if (prot & PROT_READ)
+    {
+      if (prot & PROT_WRITE)
+        regs[4] = 0x80;
+      else
+        regs[4] = 0x81;
+    }
     else
-      {
-	regs[8] = (int)__get_program_name (__u->argv[0], namebuf,
-					   sizeof(namebuf) - sizeof(" MMap"));
-	strcat (namebuf, " MMap");
-      }
+      regs[4] = 0x82;
 
-    if (__os_swi (OS_DynamicArea, regs))
-      return (caddr_t) __set_errno (ENOMEM);
+    /* We allocate triple the requested memory as the virtual limit to help
+       with mremap which is used by realloc.  */
+    regs[5] = regs[2] * 3;
   }
 
+  if (__os_swi (OS_DynamicArea, regs))
+    return (caddr_t) __set_errno (ENOMEM);
+
   mmaps[i].number = regs[1];
   mmaps[i].addr	  = (caddr_t)regs[3];
   mmaps[i].len	  = len;
   mmaps[i].prot	  = prot;
+  mmaps[i].flags  = flags;
+  mmaps[i].fd     = fd;
+  mmaps[i].offset = offset;
 
-  /* Simplistic mmap file handling.  Simply read the entire file into memory */
-  if (fd != -1)
-    {
-      int count = 0;
+  if(fd == -1)
+    return mmaps[i].addr;
 
-      lseek(fd, 0, SEEK_SET);
+  /* Read in the relevant section of the file */
+  int count = 0;
 
-      while (count < len)
-        {
-          int size = read(fd, mmaps[i].addr + count, len - count);
+  off_t oldpos = lseek(fd, 0, SEEK_CUR);
 
-          if (size < 0)
-            {
-              munmap(mmaps[i].addr, len);
-              return (caddr_t) __set_errno (EINVAL);
-            }
-          else if (size == 0)
-            break;
+  lseek(fd, offset, SEEK_SET);
 
-          count += size;
-        }
+  while (count < len)
+  {
+    int size = read(fd, mmaps[i].addr + count, len - count);
+
+    if (size < 0)
+    {
+      munmap(mmaps[i].addr, len);
+      lseek(fd, oldpos, SEEK_SET);
+      return (caddr_t) __set_errno (EINVAL);
     }
+    else if (size == 0)
+      break;
 
+    count += size;
+  }
+  lseek(fd, oldpos, SEEK_SET);
+
   return mmaps[i].addr;
 }
 
@@ -282,8 +295,6 @@
 
   PTHREAD_UNSAFE
 
-  len = len; /* XXX don't care about len for now */
-
   for (i = 0; i < MAX_MMAPS; i++)
     {
       if (mmaps[i].addr == addr)
@@ -294,6 +305,19 @@
   if (i == MAX_MMAPS)
     return __set_errno (EINVAL);
 
+  /* Write back file contents if necessary
+     All the docs I've seen suggest that we only need to write
+     stuff back on msync(), but some software (e.g. mkimage)
+     seems to rely on the writeback occuring un munmap() */
+  if ((mmaps[i].fd != -1) && (mmaps[i].prot & PROT_WRITE) && !(mmaps[i].flags & MAP_PRIVATE))
+  {
+    /* Assume that the user only wants 'len' bytes writing */
+    off_t oldpos = lseek(mmaps[i].fd,0,SEEK_CUR);
+    lseek(mmaps[i].fd,mmaps[i].offset,SEEK_SET);
+    write(mmaps[i].fd,addr,len);
+    lseek(mmaps[i].fd,oldpos,SEEK_SET);
+  }
+
   /* Unmap by deleting the dynamic area.  */
   regs[0] = 1;
   regs[1] = mmaps[i].number;
@@ -325,9 +349,49 @@
 int
 msync (caddr_t addr, size_t len)
 {
-  addr = addr;
-  len = len;
-  return __set_errno (ENOSYS);
+  struct ul_global *gbl = &__ul_global;
+  int i;
+  PTHREAD_UNSAFE
+
+  for (i = 0; i < MAX_MMAPS; i++)
+    {
+      if ((mmaps[i].addr <= addr) && (mmaps[i].addr+mmaps[i].len >= addr))
+	break;
+    }
+
+  /* If we couldn't find the mmap mapping, then return an error.  */
+  if (i == MAX_MMAPS)
+    return __set_errno (EINVAL);
+
+  /* len needs to be positive */
+  if (len < 0)
+    return __set_errno (EINVAL);
+
+  /* Although it doesn't matter for us, still enforce the requirement for
+     addr to be page aligned */
+  if ((size_t)addr & (gbl->pagesize - 1))
+    return __set_errno (EINVAL);
+
+  /* Don't access areas outside the mapped range */
+  if (addr-mmaps[i].addr+len > mmaps[i].len)
+    return __set_errno (ENOMEM);
+
+  /* Only valid if this is a file open for non-private writing */
+  if ((mmaps[i].fd == -1) || !(mmaps[i].prot & PROT_WRITE) || (mmaps[i].flags & MAP_PRIVATE))
+    return __set_errno (EINVAL);
+
+  /* If no length specified, flush the whole mapping */
+  if(!len)
+  {
+    len = mmaps[i].len;
+    addr = mmaps[i].addr;
+  }
+  off_t oldpos = lseek(mmaps[i].fd,0,SEEK_CUR);
+  lseek(mmaps[i].fd,mmaps[i].offset+(addr-mmaps[i].addr),SEEK_SET);
+  int fail = (write(mmaps[i].fd,addr,len) != len);
+  lseek(mmaps[i].fd,oldpos,SEEK_SET);
+
+  return (fail?__set_errno(EIO):0);
 }
 
 /* Advise the system about particular usage patterns the program follows
@@ -368,6 +432,10 @@
   if (i == MAX_MMAPS)
     return (caddr_t) __set_errno (EINVAL);
 
+  /* For simplicity don't allow remapping of mapped files */
+  if (mmaps[i].fd != -1)
+    return (caddr_t) __set_errno (ENOSYS);
+
   old_len = page_align (gbl, old_len);
 
   /* Check correct length was passed.  */
@@ -400,8 +468,7 @@
 	 data.  */
       old_area = mmaps[i].number;
       mmaps[i].number = -1;
-      /* FIXME - get mapping type flags */
-      new_addr = mmap (0, new_len, mmaps[i].prot, MAP_ANON, -1, 0);
+      new_addr = mmap (0, new_len, mmaps[i].prot, mmaps[i].flags, -1, 0);
       if (new_addr == (caddr_t)-1)
 	{
 	  /* If mmap failed, then keep the old area.  */


More information about the gcc mailing list