Allow to revert the effects of elfhack

RESOLVED FIXED in mozilla20

Status

defect
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla20
All
Linux

Firefox Tracking Flags

(b2g18 fixed, b2g18-v1.0.0 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

2.69 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.94 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.88 KB, patch
khuey
: review+
Details | Diff | Splinter Review
14.81 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.54 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 2

7 years ago
Also, section offsets are not adjusted until the split is done.
Attachment #686572 - Flags: review?(nfroyd)
(Assignee)

Comment 4

7 years ago
Sections are positioned accordingly, which means the resulting ELF binary will
have a big gap full of zero between .rel.plt and .plt.
Attachment #686575 - Flags: review?(nfroyd)
(Assignee)

Comment 5

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
Attachment #686576 - Flags: review?(khuey)
Attachment #686573 - Flags: review?(nfroyd) → review+
Attachment #686571 - Flags: review?(nfroyd) → review+
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.
Attachment #686572 - Flags: review?(nfroyd) → review+
Attachment #686575 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

7 years ago
How about this? (I removed 0x1000 altogether, and used the PT_LOAD alignment instead,
which is better)
Attachment #687662 - Flags: review?(nfroyd)
(Assignee)

Updated

7 years ago
Attachment #686572 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #686575 - Attachment is obsolete: true
Attachment #687662 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [leave open]
(Assignee)

Updated

7 years ago
Blocks: 822584
(Assignee)

Comment 12

7 years ago
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
Attachment #686571 - Flags: approval-mozilla-b2g18?
Attachment #686571 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

7 years ago
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
Attachment #687662 - Flags: approval-mozilla-b2g18?
Attachment #687662 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

7 years ago
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
Attachment #686573 - Flags: approval-mozilla-b2g18?
Attachment #686573 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

7 years ago
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
Attachment #687663 - Flags: approval-mozilla-b2g18?
Attachment #687663 - Flags: approval-mozilla-aurora?
Attachment #686571 - Flags: approval-mozilla-b2g18?
Attachment #686571 - Flags: approval-mozilla-b2g18+
Attachment #686571 - Flags: approval-mozilla-aurora?
Attachment #686571 - Flags: approval-mozilla-aurora+
Attachment #686573 - Flags: approval-mozilla-b2g18?
Attachment #686573 - Flags: approval-mozilla-b2g18+
Attachment #686573 - Flags: approval-mozilla-aurora?
Attachment #686573 - Flags: approval-mozilla-aurora+
Attachment #687662 - Flags: approval-mozilla-b2g18?
Attachment #687662 - Flags: approval-mozilla-b2g18+
Attachment #687662 - Flags: approval-mozilla-aurora?
Attachment #687662 - Flags: approval-mozilla-aurora+
Attachment #687663 - Flags: approval-mozilla-b2g18?
Attachment #687663 - Flags: approval-mozilla-b2g18+
Attachment #687663 - Flags: approval-mozilla-aurora?
Attachment #687663 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 18

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla20

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.