Revert "Allocate bss section within proper range of other sections"

This reverts commit e019ec94f12268dd92ea5d5204e9e57e7ebf10ca.

This sadly breaks the external interpreter on i386.

For instance, see https://circleci.com/gh/ghc/ghc/10925.

16 files changed, 76 insertions, 132 deletions

diff --git a/rts/Linker.c b/rts/Linker.c
index 48d52c3f3d..b42a0de9c3 100644
--- a/rts/Linker.c
+++ b/rts/Linker.c
@@ -1284,8 +1284,6 @@ mkOc( pathchar *path, char *image, int imageSize,
 #if defined(NEED_SYMBOL_EXTRAS)
    oc->symbol_extras     = NULL;
 #endif
-   oc->bssBegin          = NULL;
-   oc->bssEnd            = NULL;
    oc->imageMapped       = mapped;
 
    oc->misalignment      = misalignment;
@@ -1499,35 +1497,35 @@ HsInt loadOc (ObjectCode* oc)
    }
 
    /* Note [loadOc orderings]
-      The order of `ocAllocateExtras` and `ocGetNames` matters. For MachO
+      The order of `ocAllocateSymbolExtras` and `ocGetNames` matters. For MachO
       and ELF, `ocInit` and `ocGetNames` initialize a bunch of pointers based
-      on the offset to `oc->image`, but `ocAllocateExtras` may relocate
+      on the offset to `oc->image`, but `ocAllocateSymbolExtras` may relocate
       the address of `oc->image` and invalidate those pointers. So we must
-      compute or recompute those pointers after `ocAllocateExtras`.
+      compute or recompute those pointers after `ocAllocateSymbolExtras`.
 
       On Windows, when we have an import library we (for now, as we don't honor
       the lazy loading semantics of the library and instead GHCi is already
       lazy) don't use the library after ocGetNames as it just populates the
-      symbol table.  Allocating space for jump tables in ocAllocateExtras
+      symbol table.  Allocating space for jump tables in ocAllocateSymbolExtras
       would just be a waste then as we'll be stopping further processing of the
       library in the next few steps. If necessary, the actual allocation
-      happens in `ocGetNames_PEi386` and `ocAllocateExtras_PEi386` simply
+      happens in `ocGetNames_PEi386` and `ocAllocateSymbolExtras_PEi386` simply
       set the correct pointers.
       */
 
 #if defined(NEED_SYMBOL_EXTRAS)
 #  if defined(OBJFORMAT_MACHO)
-   r = ocAllocateExtras_MachO ( oc );
+   r = ocAllocateSymbolExtras_MachO ( oc );
    if (!r) {
        IF_DEBUG(linker,
-                debugBelch("loadOc: ocAllocateExtras_MachO failed\n"));
+                debugBelch("loadOc: ocAllocateSymbolExtras_MachO failed\n"));
        return r;
    }
 #  elif defined(OBJFORMAT_ELF)
-   r = ocAllocateExtras_ELF ( oc );
+   r = ocAllocateSymbolExtras_ELF ( oc );
    if (!r) {
        IF_DEBUG(linker,
-                debugBelch("loadOc: ocAllocateExtras_ELF failed\n"));
+                debugBelch("loadOc: ocAllocateSymbolExtras_ELF failed\n"));
        return r;
    }
 #  endif
@@ -1548,7 +1546,7 @@ HsInt loadOc (ObjectCode* oc)
    }
 
 #  if defined(OBJFORMAT_PEi386)
-   ocAllocateExtras_PEi386 ( oc );
+   ocAllocateSymbolExtras_PEi386 ( oc );
 #  endif
 #endif
 
diff --git a/rts/LinkerInternals.h b/rts/LinkerInternals.h
index 9c34c18aef..04d873ca99 100644
--- a/rts/LinkerInternals.h
+++ b/rts/LinkerInternals.h
@@ -193,10 +193,6 @@ typedef struct _ObjectCode {
     unsigned long   first_symbol_extra;
     unsigned long   n_symbol_extras;
 #endif
-    /* Additional memory that is preallocated and contiguous with image
-       which can be used used to relocate bss sections. */
-    char* bssBegin;
-    char* bssEnd;
 
     ForeignExportStablePtr *stable_ptrs;
 
diff --git a/rts/linker/Elf.c b/rts/linker/Elf.c
index 0a7ffaec21..fe87aed0b0 100644
--- a/rts/linker/Elf.c
+++ b/rts/linker/Elf.c
@@ -579,7 +579,7 @@ ocVerifyImage_ELF ( ObjectCode* oc )
 /* Figure out what kind of section it is.  Logic derived from
    Figure 1.14 ("Special Sections") of the ELF document
    ("Portable Formats Specification, Version 1.1"). */
-static SectionKind getSectionKind_ELF( Elf_Shdr *hdr, int *is_bss )
+static int getSectionKind_ELF( Elf_Shdr *hdr, int *is_bss )
 {
     *is_bss = false;
 
@@ -681,31 +681,23 @@ ocGetNames_ELF ( ObjectCode* oc )
       StgWord mapped_size = 0, mapped_offset = 0;
       StgWord size = shdr[i].sh_size;
       StgWord offset = shdr[i].sh_offset;
-      StgWord align = shdr[i].sh_addralign;
 
       if (is_bss && size > 0) {
          /* This is a non-empty .bss section.  Allocate zeroed space for
             it, and set its .sh_offset field such that
             ehdrC + .sh_offset == addr_of_zeroed_space.  */
-#if defined(NEED_GOT) || RTS_LINKER_USE_MMAP
-          if (USE_CONTIGUOUS_MMAP || RtsFlags.MiscFlags.linkerAlwaysPic) {
-              /* The space for bss sections is already preallocated */
-              ASSERT(oc->bssBegin != NULL);
-              alloc = SECTION_NOMEM;
-              start =
-                oc->image + roundUpToAlign(oc->bssBegin - oc->image, align);
-              oc->bssBegin = (char*)start + size;
-              ASSERT(oc->bssBegin <= oc->bssEnd);
-          } else {
-              /* Use mmapForLinker to allocate .bss, otherwise the malloced
-               * address might be out of range for sections that are mmaped.
-               */
-              alloc = SECTION_MMAP;
-              start = mmapForLinker(size, MAP_ANONYMOUS, -1, 0);
-              mapped_start = start;
-              mapped_offset = 0;
-              mapped_size = roundUpToPage(size);
-          }
+#if defined(NEED_GOT)
+          /* always use mmap if we use GOT slots.  Otherwise the malloced
+           * address might be out of range for sections that are mmaped.
+           */
+          alloc = SECTION_MMAP;
+          start = mmap(NULL, size,
+                       PROT_READ | PROT_WRITE | PROT_EXEC,
+                       MAP_ANON | MAP_PRIVATE,
+                       -1, 0);
+          mapped_start = start;
+          mapped_offset = 0;
+          mapped_size = roundUpToPage(size);
 #else
           alloc = SECTION_MALLOC;
           start = stgCallocBytes(1, size, "ocGetNames_ELF(BSS)");
@@ -1881,27 +1873,22 @@ int ocRunInit_ELF( ObjectCode *oc )
 
 #if defined(NEED_SYMBOL_EXTRAS)
 
-int ocAllocateExtras_ELF( ObjectCode *oc )
+int ocAllocateSymbolExtras_ELF( ObjectCode *oc )
 {
-  Elf_Ehdr *ehdr = (Elf_Ehdr *) oc->image;
-  Elf_Shdr* shdr = (Elf_Shdr *) ( ((char *)oc->image) + ehdr->e_shoff );
-  Elf_Shdr* symtab = NULL;
-  Elf_Word shnum = elf_shnum(ehdr);
-  int bssSize = 0;
-
-  for (Elf_Word i = 0; i < shnum; ++i) {
-    if(shdr[i].sh_type == SHT_SYMTAB) {
-      symtab = &shdr[i];
-    } else {
-      int isBss = 0;
-      getSectionKind_ELF(&shdr[i], &isBss);
-      if (isBss && shdr[i].sh_size > 0) {
-        bssSize += roundUpToAlign(shdr[i].sh_size, shdr[i].sh_addralign);
-      }
-    }
-  }
+  Elf_Ehdr *ehdr;
+  Elf_Shdr* shdr;
+  Elf_Word i, shnum;
+
+  ehdr = (Elf_Ehdr *) oc->image;
+  shdr = (Elf_Shdr *) ( ((char *)oc->image) + ehdr->e_shoff );
+
+  shnum = elf_shnum(ehdr);
+
+  for( i = 0; i < shnum; i++ )
+    if( shdr[i].sh_type == SHT_SYMTAB )
+      break;
 
-  if (symtab == NULL)
+  if( i == shnum )
   {
     // Not having a symbol table is not in principle a problem.
     // When an object file has no symbols then the 'strip' program
@@ -1911,15 +1898,15 @@ int ocAllocateExtras_ELF( ObjectCode *oc )
     return 1;
   }
 
-  if( symtab->sh_entsize != sizeof( Elf_Sym ) )
+  if( shdr[i].sh_entsize != sizeof( Elf_Sym ) )
   {
     errorBelch( "The entry size (%d) of the symtab isn't %d\n",
-      (int) symtab->sh_entsize, (int) sizeof( Elf_Sym ) );
+      (int) shdr[i].sh_entsize, (int) sizeof( Elf_Sym ) );
 
     return 0;
   }
 
-  return ocAllocateExtras(oc, symtab->sh_size / sizeof( Elf_Sym ), 0, bssSize);
+  return ocAllocateSymbolExtras( oc, shdr[i].sh_size / sizeof( Elf_Sym ), 0 );
 }
 
 #endif /* NEED_SYMBOL_EXTRAS */
diff --git a/rts/linker/Elf.h b/rts/linker/Elf.h
index 30c993b98c..b0d6638e6a 100644
--- a/rts/linker/Elf.h
+++ b/rts/linker/Elf.h
@@ -13,6 +13,6 @@ int ocVerifyImage_ELF    ( ObjectCode* oc );
 int ocGetNames_ELF       ( ObjectCode* oc );
 int ocResolve_ELF        ( ObjectCode* oc );
 int ocRunInit_ELF        ( ObjectCode* oc );
-int ocAllocateExtras_ELF ( ObjectCode *oc );
+int ocAllocateSymbolExtras_ELF( ObjectCode *oc );
 
 #include "EndPrivate.h"
diff --git a/rts/linker/MachO.c b/rts/linker/MachO.c
index 7d5ff32276..ff8ef7a1e4 100644
--- a/rts/linker/MachO.c
+++ b/rts/linker/MachO.c
@@ -186,10 +186,10 @@ resolveImports(
 #if NEED_SYMBOL_EXTRAS
 #if defined(powerpc_HOST_ARCH)
 int
-ocAllocateExtras_MachO(ObjectCode* oc)
+ocAllocateSymbolExtras_MachO(ObjectCode* oc)
 {
 
-    IF_DEBUG(linker, debugBelch("ocAllocateExtras_MachO: start\n"));
+    IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: start\n"));
 
     // Find out the first and last undefined external
     // symbol, so we don't have to allocate too many
@@ -218,31 +218,28 @@ ocAllocateExtras_MachO(ObjectCode* oc)
     }
 
     if (max >= min) {
-        return ocAllocateExtras(oc, max - min + 1, min, 0);
+        return ocAllocateSymbolExtras(oc, max - min + 1, min);
     }
 
-    return ocAllocateExtras(oc, 0, 0, 0);
+    return ocAllocateSymbolExtras(oc,0,0);
 }
 
 #elif defined(x86_64_HOST_ARCH) || defined(aarch64_HOST_ARCH)
 
 int
-ocAllocateExtras_MachO(ObjectCode* oc)
+ocAllocateSymbolExtras_MachO(ObjectCode* oc)
 {
-    IF_DEBUG(linker, debugBelch("ocAllocateExtras_MachO: start\n"));
+    IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: start\n"));
 
     if (NULL != oc->info->symCmd) {
-        IF_DEBUG(linker,
-            debugBelch("ocAllocateExtras_MachO: allocate %d symbols\n",
-                oc->info->symCmd->nsyms));
-        IF_DEBUG(linker, debugBelch("ocAllocateExtras_MachO: done\n"));
-        return ocAllocateExtras(oc, oc->info->symCmd->nsyms, 0, 0);
+        IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: allocate %d symbols\n", oc->info->symCmd->nsyms));
+        IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: done\n"));
+        return ocAllocateSymbolExtras(oc, oc->info->symCmd->nsyms, 0);
     }
 
-    IF_DEBUG(linker,
-        debugBelch("ocAllocateExtras_MachO: allocated no symbols\n"));
-    IF_DEBUG(linker, debugBelch("ocAllocateExtras_MachO: done\n"));
-    return ocAllocateExtras(oc, 0, 0, 0);
+    IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: allocated no symbols\n"));
+    IF_DEBUG(linker, debugBelch("ocAllocateSymbolExtras_MachO: done\n"));
+    return ocAllocateSymbolExtras(oc,0,0);
 }
 
 #else
diff --git a/rts/linker/MachO.h b/rts/linker/MachO.h
index 4fb58e8668..b495c2b9b1 100644
--- a/rts/linker/MachO.h
+++ b/rts/linker/MachO.h
@@ -13,7 +13,7 @@ int ocGetNames_MachO       ( ObjectCode* oc );
 int ocResolve_MachO        ( ObjectCode* oc );
 int ocRunInit_MachO        ( ObjectCode* oc );
 int machoGetMisalignment( FILE * );
-int ocAllocateExtras_MachO ( ObjectCode* oc );
+int ocAllocateSymbolExtras_MachO ( ObjectCode* oc );
 
 #if defined(powerpc_HOST_ARCH)
 void machoInitSymbolsWithoutUnderscore( void );
diff --git a/rts/linker/PEi386.c b/rts/linker/PEi386.c
index bf642d8b23..ab4583da7c 100644
--- a/rts/linker/PEi386.c
+++ b/rts/linker/PEi386.c
@@ -1777,7 +1777,7 @@ ocGetNames_PEi386 ( ObjectCode* oc )
  * so simply set correct pointer here.
  */
 bool
-ocAllocateExtras_PEi386 ( ObjectCode* oc )
+ocAllocateSymbolExtras_PEi386 ( ObjectCode* oc )
 {
    /* If the ObjectCode was unloaded we don't need a trampoline, it's likely
       an import library so we're discarding it earlier.  */
diff --git a/rts/linker/PEi386.h b/rts/linker/PEi386.h
index 538f132ab5..eb5bec8b78 100644
--- a/rts/linker/PEi386.h
+++ b/rts/linker/PEi386.h
@@ -57,7 +57,7 @@ bool ocRunInit_PEi386     ( ObjectCode *oc );
 bool ocGetNames_PEi386    ( ObjectCode* oc );
 bool ocVerifyImage_PEi386 ( ObjectCode* oc );
 SymbolAddr *lookupSymbol_PEi386(SymbolName *lbl);
-bool ocAllocateExtras_PEi386 ( ObjectCode* oc );
+bool ocAllocateSymbolExtras_PEi386 ( ObjectCode* oc );
 SymbolAddr *lookupSymbolInDLLs ( const SymbolName* lbl );
 /* See Note [mingw-w64 name decoration scheme] */
 /* We use myindex to calculate array addresses, rather than
diff --git a/rts/linker/SymbolExtras.c b/rts/linker/SymbolExtras.c
index a9e4c37967..4c40b10877 100644
--- a/rts/linker/SymbolExtras.c
+++ b/rts/linker/SymbolExtras.c
@@ -31,11 +31,10 @@
 #endif /* RTS_LINKER_USE_MMAP */
 
 /*
-  ocAllocateExtras
+  ocAllocateSymbolExtras
 
   Allocate additional space at the end of the object file image to make room
-  for jump islands (powerpc, x86_64, arm), GOT entries (x86_64) and
-  bss sections.
+  for jump islands (powerpc, x86_64, arm) and GOT entries (x86_64).
 
   PowerPC relative branch instructions have a 24 bit displacement field.
   As PPC code is always 4-byte-aligned, this yields a +-32MB range.
@@ -50,11 +49,12 @@
   filled in by makeSymbolExtra below.
 */
 
-int ocAllocateExtras(ObjectCode* oc, int count, int first, int bssSize)
+int ocAllocateSymbolExtras( ObjectCode* oc, int count, int first )
 {
+  size_t n;
   void* oldImage = oc->image;
 
-  if (count > 0 || bssSize > 0) {
+  if (count > 0) {
     if (!RTS_LINKER_USE_MMAP) {
 
       // round up to the nearest 4
@@ -65,15 +65,16 @@ int ocAllocateExtras(ObjectCode* oc, int count, int first, int bssSize)
       oc->image = stgReallocBytes( oc->image,
                                misalignment +
                                aligned + sizeof (SymbolExtra) * count,
-                               "ocAllocateExtras" );
+                               "ocAllocateSymbolExtras" );
       oc->image += misalignment;
 
       oc->symbol_extras = (SymbolExtra *) (oc->image + aligned);
     } else if (USE_CONTIGUOUS_MMAP || RtsFlags.MiscFlags.linkerAlwaysPic) {
-      /* Keep image, bssExtras and symbol_extras contiguous */
-      size_t n = roundUpToPage(oc->fileSize);
-      bssSize = roundUpToAlign(bssSize, 8);
-      size_t allocated_size = n + bssSize + (sizeof(SymbolExtra) * count);
+      n = roundUpToPage(oc->fileSize);
+
+      /* Keep image and symbol_extras contiguous */
+
+      size_t allocated_size = n + (sizeof(SymbolExtra) * count);
       void *new = mmapForLinker(allocated_size, MAP_ANONYMOUS, -1, 0);
       if (new) {
           memcpy(new, oc->image, oc->fileSize);
@@ -82,10 +83,12 @@ int ocAllocateExtras(ObjectCode* oc, int count, int first, int bssSize)
           }
           oc->image = new;
           oc->imageMapped = true;
-          oc->fileSize = allocated_size;
-          oc->symbol_extras = (SymbolExtra *) (oc->image + n + bssSize);
-          oc->bssBegin = oc->image + n;
-          oc->bssEnd = oc->image + n + bssSize;
+          oc->fileSize = n + (sizeof(SymbolExtra) * count);
+          oc->symbol_extras = (SymbolExtra *) (oc->image + n);
+          if (mprotect(new, allocated_size,
+                       PROT_READ | PROT_WRITE | PROT_EXEC) != 0) {
+            sysErrorBelch("unable to protect memory");
+          }
       }
       else {
           oc->symbol_extras = NULL;
diff --git a/rts/linker/SymbolExtras.h b/rts/linker/SymbolExtras.h
index af828e66fa..4974c06e7d 100644
--- a/rts/linker/SymbolExtras.h
+++ b/rts/linker/SymbolExtras.h
@@ -7,7 +7,7 @@
 
 #if defined(NEED_SYMBOL_EXTRAS)
 
-int ocAllocateExtras(ObjectCode* oc, int count, int first, int bssSize);
+int ocAllocateSymbolExtras( ObjectCode* oc, int count, int first );
 
 #if defined(arm_HOST_ARCH)
 SymbolExtra* makeArmSymbolExtra( ObjectCode const* oc,
diff --git a/rts/sm/OSMem.h b/rts/sm/OSMem.h
index ea123e80c6..7dd0efdc23 100644
--- a/rts/sm/OSMem.h
+++ b/rts/sm/OSMem.h
@@ -32,16 +32,10 @@ roundDownToPage (size_t x)
 }
 
 INLINE_HEADER size_t
-roundUpToAlign (size_t size, size_t align)
-{
-    /* alignment must always be a power of 2 */
-    return (size + align - 1) & ~(align - 1);
-}
-
-INLINE_HEADER size_t
 roundUpToPage (size_t x)
 {
-    return roundUpToAlign(x, getPageSize());
+    size_t size = getPageSize();
+    return ((x + size - 1) & ~(size - 1));
 }
 
 
diff --git a/testsuite/tests/ghci/linking/Makefile b/testsuite/tests/ghci/linking/Makefile
index bfbcf24350..793998eb92 100644
--- a/testsuite/tests/ghci/linking/Makefile
+++ b/testsuite/tests/ghci/linking/Makefile
@@ -134,8 +134,3 @@ T14708:
 	"$(TEST_HC)" -c add.c -o T14708scratch/add.o
 	"$(AR)" cqs T14708scratch/libadd.a T14708scratch/add.o
 	-"$(TEST_HC)" $(TEST_HC_OPTS_INTERACTIVE) -LT14708scratch -ladd T14708.hs
-
-.PHONY: T15729
-T15729:
-	"$(TEST_HC)" -fPIC -c T15729.c -o bss.o
-	echo "main" | "$(TEST_HC)" $(TEST_HC_OPTS_INTERACTIVE) bss.o T15729.hs
diff --git a/testsuite/tests/ghci/linking/T15729.c b/testsuite/tests/ghci/linking/T15729.c
deleted file mode 100644
index 67cc6cd173..0000000000
--- a/testsuite/tests/ghci/linking/T15729.c
+++ /dev/null
@@ -1,4 +0,0 @@
-int readBss(int i) {
-  static int bss[1 << 20];
-  return bss[i];
-}
diff --git a/testsuite/tests/ghci/linking/T15729.hs b/testsuite/tests/ghci/linking/T15729.hs
deleted file mode 100644
index f35f96eac1..0000000000
--- a/testsuite/tests/ghci/linking/T15729.hs
+++ /dev/null
@@ -1,14 +0,0 @@
-module T15729 (main) where
-
-import Foreign
-import Foreign.C
-
-foreign import ccall unsafe "readBss"
-  readBss :: Int -> IO Int
-
-main :: IO ()
-main = do
-  prefix <- mapM readBss [0 .. 10]
-  print prefix
-  samples <- mapM readBss [0, 19 .. bit 20 - 1]
-  print $ foldr1 (.|.) samples
diff --git a/testsuite/tests/ghci/linking/T15729.stdout b/testsuite/tests/ghci/linking/T15729.stdout
deleted file mode 100644
index 2a408d0e6d..0000000000
--- a/testsuite/tests/ghci/linking/T15729.stdout
+++ /dev/null
@@ -1,2 +0,0 @@
-[0,0,0,0,0,0,0,0,0,0,0]
-0
diff --git a/testsuite/tests/ghci/linking/all.T b/testsuite/tests/ghci/linking/all.T
index 16d91f1f71..f9617c5cf7 100644
--- a/testsuite/tests/ghci/linking/all.T
+++ b/testsuite/tests/ghci/linking/all.T
@@ -39,9 +39,3 @@ test('T14708',
       extra_clean(['T14708scratch/*', 'T14708'])],
      run_command,
      ['$MAKE -s --no-print-directory T14708'])
-
-test('T15729',
-     [extra_files(['T15729.hs', 'T15729.c']),
-      unless(doing_ghci, skip)],
-     run_command,
-     ['$MAKE -s --no-print-directory T15729'])