elfhack doesn't preserve alignment in program headers

RESOLVED FIXED in mozilla13

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ted, Assigned: glandium)

Tracking

Trunk
mozilla13
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Assignee: nobody → mh+mozilla
Created attachment 597425 [details] [diff] [review]
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.
Created attachment 597428 [details] [diff] [review]
WIP

Was missing a bit to work properly.
Attachment #597425 - Attachment is obsolete: true
Created attachment 597737 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64
Attachment #597737 - Flags: review?(taras.mozilla)
Attachment #597428 - Attachment is obsolete: true
Created attachment 597748 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64
Attachment #597748 - Flags: review?(taras.mozilla)
Attachment #597737 - Attachment is obsolete: true
Attachment #597737 - Flags: review?(taras.mozilla)

Comment 5

5 years ago
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
Attachment #597748 - Flags: review?(taras.mozilla)
Attachment #597748 - Flags: review+
Attachment #597748 - Flags: feedback?(nfroyd)
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.
Attachment #597748 - Flags: feedback?(nfroyd) → feedback+
(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)
(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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2c6e1e5433
https://hg.mozilla.org/mozilla-central/rev/4c2c6e1e5433
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.