diff mbox

exec: Don't request an address for code_gen_buffer if -fpie

Message ID 5071D5F9.4010106@twiddle.net
State New
Headers show

Commit Message

Richard Henderson Oct. 7, 2012, 7:20 p.m. UTC 
On 10/07/2012 09:34 AM, Blue Swirl wrote:
>> > +#ifdef USE_MMAP
>> > +    code_gen_buffer = mmap((void *)start, code_gen_buffer_size,
>> > +                           PROT_WRITE | PROT_READ | PROT_EXEC,
>> > +                           flags, -1, 0);
>> > +    if (code_gen_buffer == MAP_FAILED) {
>> > +        fprintf(stderr, "Could not allocate dynamic translator buffer\n");
>> > +        exit(1);
>> >      }
>> >  #else
>> >      code_gen_buffer = g_malloc(code_gen_buffer_size);
>> >      map_exec(code_gen_buffer, code_gen_buffer_size);
> In this branch (e.g. mingw32), 'start' is unused:
> /src/qemu/exec.c: In function 'code_gen_alloc':
> /src/qemu/exec.c:531: warning: unused variable 'start'

Well, I've rearranged the code to handle this, and it does avoid the warning.
But I'm not sure I like the two separate blocks.  Especially for the x86_64
MAP32 case.  Perhaps we're better off with an __attribute__((unused)) there?


r~

Comments

Blue Swirl Oct. 7, 2012, 7:40 p.m. UTC | #1 
On Sun, Oct 7, 2012 at 7:20 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 10/07/2012 09:34 AM, Blue Swirl wrote:
>>> > +#ifdef USE_MMAP
>>> > +    code_gen_buffer = mmap((void *)start, code_gen_buffer_size,
>>> > +                           PROT_WRITE | PROT_READ | PROT_EXEC,
>>> > +                           flags, -1, 0);
>>> > +    if (code_gen_buffer == MAP_FAILED) {
>>> > +        fprintf(stderr, "Could not allocate dynamic translator buffer\n");
>>> > +        exit(1);
>>> >      }
>>> >  #else
>>> >      code_gen_buffer = g_malloc(code_gen_buffer_size);
>>> >      map_exec(code_gen_buffer, code_gen_buffer_size);
>> In this branch (e.g. mingw32), 'start' is unused:
>> /src/qemu/exec.c: In function 'code_gen_alloc':
>> /src/qemu/exec.c:531: warning: unused variable 'start'
>
> Well, I've rearranged the code to handle this, and it does avoid the warning.
> But I'm not sure I like the two separate blocks.  Especially for the x86_64
> MAP32 case.  Perhaps we're better off with an __attribute__((unused)) there?

How about splitting the function into three: common part, mmap case
and non-mmap case? It could improve readability.

>
>
> r~
>
>
>
>
Richard Henderson Oct. 12, 2012, 9:20 p.m. UTC | #2 
This revision of the patch set takes Blue's suggestion to split up
code_gen_alloc into several pieces.  It does seem to clean things
up a bit.

The first patch is cleanup, doing the split.  The third patch does
in one line what I was trying to accomplish with the first revision
of this patch.  The second and fourth patches are new.

The patch set is available from

  git://github.com/rth7680/qemu.git exec


r~


Richard Henderson (4):
  exec: Split up and tidy code_gen_buffer
  exec: Don't make DEFAULT_CODE_GEN_BUFFER_SIZE too large
  exec: Do not use absolute address hints for code_gen_buffer with
    -fpie
  exec: Allocate code_gen_prologue from code_gen_buffer

 exec.c    | 232 +++++++++++++++++++++++++++++++++-----------------------------
 tcg/tcg.h |   2 +-
 2 files changed, 123 insertions(+), 111 deletions(-)
diff mbox

Patch

diff --git a/exec.c b/exec.c
index 8f3bc74..704426c 100644
--- a/exec.c
+++ b/exec.c
@@ -527,15 +527,14 @@  static void code_gen_alloc(unsigned long tb_size)
 #else
 #ifdef USE_MMAP
     int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+    uintptr_t start = 0;
 #endif
-    uintptr_t max_buf = -1, start = 0;
+    size_t max_buf = ~(size_t)0;
 
-    /* Constrain the size and position of the buffer based on the host cpu.  */
+    /* Constrain the size of the buffer based on the host cpu.  */
 #if defined(__x86_64__)
-# if !defined(__PIE__) && !defined(__PIC__) && defined(MAP_32BIT)
-    /* Force the memory down into low memory with the executable.
-       Leave the choice of exact location with the kernel.  */
-    flags |= MAP_32BIT;
+# if !defined(__PIE__) && !defined(__PIC__) \
+     && defined(USE_MMAP) && defined(MAP_32BIT)
     /* Cannot expect to map more than 800MB in low memory.  */
     max_buf = 800 * 1024 * 1024;
 # else
@@ -545,22 +544,12 @@  static void code_gen_alloc(unsigned long tb_size)
 #elif defined(__sparc__) && HOST_LONG_BITS == 64
     /* Maximum range of direct branches between TB (via "call").  */
     max_buf = 2ul * 1024 * 1024 * 1024;
-    start = 0x40000000ul;
 #elif defined(__arm__)
     /* Keep the buffer no bigger than 16MB to branch between blocks */
     max_buf = 16 * 1024 * 1024;
 #elif defined(__s390x__)
-    /* Map the buffer so that we can use direct calls and branches.  */
     /* We have a +- 4GB range on the branches; leave some slop.  */
     max_buf = 3ul * 1024 * 1024 * 1024;
-    start = 0x90000000ul;
-#endif
-#if defined(__PIE__) || defined(__PIC__)
-    /* Don't bother setting a preferred location if we're building
-       a position-independent executable.  We're more likely to get
-       an address near the main executable if we let the kernel
-       choose the address.  */
-    start = 0;
 #endif
 
     /* Size the buffer.  */
@@ -581,6 +570,23 @@  static void code_gen_alloc(unsigned long tb_size)
     }
 
 #ifdef USE_MMAP
+    /* Constrain the position of the buffer based on the host cpu.
+       Note that these addresses are chosen in concert with the
+       addresses assigned in the relevant linker script file.  */
+# if defined(__PIE__) || defined(__PIC__)
+    /* Don't bother setting a preferred location if we're building
+       a position-independent executable.  We're more likely to get
+       an address near the main executable if we let the kernel
+       choose the address.  */
+# elif defined(__x86_64__) && defined(MAP_32BIT)
+    /* Force the memory down into low memory with the executable.
+       Leave the choice of exact location with the kernel.  */
+    flags |= MAP_32BIT;
+# elif defined(__sparc__) && HOST_LONG_BITS == 64
+    start = 0x40000000ul;
+# elif defined(__s390x__)
+    start = 0x90000000ul;
+# endif
     code_gen_buffer = mmap((void *)start, code_gen_buffer_size,
                            PROT_WRITE | PROT_READ | PROT_EXEC,
                            flags, -1, 0);