[elfhack] Remove __cxa_pure_virtual relocations

RESOLVED FIXED in mozilla7

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug, {mobile, perf})

Trunk
mozilla7
All
Linux
mobile, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
All pure virtual members of classes (those declared as virtual type foo(...) = 0;) are set to a __cxa_pure_virtual function in libstdc++. This means we have as many relocations to point there during startup, while these functions are meant never to be called.
We can safely replace them with NULL (or some poison value) pointers, and avoid these relocations altogether.

Comment 1

6 years ago
We'd do this by postprocessing the objs somehow, instead of hacking the compiler?
(Assignee)

Comment 2

6 years ago
(In reply to comment #1)
> We'd do this by postprocessing the objs somehow, instead of hacking the
> compiler?

Yes, with elfhack. It's pretty straightforward to do.
This means we'd never get purecall crashes, right? We'd just crash with a NULL deref?
(Assignee)

Comment 4

6 years ago
(In reply to comment #3)
> This means we'd never get purecall crashes, right? We'd just crash with a NULL
> deref?

yes. Which is why a fixed poison value could be better.
MSVC has the ability to not emit vtables for classes with pure virtual members (provided you mark them for the compiler).  Does gcc have a similar capability?
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> MSVC has the ability to not emit vtables for classes with pure virtual members
> (provided you mark them for the compiler).  Does gcc have a similar capability?

It doesn't look like so, and I am doubtful that there are many vtables with pure virtual members only.

Comment 7

6 years ago
All of the interfaces have only pure-virtuals. We already emit NS_NO_VTABLE for them.
Blocks: 622908
Keywords: mobile, perf
Whiteboard: [ts]
(Assignee)

Comment 8

6 years ago
Created attachment 540942 [details] [diff] [review]
part 1 - Add a helper function to lookup symbols in a ElfSymtab_Section
Attachment #540942 - Flags: review?(tglek)
(Assignee)

Comment 9

6 years ago
Created attachment 540943 [details] [diff] [review]
part 2 - Add a constructor to serializable that takes a buffer instead of an istream
Attachment #540943 - Flags: review?(tglek)
(Assignee)

Comment 10

6 years ago
Created attachment 540946 [details] [diff] [review]
part 3 - Remove __cxa_pure_virtual relocations

We have a tad more than 27000 relocations due to __cxa_pure_virtual. On our linux builds, where we dynamically link against libstdc++, these are R_386_32 and R_X86_64_64 relocations, which weren't packed by elfhack, so we further save 27000 * 8 on x86 (216000+) and 27000 * 24 on x64 (648000) on libxul.so. On android, we statically link against libstdc++, which means we had R_ARM_RELATIVE relocations, which were already packed by elfhack. We however save the need to apply these 27000 relocations (which is more than 10% of the total number of relocations), and further save 8KB on libxul.so. We also open the way for better packing of vtables (bug 651894).
Attachment #540946 - Flags: review?(tglek)
(Assignee)

Comment 11

6 years ago
(In reply to comment #9)
> Created attachment 540943 [details] [diff] [review] [review]
> part 2 - Add a constructor to serializable that takes a buffer instead of an
> istream

Note that this kind of clutters the serializable template, but I've also had a WIP for quite some time that gets rid of the constructor using istream.

Comment 12

6 years ago
Comment on attachment 540942 [details] [diff] [review]
part 1 - Add a helper function to lookup symbols in a ElfSymtab_Section


> 
>+Elf_SymValue *
>+ElfSymtab_Section::lookup(const char *name, unsigned int type_filter)
>+{
>+    for (std::vector<Elf_SymValue>::iterator sym = syms.begin();
>+         sym != syms.end(); sym++) {
>+        if ((strcmp(sym->name, name) == 0) &&
>+            (type_filter & (1 << ELF32_ST_TYPE(sym->info)))) {
>+            return &*sym;
>+        }
>+    }
>+    return NULL;
>+}
>+

nit. Usually one does cheap part of the conditional first and then the string comparison.
Attachment #540942 - Flags: review?(tglek) → review+

Comment 13

6 years ago
Comment on attachment 540943 [details] [diff] [review]
part 2 - Add a constructor to serializable that takes a buffer instead of an istream

># HG changeset patch
># User Mike Hommey <mh+mozilla@glandium.org>
># Date 1308706295 -7200
># Node ID 0195006431758b67bb79e6fcb50fb7304a70451e
># Parent  1ae096a8fa5e2195cbf6a316e78c6485de624945
>Bug 651892 part 2 - Add a constructor to serializable that takes a buffer instead of an istream
>
>diff --git a/elfxx.h b/elfxx.h
>--- a/elfxx.h
>+++ b/elfxx.h
>@@ -185,16 +185,42 @@ public:
>     ElfSection *getSection() { return section; }
> };
> 
> template <typename T>
> class serializable: public T::Type32 {
> public:
>     serializable() {};
>     serializable(const typename T::Type32 &p): T::Type32(p) {};
>+    serializable(const char *buf, size_t len, char ei_class, char ei_data)
>+    {
>+        if (ei_class == ELFCLASS32) {
>+            typename T::Type32 e;
>+            assert(len <= sizeof(e));
>+            memcpy(&e, buf, sizeof(e));
>+            if (ei_data == ELFDATA2LSB) {
>+                T::template swap<little_endian>(e, *this);
>+            } else if (ei_data == ELFDATA2MSB) {
>+                T::template swap<big_endian>(e, *this);
>+            }
>+            return;
>+        } else if (ei_class == ELFCLASS64) {
>+            typename T::Type64 e;
>+            assert(len <= sizeof(e));
>+            memcpy(&e, buf, sizeof(e));
>+            if (ei_data == ELFDATA2LSB) {
>+                T::template swap<little_endian>(e, *this);
>+            } else if (ei_data == ELFDATA2MSB) {
>+                T::template swap<big_endian>(e, *this);
>+            }
>+            return;
Feels like instead of copying code around should use a template for the if body and let the compiler do the copying.
>+        }
>+        throw std::runtime_error("Unsupported ELF class or data encoding");
>+    }
>+
>     serializable(std::ifstream &file, char ei_class, char ei_data)
>     {
>         if (ei_class == ELFCLASS32) {
>             typename T::Type32 e;
>             file.read((char *)&e, sizeof(e));
>             if (ei_data == ELFDATA2LSB) {
>                 T::template swap<little_endian>(e, *this);
>                 return;
Attachment #540943 - Flags: review?(tglek) → review+

Comment 14

6 years ago
Comment on attachment 540946 [details] [diff] [review]
part 3 - Remove __cxa_pure_virtual relocations

rubberstamp
Attachment #540946 - Flags: review?(tglek) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/81641a0154da
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/963e1f587112
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/34e6f45571cc
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/080a8d067889
http://hg.mozilla.org/integration/mozilla-inbound/rev/9bf9fb1544bc
http://hg.mozilla.org/integration/mozilla-inbound/rev/8b6ea9fa6173
Whiteboard: [ts] → [ts] [inbound]
http://hg.mozilla.org/mozilla-central/rev/080a8d067889
http://hg.mozilla.org/mozilla-central/rev/9bf9fb1544bc
http://hg.mozilla.org/mozilla-central/rev/8b6ea9fa6173
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [ts] [inbound] → [ts]
Target Milestone: --- → mozilla7
Is the way pure virtual function calls now crash:
 * reliably safe (i.e., not exploitable)?
 * easily detectable when looking at a crash report (i.e., distingishable from other types of crashes)?
(Assignee)

Comment 19

6 years ago
(In reply to comment #18)
> Is the way pure virtual function calls now crash:
>  * reliably safe (i.e., not exploitable)?
>  * easily detectable when looking at a crash report (i.e., distingishable
> from other types of crashes)?

It ends up being a jump to address 0x0, so as long as there's nothing mapped at this address, it's safe. It's detectable by the fact that the crashing instruction is at address 0x0, but it's not distinguishable from other 0x0 jumps if there are. As I said in comment 0 and comment 4, we can use a poison value instead of 0x0 if necessary. This can be done in a followup.
You need to log in before you can comment on or make changes to this bug.