Last Comment Bug 725284 - elfhack doesn't preserve alignment in program headers
: elfhack doesn't preserve alignment in program headers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla13
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 723939
  Show dependency treegraph
 
Reported: 2012-02-08 05:40 PST by Ted Mielczarek [:ted.mielczarek]
Modified: 2012-02-22 04:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (3.70 KB, patch)
2012-02-15 08:50 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
WIP (4.26 KB, patch)
2012-02-15 08:56 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Preserve PT_LOAD alignment, except when it's the default on x86-64 (7.37 KB, patch)
2012-02-16 02:44 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Preserve PT_LOAD alignment, except when it's the default on x86-64 (7.38 KB, patch)
2012-02-16 03:25 PST, Mike Hommey [:glandium]
taras.mozilla: review+
nfroyd: feedback+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2012-02-08 05:40:49 PST
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] 2012-02-15 08:50:36 PST
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.
Comment 2 Mike Hommey [:glandium] 2012-02-15 08:56:47 PST
Created attachment 597428 [details] [diff] [review]
WIP

Was missing a bit to work properly.
Comment 3 Mike Hommey [:glandium] 2012-02-16 02:44:42 PST
Created attachment 597737 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64
Comment 4 Mike Hommey [:glandium] 2012-02-16 03:25:07 PST
Created attachment 597748 [details] [diff] [review]
Preserve PT_LOAD alignment, except when it's the default on x86-64
Comment 5 (dormant account) 2012-02-17 10:54:22 PST
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] 2012-02-20 05:50:12 PST
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] 2012-02-20 05:56:57 PST
(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] 2012-02-20 06:02:08 PST
(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 10 Ed Morley [:emorley] 2012-02-22 04:45:10 PST
https://hg.mozilla.org/mozilla-central/rev/4c2c6e1e5433

Note You need to log in before you can comment on or make changes to this bug.