816494 - Allow to revert the effects of elfhack

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

One of the annoying effect of elfhack is that some tools (like perf or valgrind) don't play very well with it.

While some of the things elfhack does are destructive (removal of the __cxa_pure_virtual relocations), there is enough information in a elfhacked ELF binary to re-create something mostly identical to the original file.

Comment 1

[Mike Hommey [:glandium]]

Attachment: part 1 - When inserting a section before or after another, also insert it in the segments containing that other.

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 2 - Move the PT_LOAD splitting logic in elfhack.cpp

Also, section offsets are not adjusted until the split is done.

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 3 - Allocate Elf instance on stack in do_file()

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 4 - Add a -r option to elfhack that re-merges the split PT_LOADs

Sections are positioned accordingly, which means the resulting ELF binary will
have a big gap full of zero between .rel.plt and .plt.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Created attachment 686575 [details] [diff] [review]
> part 4 - Add a -r option to elfhack that re-merges the split PT_LOADs
> 
> Sections are positioned accordingly, which means the resulting ELF binary
> will
> have a big gap full of zero between .rel.plt and .plt.

At this point, we aren't really close to the original file, but it's unelfhacked enough that valgrind is happy.

Comment 6

[Mike Hommey [:glandium]]

Attachment: part 5 - Add a test for elfhack reversal

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 686572 [details] [diff] [review]
part 2 - Move the PT_LOAD splitting logic in elfhack.cpp

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

::: build/unix/elfhack/elfhack.cpp
@@ +535,5 @@
> +    relhack->insertAfter(relhackcode);
> +
> +    // Adjust PT_LOAD segments
> +    for (ElfSegment *segment = elf->getSegmentByType(PT_LOAD); segment;
> +         segment = elf->getSegmentByType(PT_LOAD, segment)) {

The inner body of this loop probably deserves its own function for clarity's sake.

@@ +539,5 @@
> +         segment = elf->getSegmentByType(PT_LOAD, segment)) {
> +        std::list<ElfSection *>::iterator it = segment->begin();
> +        for (ElfSection *last = *(it++); it != segment->end(); last = *(it++)) {
> +            if (((*it)->getType() != SHT_NOBITS) && (last->getType() != SHT_NOBITS) &&
> +                ((*it)->getOffset() - last->getOffset() - last->getSize() > 0x1000)) {

Is this magic 0x1000 supposed to be page size?  Can we declare a |const| variable somewhere to make this clearer?

Can this condition be given its own reasonably named function as well?

@@ +551,5 @@
> +                phdr.p_filesz = (unsigned int)-1;
> +                phdr.p_memsz = (unsigned int)-1;
> +                ElfSegment *newSegment = new ElfSegment(&phdr);
> +                elf->insertSegmentAfter(segment, newSegment);
> +                std::list<ElfSection *>::reverse_iterator firstSection(it);

This variable is never used.

::: build/unix/elfhack/elfxx.h
@@ +284,5 @@
>      unsigned int getSize();
> +
> +    void insertSegmentAfter(ElfSegment *previous, ElfSegment *segment) {
> +        std::vector<ElfSegment *>::iterator prev = std::find(segments.begin(), segments.end(), previous);
> +        segments.insert(++prev, segment);

Nit: I think |prev + 1| is clearer here in declaring intent and avoiding side-effects.

Comment 8

[Mike Hommey [:glandium]]

Attachment: part 2 - Move the PT_LOAD splitting logic in elfhack.cpp. section offsets are not adjusted until the split is done.

How about this? (I removed 0x1000 altogether, and used the PT_LOAD alignment instead,
which is better)

Comment 9

[Mike Hommey [:glandium]]

Attachment: part 4 - Add a -r option to elfhack that re-merges the split PT_LOADs. are positioned accordingly, which means the resulting ELF binary will

have a big gap full of zero between .rel.plt and .plt.


I added a fprintf.

Comment 10

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae98ad3b851
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ad94351bf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ebcd9235d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9bc6febd7f

(there was a small change in part 2, because the build would fail otherwise when trying to apply elfhack on e.g. libmozalloc.so)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ae98ad3b851
https://hg.mozilla.org/mozilla-central/rev/84ad94351bf3
https://hg.mozilla.org/mozilla-central/rev/d2ebcd9235d1
https://hg.mozilla.org/mozilla-central/rev/0e9bc6febd7f

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 686571 [details] [diff] [review]
part 1 - When inserting a section before or after another, also insert it in the segments containing that other.

[Approval Request Comment]
This patch is required for bug 822584
Testing completed: It has been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): Low. This patch doesn't have any impact on functionality. It only adds an API to elfhack (it's a build system change only, elfhack being a build tool). 
String or UUID changes made by this patch: None

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 687662 [details] [diff] [review]
part 2 - Move the PT_LOAD splitting logic in elfhack.cpp.  section offsets are not adjusted until the split is done.

[Approval Request Comment]
This patch is required for bug 822584
Testing completed: It has been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): Low. This patch doesn't have any impact on functionality. It only moves code around in elfhack (it's a build system change only, elfhack being a build tool).
String or UUID changes made by this patch: None

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 686573 [details] [diff] [review]
part 3 - Allocate Elf instance on stack in do_file()

[Approval Request Comment]
This patch is required for bug 822584
Testing completed: It has been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): Low. This patch doesn't have any impact on functionality. It only refactors a part of elfhack (it's a build system change only, elfhack being a build tool). 
String or UUID changes made by this patch: None

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 687663 [details] [diff] [review]
part 4 - Add a -r option to elfhack that re-merges the split PT_LOADs.  are positioned accordingly, which means the resulting ELF binary will

[Approval Request Comment]
This patch is required for bug 822584
Testing completed: It has been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): Low. This patch only adds new API and feature to elfhack (it's a build system change only, elfhack being a build tool). The new feature is not used at build time.
String or UUID changes made by this patch: None

Comment 16

[Mike Hommey [:glandium]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bad85e46f9d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f952d7c6bb1
https://hg.mozilla.org/releases/mozilla-aurora/rev/0881ada9a749
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7f812ae0ab1

Comment 17

[Mike Hommey [:glandium]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ac6b5cacbfec
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a82128610aa6
https://hg.mozilla.org/releases/mozilla-b2g18/rev/704dddf018e5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3598645a878d

Comment 18

[Mike Hommey [:glandium]]

After so long without activity, let's say this is fixed. I'll open a new bug if I ever want to fix what I had left this bug open for.