Fix dynamic library linking.

Fix dynamic library linking.

Align the BSS to a 64k page boundary in order to work around the issue
NaCl's mmap() has with zero filling.

More discussion can be found here:
  http://code.google.com/p/nativeclient/issues/detail?id=1068

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2184
TEST=see bug #2184

R=pasko@google.com

Committed: http://git.chromium.org/gitweb?p=native_client/nacl-glibc.git;a=commit;h=3e75d2e

[Brad Chen, 2011-08-24 15:28:51]

Q: will this fix some of the failing browser tests?

On Wed, Aug 24, 2011 at 8:25 AM, <khim@chromium.org> wrote:

> Reviewers: pasko,
>
> Description:
> Fix dynamic library linking.
>
> Align the BSS to a 64k page boundary in order to work around the issue
> NaCl's mmap() has with zero filling.
>
> More discussion can be found here:
> 
http://code.google.com/p/**nativeclient/issues/detail?id=**1068<http://code.g...
>
>
BUG=http://code.google.com/p/**nativeclient/issues/detail?id=**2184<http://code.google.com/p/nativeclient/issues/detail?id=2184>
> TEST=see bug #2184
>
> R=pasko@google.com
>
>
> Please review this at
http://codereview.chromium.**org/7715030/<http://codereview.chromium.org/7715...
>
> SVN Base:
http://git.chromium.org/**native_client/nacl-glibc.git@**master<http://git.ch...
>
> Affected files:
>  M elf/Makefile
>  M nacl/dyn-link/ldscripts/elf64_**nacl.xs
>  M nacl/dyn-link/ldscripts/elf_**nacl.xs
>
>
> Index: elf/Makefile
> diff --git a/elf/Makefile b/elf/Makefile
> index 07fc811c01aae3647c331a0cd82204**80ba2a434a..**
> fb5db6d40cbdaa249decf2c68c2560**0edd990552 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -308,8 +308,13 @@ $(objpfx)ld.so: $(objpfx)librtld.os $(ld-map)
>        $(LINK.o) -nostdlib -nostartfiles -shared $(z-now-$(bind-now))  \
>                  $(LDFLAGS-rtld) -Wl,-z,defs -Wl,--verbose 2>&1 |      \
>                  LC_ALL=C \
> -                 sed -e '/^=========/,/^=========/!d;/**^=========/d'
>  \
> +                 sed -e '/^=========/,/^=========/!d;/**^=========/d' \
>                      -e 's/\. = 0 + SIZEOF_HEADERS;/& _begin = . -
> SIZEOF_HEADERS;/' \
> +                     -e 's/_end = \.; PROVIDE (end = \.);/& . =
> DATA_SEGMENT_END(.);/' \
> +                     -e 's/seg_bss      PT_LOAD FLAGS(6) ;       \/\* read
> + write \*\///' \
> +                     -e 's/\. = DATA_SEGMENT_END (\.);//' -e 's/}
> :seg_bss/}/' \
> +                     -e 's/\. = ALIGN (CONSTANT (MAXPAGESIZE));//' \
> +                     -e 's/\. = ALIGN (CONSTANT (COMMONPAGESIZE));//' \
>                  > $@.lds
>        $(LINK.o) -nostdlib -nostartfiles -shared -o $@                 \
>                  $(LDFLAGS-rtld) -Wl,-z,defs $(z-now-$(bind-now))      \
> Index: nacl/dyn-link/ldscripts/elf64_**nacl.xs
> diff --git a/nacl/dyn-link/ldscripts/**elf64_nacl.xs
> b/nacl/dyn-link/ldscripts/**elf64_nacl.xs
> index befc62076bdf2c2bf3ee0f445c4568**fab60d4127..**
> 3de9663154f0a959fee0caa0f3ca9d**4fb8a8d2ba 100644
> --- a/nacl/dyn-link/ldscripts/**elf64_nacl.xs
> +++ b/nacl/dyn-link/ldscripts/**elf64_nacl.xs
> @@ -9,6 +9,7 @@ PHDRS
>   seg_code     PT_LOAD FLAGS(5) ;       /* read + execute */
>   seg_rodata   PT_LOAD FLAGS(4) ;       /* read */
>   seg_rwdata   PT_LOAD FLAGS(6) ;       /* read + write */
> +  seg_bss      PT_LOAD FLAGS(6) ;       /* read + write */
>   seg_dynamic  PT_DYNAMIC FLAGS(6) ;
>   seg_stack    PT_GNU_STACK FLAGS(6) ;
>   seg_tls      PT_TLS FLAGS(4) ;
> @@ -163,6 +164,19 @@ SECTIONS
>   }
>   .data1          : { *(.data1) }
>   _edata = .; PROVIDE (edata = .);
> +
> +  /* We place the BSS in a separate segment as a workaround for the
> +     behaviour of NaCl's mmap() on the last page in a file.  When using
> +     copy-on-write MAP_PRIVATE, we cannot necessarily write beyond the
> +     file's extent within the 64k page.
> +     See
http://code.google.com/p/**nativeclient/issues/detail?id=**824<http://code.go...
> .
> +     TODO(mseaborn): A better fix would be to change the dynamic linker
> +     to copy the last page.  This would save a page of memory because
> +     the BSS could share a page with the data segment.  But for now, it
> +     is simpler to change the linker script. */
> +  . = DATA_SEGMENT_END (.);
> +  . = ALIGN (CONSTANT (COMMONPAGESIZE));
> +
>   __bss_start = .;
>   .bss            :
>   {
> @@ -175,11 +189,10 @@ SECTIONS
>       FIXME: Why do we need it? When there is no .bss section, we don't
>       pad the .data section.  */
>    . = ALIGN(. != 0 ? 32 / 8 : 1);
> -  }
> +  } :seg_bss
>   . = ALIGN(32 / 8);
>   . = ALIGN(32 / 8);
>   _end = .; PROVIDE (end = .);
> -  . = DATA_SEGMENT_END (.);
>   /* Stabs debugging sections.  */
>   .stab          0 : { *(.stab) }
>   .stabstr       0 : { *(.stabstr) }
> Index: nacl/dyn-link/ldscripts/elf_**nacl.xs
> diff --git a/nacl/dyn-link/ldscripts/elf_**nacl.xs
> b/nacl/dyn-link/ldscripts/elf_**nacl.xs
> index 9de54d9238a251f1f45e3ef29575fb**659b974ccf..**
> c407c88c8963a67741a35af2de011f**67dc464326 100644
> --- a/nacl/dyn-link/ldscripts/elf_**nacl.xs
> +++ b/nacl/dyn-link/ldscripts/elf_**nacl.xs
> @@ -10,6 +10,7 @@ PHDRS
>   seg_code     PT_LOAD FLAGS(5) ;       /* read + execute */
>   seg_rodata   PT_LOAD FLAGS(4) ;       /* read */
>   seg_rwdata   PT_LOAD FLAGS(6) ;       /* read + write */
> +  seg_bss      PT_LOAD FLAGS(6) ;       /* read + write */
>   seg_dynamic  PT_DYNAMIC FLAGS(6) ;
>   seg_stack    PT_GNU_STACK FLAGS(6) ;
>   seg_tls      PT_TLS FLAGS(4) ;
> @@ -164,6 +165,19 @@ SECTIONS
>   }
>   .data1          : { *(.data1) }
>   _edata = .; PROVIDE (edata = .);
> +
> +  /* We place the BSS in a separate segment as a workaround for the
> +     behaviour of NaCl's mmap() on the last page in a file.  When using
> +     copy-on-write MAP_PRIVATE, we cannot necessarily write beyond the
> +     file's extent within the 64k page.
> +     See
http://code.google.com/p/**nativeclient/issues/detail?id=**824<http://code.go...
> .
> +     TODO(mseaborn): A better fix would be to change the dynamic linker
> +     to copy the last page.  This would save a page of memory because
> +     the BSS could share a page with the data segment.  But for now, it
> +     is simpler to change the linker script. */
> +  . = DATA_SEGMENT_END (.);
> +  . = ALIGN (CONSTANT (MAXPAGESIZE));
> +
>   __bss_start = .;
>   .bss            :
>   {
> @@ -176,11 +190,10 @@ SECTIONS
>       FIXME: Why do we need it? When there is no .bss section, we don't
>       pad the .data section.  */
>    . = ALIGN(. != 0 ? 32 / 8 : 1);
> -  }
> +  } :seg_bss
>   . = ALIGN(32 / 8);
>   . = ALIGN(32 / 8);
>   _end = .; PROVIDE (end = .);
> -  . = DATA_SEGMENT_END (.);
>   /* Stabs debugging sections.  */
>   .stab          0 : { *(.stab) }
>   .stabstr       0 : { *(.stabstr) }
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to native-client-reviews@**
> googlegroups.com <native-client-reviews@googlegroups.com>.
> To unsubscribe from this group, send email to native-client-reviews+**
>
unsubscribe@googlegroups.com<native-client-reviews%2Bunsubscribe@googlegroups...
> .
> For more options, visit this group at http://groups.google.com/**
>
group/native-client-reviews?**hl=en<http://groups.google.com/group/native-client-reviews?hl=en>
> .
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2011-08-24 15:35:33]

On Wed, Aug 24, 2011 at 7:28 PM, Brad Chen <bradchen@google.com> wrote:
> Q: will this fix some of the failing browser tests?

some of them, yes

-- 
Egor Pasko

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2011-08-24 16:38:14]

LGTM with some concerns that you are free to resolve at your will

http://codereview.chromium.org/7715030/diff/1/elf/Makefile
File elf/Makefile (right):

http://codereview.chromium.org/7715030/diff/1/elf/Makefile#newcode308
elf/Makefile:308: $(LINK.o) -nostdlib -nostartfiles -shared
$(z-now-$(bind-now))	\
would be nice to leave a comment above this line:
# NaCl loader refuses to run ld.so with more than 3 loadable segments.  Remove
the bss segment.

http://codereview.chromium.org/7715030/diff/1/elf/Makefile#newcode315
elf/Makefile:315: -e 's/\. = DATA_SEGMENT_END (\.);//' -e 's/} :seg_bss/}/' \
please, wrap the 2-nd -e here to the next line, would be more readable

http://codereview.chromium.org/7715030/diff/1/nacl/dyn-link/ldscripts/elf64_n...
File nacl/dyn-link/ldscripts/elf64_nacl.xs (right):

http://codereview.chromium.org/7715030/diff/1/nacl/dyn-link/ldscripts/elf64_n...
nacl/dyn-link/ldscripts/elf64_nacl.xs:178: . = ALIGN (CONSTANT
(COMMONPAGESIZE));
please, take a look it it did not significantly increase the overall size of
library files on disk, when files are not sparse

[khim, 2011-08-24 17:03:07]

http://codereview.chromium.org/7715030/diff/1/elf/Makefile
File elf/Makefile (right):

http://codereview.chromium.org/7715030/diff/1/elf/Makefile#newcode308
elf/Makefile:308: $(LINK.o) -nostdlib -nostartfiles -shared
$(z-now-$(bind-now))	\
On 2011/08/24 16:38:14, pasko wrote:
> would be nice to leave a comment above this line:
> # NaCl loader refuses to run ld.so with more than 3 loadable segments.  Remove
> the bss segment.

Done.

http://codereview.chromium.org/7715030/diff/1/elf/Makefile#newcode315
elf/Makefile:315: -e 's/\. = DATA_SEGMENT_END (\.);//' -e 's/} :seg_bss/}/' \
On 2011/08/24 16:38:14, pasko wrote:
> please, wrap the 2-nd -e here to the next line, would be more readable

Done.

http://codereview.chromium.org/7715030/diff/1/nacl/dyn-link/ldscripts/elf64_n...
File nacl/dyn-link/ldscripts/elf64_nacl.xs (right):

http://codereview.chromium.org/7715030/diff/1/nacl/dyn-link/ldscripts/elf64_n...
nacl/dyn-link/ldscripts/elf64_nacl.xs:178: . = ALIGN (CONSTANT
(COMMONPAGESIZE));
On 2011/08/24 16:38:14, pasko wrote:
> please, take a look it it did not significantly increasthe overall size of
library files on disk, when files are not sparse

It adds few kilobytes to file with debug info (but these are large anyway),
after strip size is the same.