Last Comment Bug 651892 - [elfhack] Remove __cxa_pure_virtual relocations
: [elfhack] Remove __cxa_pure_virtual relocations
Status: RESOLVED FIXED
[ts]
: mobile, perf
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla7
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: 622908
  Show dependency treegraph
 
Reported: 2011-04-21 09:50 PDT by Mike Hommey [:glandium]
Modified: 2011-06-27 21:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Add a helper function to lookup symbols in a ElfSymtab_Section (3.42 KB, patch)
2011-06-21 18:31 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 2 - Add a constructor to serializable that takes a buffer instead of an istream (1.86 KB, patch)
2011-06-21 18:32 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 3 - Remove __cxa_pure_virtual relocations (6.06 KB, patch)
2011-06-21 18:37 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-04-21 09:50:02 PDT
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 Benjamin Smedberg [:bsmedberg] 2011-04-21 09:56:48 PDT
We'd do this by postprocessing the objs somehow, instead of hacking the compiler?
Comment 2 Mike Hommey [:glandium] 2011-04-21 10:00:05 PDT
(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.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2011-04-21 10:02:33 PDT
This means we'd never get purecall crashes, right? We'd just crash with a NULL deref?
Comment 4 Mike Hommey [:glandium] 2011-04-21 10:10:33 PDT
(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.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-21 17:56:28 PDT
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?
Comment 6 Mike Hommey [:glandium] 2011-04-21 23:39:32 PDT
(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 Benjamin Smedberg [:bsmedberg] 2011-04-22 06:09:14 PDT
All of the interfaces have only pure-virtuals. We already emit NS_NO_VTABLE for them.
Comment 8 Mike Hommey [:glandium] 2011-06-21 18:31:57 PDT
Created attachment 540942 [details] [diff] [review]
part 1 - Add a helper function to lookup symbols in a ElfSymtab_Section
Comment 9 Mike Hommey [:glandium] 2011-06-21 18:32:18 PDT
Created attachment 540943 [details] [diff] [review]
part 2 - Add a constructor to serializable that takes a buffer instead of an istream
Comment 10 Mike Hommey [:glandium] 2011-06-21 18:37:52 PDT
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).
Comment 11 Mike Hommey [:glandium] 2011-06-21 18:45:42 PDT
(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 (dormant account) 2011-06-21 20:47:54 PDT
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.
Comment 13 (dormant account) 2011-06-21 20:52:22 PDT
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;
Comment 14 (dormant account) 2011-06-21 20:54:26 PDT
Comment on attachment 540946 [details] [diff] [review]
part 3 - Remove __cxa_pure_virtual relocations

rubberstamp
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-27 15:01:07 PDT
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)?
Comment 19 Mike Hommey [:glandium] 2011-06-27 21:12:31 PDT
(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.

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