Re: [PATCH v3 12/14] add linux_infcall_mmap

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 11 Nov 2014 17:43:19 +0100, Pedro Alves wrote:
> On 11/01/2014 09:47 PM, Jan Kratochvil wrote:
> > +/* See gdbarch.sh 'infcall_mmap'.  */
> > +
> > +static CORE_ADDR
> > +linux_infcall_mmap (CORE_ADDR size, unsigned prot)
> > +{
> > +  struct objfile *objf;
> > +  /* Do there still exist any Linux systems without "mmap64"?
> > +     "mmap" uses 64-bit off_t on x86_64 and 32-bit off_t on i386 and x32.  */
> > +  struct value *mmap_val = find_function_in_inferior ("mmap64", &objf);
> > +  struct value *addr_val;
> > +  struct gdbarch *gdbarch = get_objfile_arch (objf);
> > +  CORE_ADDR retval;
> > +  enum
> > +    {
> > +      ARG_ADDR, ARG_LENGTH, ARG_PROT, ARG_FLAGS, ARG_FD, ARG_OFFSET, ARG_MAX
> > +    };
> > +  struct value *arg[ARG_MAX];
> > +
> > +  arg[ARG_ADDR] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr,
> > +				      0);
> > +  /* Assuming sizeof (unsigned long) == sizeof (size_t).  */
> > +  arg[ARG_LENGTH] = value_from_ulongest
> > +		    (builtin_type (gdbarch)->builtin_unsigned_long, size);
> > +  gdb_assert ((prot & ~7) == 0);
> > +  arg[ARG_PROT] = value_from_longest (builtin_type (gdbarch)->builtin_int,
> > +				      0
> > +				      | ((prot & 4) != 0 ? PROT_READ : 0)
> > +				      | ((prot & 2) != 0 ? PROT_WRITE : 0)
> > +				      | ((prot & 1) != 0 ? PROT_EXEC : 0));
> > +  arg[ARG_FLAGS] = value_from_longest (builtin_type (gdbarch)->builtin_int,
> > +				       MAP_PRIVATE | MAP_ANONYMOUS);
> 
> PROT_READ, PROT_WRITE, PROT_EXEC, MAP_PRIVATE, MAP_ANONYMOUS
> are host values/macros.

Added to arch-utils.h:
+/* Symbols for gdbarch_infcall_mmap; their Linux PROT_* system
+   definitions would be dependent on compilation host.  */
+#define GDB_MMAP_PROT_READ     0x1     /* Page can be read.  */
+#define GDB_MMAP_PROT_WRITE    0x2     /* Page can be written.  */
+#define GDB_MMAP_PROT_EXEC     0x4     /* Page can be executed.  */

Added to linux-tdep.c:
+/* Symbols for linux_infcall_mmap's ARG_FLAGS; their Linux MAP_* system
+   definitions would be dependent on compilation host.  */
+#define GDB_MMAP_MAP_PRIVATE   0x02            /* Changes are private.  */
+#define GDB_MMAP_MAP_ANONYMOUS 0x20            /* Don't use a file.  */
and
-  gdb_assert ((prot & ~7) == 0);
+  gdb_assert ((prot & ~(GDB_MMAP_PROT_READ | GDB_MMAP_PROT_WRITE
+                       | GDB_MMAP_PROT_EXEC))
+             == 0);

Changed gdbarch.sh comment:
-# PROT has rwx bitmask format - bit 2 (value 4) is for readable memory, bit 1
-# (value 2) is for writable memory and bit 0 (value 1) is for executable memory.
+# PROT has GDB_MMAP_PROT_* bitmask format.

Change changed the magic numbers to GDB_MMAP_PROT_* accordingly:
       // Make the memory always readable.
-      prot = 4;
+      prot = GDB_MMAP_PROT_READ;
       if ((bfd_get_section_flags (abfd, sect) & SEC_READONLY) == 0)
-       prot |= 2;
+       prot |= GDB_MMAP_PROT_WRITE;
       if ((bfd_get_section_flags (abfd, sect) & SEC_CODE) != 0)
-       prot |= 1;
+       prot |= GDB_MMAP_PROT_EXEC;
[...]
-                                       TYPE_LENGTH (regs_type), 4);
+                                       TYPE_LENGTH (regs_type),
+                                       GDB_MMAP_PROT_READ);


> It also likely breaks --enable-targets=all builds on hosts
> that don't have mmap at all.  E.g., this comes out empty on F20:
> 
>  $ grep -rn PROT_READ /usr/i686-w64-mingw32/sys-root/mingw/include/

This could be caught automatically by Gerrit+Jenkins like other projects do
instead of wasting engineering time to do by hand all the parts of reviews
which machines can do.  Moreover such regressions would be caught before the
commit, compared to the GDB regressions happening post-commit as there is no
automatic pre-commit checking.  My Gerrit proposal was denied and replaced by
Patchwork instead.  This also makes regression results comparison difficult to
do as the regressions stack on top of each others.


Thanks,
Jan