725284 - elfhack doesn't preserve alignment in program headers

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

We need to fix bug 723939 to make our Android builds work on armv6. Unfortunately, elfhack breaks the alignment of program headers. It should preserve whatever alignment is already there.

Comment 1

[Mike Hommey [:glandium]]

Attachment: WIP

This makes elfhack strictly respect segment alignments, but that makes it less useful on x64 because of the huge default alignment. It also breaks the testcase when the alignment is big enough that the testcase doesn't end up having any gain at all.

Comment 2

[Mike Hommey [:glandium]]

Attachment: WIP

Was missing a bit to work properly.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Preserve PT_LOAD alignment, except when it's the default on x86-64

Comment 4

[Mike Hommey [:glandium]]

Attachment: Preserve PT_LOAD alignment, except when it's the default on x86-64

Comment 5

[(dormant account)]

Comment on attachment 597748 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64

rubberstamp.

+ if ((*seg)->getAlign() > align)
+            align = (*seg)->getAlign();

I prefer std::max

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 597748 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64

Review of attachment 597748 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/unix/elfhack/Makefile.in
@@ +94,5 @@
>  	@echo === and your environment \(compiler and linker versions\), and use
>  	@echo === --disable-elf-hack until this is fixed.
>  	@echo ===
>  	@rm -f $@.bak
> +	$(CURDIR)/elfhack -b -f $@

This just forces it on for the tests, correct?  Why not additionally tweak the packager.mk invocation?  (And then I wonder what the point of the -f flag is, if we do that.)

::: build/unix/elfhack/elf.cpp
@@ +266,5 @@
>      for (int i = 0; i < ehdr->e_phnum; i++) {
>          Elf_Phdr phdr(file, e_ident[EI_CLASS], e_ident[EI_DATA]);
> +        if (phdr.p_type == PT_LOAD) {
> +            // Default alignment for PT_LOAD on x86-64 prevent elfhack to
> +            // do anything useful. However, the system doesn't actually

Nit: "prevents elfhack from doing anything useful."

::: build/unix/elfhack/elfhack.cpp
@@ +540,5 @@
>              elf->write(ofile);
>              fprintf(stderr, "Reduced by %d bytes\n", size - elf->getSize());
>          }
>      }
> +   delete elf;

Nit: indentation change.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #6)
> Comment on attachment 597748 [details] [diff] [review]
> Preserve PT_LOAD alignment, except when it's the default on x86-64
> 
> Review of attachment 597748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/unix/elfhack/Makefile.in
> @@ +94,5 @@
> >  	@echo === and your environment \(compiler and linker versions\), and use
> >  	@echo === --disable-elf-hack until this is fixed.
> >  	@echo ===
> >  	@rm -f $@.bak
> > +	$(CURDIR)/elfhack -b -f $@
> 
> This just forces it on for the tests, correct?  Why not additionally tweak
> the packager.mk invocation?  (And then I wonder what the point of the -f
> flag is, if we do that.)

-f forces to ignore alignment. The reason this is not in packager.mk is that while we do want the elfhack test to completely ignore alignment (on top of ignoring the x86-64 default), we don't want alignment to be ignored when working normally. (The only reason we do need to ignore alignment for the test is that depending on alignment requirement, elfhack would do nothing on the testcase because alignment would prevent it from doing anything useful)

Comment 8

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #7)
> -f forces to ignore alignment. The reason this is not in packager.mk is that
> while we do want the elfhack test to completely ignore alignment (on top of
> ignoring the x86-64 default), we don't want alignment to be ignored when
> working normally.

Erm, the week end blew off my mind... -f doesn't actually ignore alignment, it just forces elfhack to do its job even when the resulting binary is bigger than the original one, which is what happens when the alignment requirement makes elfhack useless. This is not a problem on x86 or x86-64, but it is on armv6, where we force an alignment of 0x4000, and that makes elfhack useless on the testcase (but not useless on libxul.so)

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2c6e1e5433

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c2c6e1e5433