683127 - New custom ELF linker

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

This is something I had spotted in my previous linker hacking, and it seems Google is working on binary incompatible changes to the linker structures that effectively make Firefox crash with it. The problem is that our linker uses data it gets from the system linker as if it were exactly in the format it knows, which assumes the system linker would always be binary compatible with whatever revision we forked from.

The binary incompatible changes are however not planned for Ice Cream Sandwich, but probably for the following release, which fortunately leaves us some time to get things right.

Comment 1

[Mike Hommey [:glandium]]

Attachment: [WIP] Proof of concept

This works on desktop x86, x86-64 and Android. On desktop builds, it breaks nss tools because nss requires the linker, but the nss tools don't provide it. I also expect the plugin-container to crash at startup because it actually is linked against the libraries (that is, it doesn't dlopen them).
On Android, the linker doesn't share memory between main and content processes like the current linker does, but considering incremental decompression is going to require serious changes to that anyways, it's not worth doing memory sharing until then. Plus, for the short term, mobile is not going to use a separate content process.
In both cases, it doesn't actually dlclose() properly, and many cases may lead to memory leaks, so it still needs some cleanup.
The nice thing is that despite it being completely not optimized (most notably, it doesn't try to reduce the amount of symbol resolution (it ends up doing symbols resolution several times in the same libraries), and it doesn't do dynamic symbol resolution for plt calls like the current linker does), it manages to be (slightly) faster than the current linker, probably because it doesn't load libfreebl and libsoftokn until actually required.
I think the ZipContent part needs serious changes to handle various cases (such as forcing decompression for the debug intent).

Comment 2

[Mike Hommey [:glandium]]

I forgot to mention, on android, the system linker's dlsym doesn't return values for defined weak symbols, which, unfortunately, we do required. Fortunately, it made me realize that we do need to handle __cxa_atexit for dlclose to work properly (the weak symbol we fail to dlsym is __aeabi_atexit, which is the ARM ABI equivalent). The PoC patch just defines an _aeabi_atexit dummy function that can be resolved at runtime (and breaking the shutdown path as aside effect).

Comment 3

[(dormant account)]

I realize this is a WiP, but 
+void ElfLinker::SetZip(const char *aZip)
+{
+  zip = new Zip(aZip);
+}

let hope final version uses RAII or something more robust than above.

Comment 4

[(dormant account)]

This also needs more comments.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #3)
> I realize this is a WiP, but 
> +void ElfLinker::SetZip(const char *aZip)
> +{
> +  zip = new Zip(aZip);
> +}
> 
> let hope final version uses RAII or something more robust than above.

Yeah, initialization and shutdown paths are currently not handled very well. FWIW, I think it should initialize itself on some environment variables instead of using that SetZip.

(In reply to Taras Glek (:taras) from comment #4)
> This also needs more comments.

The lack of comments was deliberate, until I have something stable enough code-wise. Next iteration will be properly documented.

Comment 6

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker

Comment 7

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker

Comment 8

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker

Fixed an erroneous magic number

Comment 9

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker

Comment 10

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker

Oops

Comment 11

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker

End-of-day re-oops

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 580924 [details] [diff] [review]
part 2 - Build system glue for the new linker

Forget it, i'm tired.

Comment 13

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker

This should be the one.

Comment 14

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker

A few modifications I had forgotten

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 580953 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

Turns out this only works because our apk is a weird Zip. I'm going to fix the implementation to work with more standard Zips.

Comment 16

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker

Comment 17

[Mike Hommey [:glandium]]

Attachment: part 3 - Test for the Zip reader

Comment 18

[Mike Hommey [:glandium]]

Attachment: part 4 - Use the new ELF linker Zip reader in the old linker

Comment 19

[Mike Hommey [:glandium]]

Attachment: part 3 - Test for the Zip reader

Comment 20

[(dormant account)]

Comment on attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

>+    static const T *cast(const void *buf)
>+    {
>+      const T *ret = static_cast<const T *>(buf);
>+      if (ret->signature == T::magic)
>+        return ret;
>+      return NULL;
>+    }

I'd call this validate since it's not a cast

Comment 21

[(dormant account)]

Comment on attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

There is very little bounds checking in this, but I guess we can trust the file we are executing from.
Have you measured a speedup from bypassing reading the central directory? Are the files actually laid out in the startup order in apk so the hot path is always used?

Seems ok given that this code only needs to be good enough to read the apk.

Comment 22

[Michael Wu [:mwu]]

Comment on attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

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

Looks mostly fine to me.

::: mozutils/linker/Zip.cpp
@@ +84,5 @@
> +{
> +  if (mapped != MAP_FAILED) {
> +    munmap(mapped, size);
> +    debug("Unmapped %s @%p", name, mapped);
> +    free(name);

name looks like it's leaked when mapped == MAP_FAILED.

@@ +113,5 @@
> +    out->type = static_cast<Stream::Type>(uint16_t(nextFile->compression));
> +
> +    /* Find the next Local File header. It is usually simply following the
> +     * compressed stream, but in cases where the 3rd bit of the generalFlag
> +     * is set, there is a Data Descriptor header before. */

Can you just test that flag instead of trying both options?

@@ +128,5 @@
> +   * Directory for the entry corresponding to the given path */
> +  if (!nextDir || !nextDir->GetName().Equals(path)) {
> +    const DirectoryEntry *entry = GetFirstEntry();
> +    debug("%s - Scan directory entries in search for %s", name, path);
> +    for (; entry && !entry->GetName().Equals(path); entry = entry->GetNext());

I think a while loop would look more normal, but hey, it's all the same..

Comment 23

[Michael Wu [:mwu]]

Comment on attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

bugzilla fail

Comment 24

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker.

Addressed mwu and taras' comments

Comment 25

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker.

Refreshed with a few minor changes

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 583143 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker.

Carrying over r+

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 583144 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+

Comment 28

[Mike Hommey [:glandium]]

Attachment: part 1.5 - Add bookkeeping for Zips

This is meant to be folded with part 1, but is easier to review separately. This adds bookkeeping for Zip instances (will be used by the linker) and a RAII wrapper for file descriptors.

Comment 29

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker.

Some changes in configure.in

Comment 30

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose

Comment 31

[Mike Hommey [:glandium]]

Attachment: part 6 - Use wrapped dl* functions in the XPCOM standalone glue when building with MOZ_LINKER on Linux

The sXULLibHandle change could be ifdef MOZ_LINKER, but it actually doesn't hurt to change everywhere. I limited to GLIBC, though, because I'm not sure what other dlopen implementations do.

Comment 32

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Comment 33

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name

This is the main part of the linker. That is, the linker itself.

Comment 34

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Comment 35

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name

Comment 36

[Mike Hommey [:glandium]]

Attachment: part 8 - Allow to load Elf files from a Zip archive

Comment 37

[Mike Hommey [:glandium]]

Attachment: part 9 - Enable the new linker on Android Native UI

This last part is not completely ready. The debug intent is not going to be very useful due to the lack of hook for gdb.

Comment 38

[Mike Hommey [:glandium]]

Please note that one problem that I know of that is not marked with a TODO in the patches is that fini_array functions are not called in reverse order as they ought to. Fortunately, the only library we have that has a fini_array has only one entry in it, so it's not a problem that is going to affect us. Most C++ code does destruction with functions registered with __cxa_atexit anyways.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 584737 [details] [diff] [review]
part 2 - Build system glue for the new linker.

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

I thought we didn't like system zlib?

Comment 40

[Mike Hommey [:glandium]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #39)
> Comment on attachment 584737 [details] [diff] [review]
> part 2 - Build system glue for the new linker.
> 
> Review of attachment 584737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I thought we didn't like system zlib?

We already use it on android. And for the moment, the elf linker is there for debugging only on desktop, so in practice, zlib is not being required here. That being said, no one has been able to tell me if there was a compelling reason why we don't use the system zlib on linux nowadays. The alternative would be to link mozzlib in mozglue, but that's kind of gross.

Comment 41

[(dormant account)]

Comment on attachment 584533 [details] [diff] [review]
part 1.5 - Add bookkeeping for Zips

Why aren't you using std::find? instead of for loops. 

Overall this is looking nicer than our ZipArchive stuff.

Why are there multiple zipfiles involved in library loading?

Comment 42

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #41)
> Comment on attachment 584533 [details] [diff] [review]
> part 1.5 - Add bookkeeping for Zips
> 
> Why aren't you using std::find? instead of for loops.

Because in the long term I want to remove libstdc++ requirement.

> Overall this is looking nicer than our ZipArchive stuff.
> 
> Why are there multiple zipfiles involved in library loading?

There aren't. It's future proofing, to allow loading components from extensions XPIs.

Comment 43

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #42)
> (In reply to Taras Glek (:taras) from comment #41)
> > Comment on attachment 584533 [details] [diff] [review]
> > part 1.5 - Add bookkeeping for Zips
> > 
> > Why aren't you using std::find? instead of for loops.
> 
> Because in the long term I want to remove libstdc++ requirement.

Though now that I look at it, it doesn't look like this pulls stuff from libstdc++. It looks like it's all defined in headers.

Comment 44

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #34)
> Created attachment 584995 [details] [diff] [review]
> part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Looks plausible, to the limited extent that I can tell

+  /* Build up a list of all library handles with direct (external) references.
+   * We actually skip system library handles because we want to keep at least
+   * some of these open. Most notably, Mozilla codebase keeps a few libgnome
+   * libraries deliberately open because of the mess that libORBit destruction
+   * is. dlclose()ing these libraries actually leads to problems. */

Where is the logic / list of libraries that are never closed?


+  void ReleaseDirectRef()
+      // ASSERT(directRefCnt >= mozilla::RefCounted<LibHandle>::refCount())

why is this commented out?  it looks useful.   ditto in
+  void ReleaseAllDirectRefs()

Comment 45

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #35)
> Created attachment 584996 [details] [diff] [review]
> part 7 - Use a custom Elf linker for libraries given with an absolute path
> name

various instances of
+#ifndef __GLIBC__

is that the right guarding symbol?  I suppose it probably is if it is
guarding glibc compatibility hacks.



+CustomElf::LoadSegment(const Phdr *pt_load) const
+#if PF_X == PROT_READ && PF_W == PROT_WRITE && PF_R == PROT_EXEC
+  // Optimized version of the following, in the standard case where
+  // read and exec flags are simply inverted between PF_* and PROT_*.
+  prot = ((((prot >> 2) & 1) | (prot << 2)) & 0x7) | (prot & 2);
+#else
+  prot = ((prot & PF_X) ? PROT_EXEC : 0) ||
+          ((prot & PF_W) ? PROT_WRITE : 0) ||
+          ((prot & PF_R) ? PROT_READ : 0);
+#endif

Is the optimised version really necessary?  How many times will
this get executed per Fx startup -- 15 libraries x (say) 5 segments
per library ?

Comment 46

[Julian Seward [:jseward]]

(In reply to Julian Seward from comment #45)
> (In reply to Mike Hommey [:glandium] from comment #35)
> > Created attachment 584996 [details] [diff] [review]
> > part 7 - Use a custom Elf linker for libraries given with an absolute path
> > name

Duh, I missed copy-pasting some comments.  More comments for this patch:

+class CustomElf: public LibHandle
+   * content. The file descriptor is fully appropriated, and will be closed

What does "fully appropriated" mean?  (is also used below).  Not sure
what appropriated means.


+class UnboundArray

Would UnsizedArray be a better name?  I know you mean "array that 
does not have an upper bound", but it also gives the intuition
"array that has not been bound to anything", whatever that might
mean.

Comment 47

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #36)
> Created attachment 584997 [details] [diff] [review]
> part 8 - Allow to load Elf files from a Zip archive

+/**
+ * RAII wrapper for a mapping of the first page off a Mappable object.
+ * This calls Mappable::munmap instead of system munmap.
+ */
+class Mappable1stPagePtr: public GenericMappedPtr<Mappable1stPagePtr> {

Is this the/a replacement for AutoMunmap in the 27 Dec patch set?
Ifo so it will be too limited in some cases, eg if I want to traverse
the section header table and hence the section header's string table,
in order to pick up the .text VMA to calculate "add-symbol-file" addresses
for GDB/Valgrind etc, since IIUC the section header table can be anywhere in the file.


Sanity check: in the 27 Dec patch, 

bool
CustomElf::LoadSegment(const Phdr *pt_load,
has
      if (mprotect(GetPtr(next_page), mem_end - next_page, flags) < 0) {

is it correct that this is a direct-to-the-kernel mprotect, and
does not need to be against a Mappable ?  This patch also does not
change it to be a against-Mappable operation.

Comment 48

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #44)
> (In reply to Mike Hommey [:glandium] from comment #34)
> > Created attachment 584995 [details] [diff] [review]
> > part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.
> 
> Looks plausible, to the limited extent that I can tell
> 
> +  /* Build up a list of all library handles with direct (external)
> references.
> +   * We actually skip system library handles because we want to keep at
> least
> +   * some of these open. Most notably, Mozilla codebase keeps a few libgnome
> +   * libraries deliberately open because of the mess that libORBit
> destruction
> +   * is. dlclose()ing these libraries actually leads to problems. */
> 
> Where is the logic / list of libraries that are never closed?

The list most probably varies between versions. Most (all?) are GNOME libraries.

> +  void ReleaseDirectRef()
> +      // ASSERT(directRefCnt >= mozilla::RefCounted<LibHandle>::refCount())
> 
> why is this commented out?  it looks useful.   ditto in
> +  void ReleaseAllDirectRefs()

The ASSERTs are commented because I want to mozilla/Assertions.h, but it uses JS_Assert at the moment.(In reply to Julian Seward from comment #45)
> (In reply to Mike Hommey [:glandium] from comment #35)
> > Created attachment 584996 [details] [diff] [review]
> > part 7 - Use a custom Elf linker for libraries given with an absolute path
> > name
> 
> various instances of
> +#ifndef __GLIBC__
> 
> is that the right guarding symbol?  I suppose it probably is if it is
> guarding glibc compatibility hacks.

Indeed.

> +CustomElf::LoadSegment(const Phdr *pt_load) const
> +#if PF_X == PROT_READ && PF_W == PROT_WRITE && PF_R == PROT_EXEC
> +  // Optimized version of the following, in the standard case where
> +  // read and exec flags are simply inverted between PF_* and PROT_*.
> +  prot = ((((prot >> 2) & 1) | (prot << 2)) & 0x7) | (prot & 2);
> +#else
> +  prot = ((prot & PF_X) ? PROT_EXEC : 0) ||
> +          ((prot & PF_W) ? PROT_WRITE : 0) ||
> +          ((prot & PF_R) ? PROT_READ : 0);
> +#endif
> 
> Is the optimised version really necessary?

Probably not, but does it hurt to keep it?

(In reply to Julian Seward from comment #46)
> (In reply to Julian Seward from comment #45)
> > (In reply to Mike Hommey [:glandium] from comment #35)
> > > Created attachment 584996 [details] [diff] [review]
> > > part 7 - Use a custom Elf linker for libraries given with an absolute path
> > > name
> 
> Duh, I missed copy-pasting some comments.  More comments for this patch:
> 
> +class CustomElf: public LibHandle
> +   * content. The file descriptor is fully appropriated, and will be closed
> 
> What does "fully appropriated" mean?  (is also used below).  Not sure
> what appropriated means.

Ownership of the object is taken by the CustomElf.

> +class UnboundArray
> 
> Would UnsizedArray be a better name?

Yes, that sounds better.

(In reply to Julian Seward from comment #47)
> (In reply to Mike Hommey [:glandium] from comment #36)
> > Created attachment 584997 [details] [diff] [review]
> > part 8 - Allow to load Elf files from a Zip archive
> 
> +/**
> + * RAII wrapper for a mapping of the first page off a Mappable object.
> + * This calls Mappable::munmap instead of system munmap.
> + */
> +class Mappable1stPagePtr: public GenericMappedPtr<Mappable1stPagePtr> {
> 
> Is this the/a replacement for AutoMunmap in the 27 Dec patch set?
> Ifo so it will be too limited in some cases, eg if I want to traverse
> the section header table and hence the section header's string table,
> in order to pick up the .text VMA to calculate "add-symbol-file" addresses
> for GDB/Valgrind etc, since IIUC the section header table can be anywhere in
> the file.

You don't need to traverse the section header table with the r_debug patch I sent you.

> Sanity check: in the 27 Dec patch, 
> 
> bool
> CustomElf::LoadSegment(const Phdr *pt_load,
> has
>       if (mprotect(GetPtr(next_page), mem_end - next_page, flags) < 0) {
> 
> is it correct that this is a direct-to-the-kernel mprotect, and
> does not need to be against a Mappable ?  This patch also does not
> change it to be a against-Mappable operation.

Yes, this mprotects anonymous memory.

Comment 49

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #48)

> The ASSERTs are commented because I want to mozilla/Assertions.h, but it
> uses JS_Assert at the moment.

MappableSZ.cpp has a whole bunch of assertions, which will have the same problem,
so it would be good to get a solution to this.


> > +CustomElf::LoadSegment(const Phdr *pt_load) const
> > +#if PF_X == PROT_READ && PF_W == PROT_WRITE && PF_R == PROT_EXEC
> > +  // Optimized version of the following, in the standard case where
> > +  // read and exec flags are simply inverted between PF_* and PROT_*.
> > +  prot = ((((prot >> 2) & 1) | (prot << 2)) & 0x7) | (prot & 2);
> > +#else
> > +  prot = ((prot & PF_X) ? PROT_EXEC : 0) ||
> > +          ((prot & PF_W) ? PROT_WRITE : 0) ||
> > +          ((prot & PF_R) ? PROT_READ : 0);
> > +#endif
> > 
> > Is the optimised version really necessary?
> 
> Probably not, but does it hurt to keep it?

Well, yes .. it makes the code generally longer and more difficult to
understand in return for no clear benefit.

Actually (just spotted this) in the general case shouldn't those ||s be |s ?


> > What does "fully appropriated" mean?  (is also used below).  Not sure
> > what appropriated means.
> 
> Ownership of the object is taken by the CustomElf.

ok .. I didn't get that from the comment.  I know what misappropriated
means (stolen, basically) but not appropriated.


> > Is this the/a replacement for AutoMunmap in the 27 Dec patch set?
> > Ifo so it will be too limited in some cases, eg if I want to traverse
> > the section header table and hence the section header's string table,
> > in order to pick up the .text VMA to calculate "add-symbol-file" addresses
> > for GDB/Valgrind etc, since IIUC the section header table can be anywhere in
> > the file.
> 
> You don't need to traverse the section header table with the r_debug patch I
> sent you.

Yeah .. I realised that after posting the above comment :-(

Comment 50

[Julian Seward [:jseward]]

Mike, pls can you indicate which of these patches need to be applied in
which order?  I'd like to rebase my stuff against this, but it's not obvious
(to me) how to get to a suitable starting point.

Comment 51

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #50)
> Mike, pls can you indicate which of these patches need to be applied in
> which order?  I'd like to rebase my stuff against this, but it's not obvious
> (to me) how to get to a suitable starting point.

All of them, in the order of their part number. They replace the bug683127-wip and subsequent patches I sent you. You'll still need the dependencies patch (which, in bugzilla, is scattered across the dependencies to this bug).

(In reply to Julian Seward from comment #49)
> Actually (just spotted this) in the general case shouldn't those ||s be |s ?

yes.

Comment 52

[(dormant account)]

Comment on attachment 584995 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

seems like if you made ReleaseDirectRef() return a boolean, ReleaseAllDirectRefs would not be needed.

This looks ok. Please ask Julian for subsequent reviews since he's actually working with the code.

Comment 53

[(dormant account)]

Comment on attachment 584996 [details] [diff] [review]
part 7 - Use a custom Elf linker for libraries given with an absolute path name

I don't know about linkers to honestly r+ this patch. I don't see anything wrong beyond Julian's review. He can review followups.

Comment 54

[(dormant account)]

Comment on attachment 584997 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive

this is a pretty cool hack

Comment 55

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker.

Refreshed against bug 701371, changed headers to MPL 2.0 (yay for shortened licensing boilerplate), merged part 1.5 and replaced a loop with std::find.

Comment 56

[Mike Hommey [:glandium]]

Comment on attachment 586914 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker.

Carrying over r+

Comment 57

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker.

Refreshed against bug 701371.

Comment 58

[Mike Hommey [:glandium]]

Comment on attachment 586916 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+

Comment 59

[Mike Hommey [:glandium]]

Attachment: part 3 - Test for the Zip reader.

Refreshed against bug 701371.

Comment 60

[Mike Hommey [:glandium]]

Comment on attachment 586918 [details] [diff] [review]
part 3 - Test for the Zip reader.

Carrying over r+

Comment 61

[Mike Hommey [:glandium]]

Attachment: part 3 - Test for the Zip reader.

Forgot to update the license boilerplates.

Comment 62

[Mike Hommey [:glandium]]

Comment on attachment 586919 [details] [diff] [review]
part 3 - Test for the Zip reader.

Carrying over r+

Comment 63

[Mike Hommey [:glandium]]

Attachment: part 4 - Use the new ELF linker Zip reader in the old linker.

Refreshed against bug 701371.

Comment 64

[Mike Hommey [:glandium]]

Comment on attachment 586921 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker.

Carrying over r+

Comment 65

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Applied the suggested change to ReleaseDirectRef and got rid of ReleaseAllDirectRefs.

Comment 66

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker.

Better std::find logic

Comment 67

[Mike Hommey [:glandium]]

Comment on attachment 586930 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker.

Carrying over r+

Comment 68

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Use std::find in ElfLoader::Forget, like in ZipCollection::Forget

Comment 69

[Mike Hommey [:glandium]]

Comment on attachment 586931 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Carrying over r+

Comment 70

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name.

Modified the CustomElf::Load comment, changed the flags setting in CustomElf::LoadSegment and renamed UnboundArray to UnsizedArray.

Comment 71

[Mike Hommey [:glandium]]

Attachment: part 8 - Allow to load Elf files from a Zip archive.

Refreshed against changes to other patches. (context changes only)

Comment 72

[Mike Hommey [:glandium]]

Comment on attachment 586936 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive.

Carrying over r+

Comment 73

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker.

Re-refreshed against bug 701371.

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 586942 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+

Comment 75

[Julian Seward [:jseward]]

Comment on attachment 586934 [details] [diff] [review]
part 7 - Use a custom Elf linker for libraries given with an absolute path name.

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

+#ifndef PAGE_SIZE
+#define PAGE_SIZE 4096
+#endif

is it ok to hardwire this instead of using sysconf() ?  At least for
some ppc64-linux and mips-linux setups, the page size isn't
necessarily 4k.


+unsigned long
+ElfHash(const char *symbol)
+{
+  unsigned long h = 0, g;
+  while (*symbol) {
+    h = (h << 4) + *symbol++;

Does the behaviour of this depend on the signedness of char?
Is the sign extension of *symbol independent of the signedness
of *symbol?  eg
(gdb) p (unsigned int)( (signed char) 0xFF )
$3 = 4294967295
(gdb) p (unsigned int)( (unsigned char) 0xFF )
$4 = 255


r+ provided the sign-extension thing is OK.  I'm not so bothered about the page size thing.

Comment 76

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #75)
> Comment on attachment 586934 [details] [diff] [review]
> part 7 - Use a custom Elf linker for libraries given with an absolute path
> name.
> 
> Review of attachment 586934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> +#ifndef PAGE_SIZE
> +#define PAGE_SIZE 4096
> +#endif
> 
> is it ok to hardwire this instead of using sysconf() ?  At least for
> some ppc64-linux and mips-linux setups, the page size isn't
> necessarily 4k.

But these systems are far from being supported at the moment.

> +unsigned long
> +ElfHash(const char *symbol)
> +{
> +  unsigned long h = 0, g;
> +  while (*symbol) {
> +    h = (h << 4) + *symbol++;
> 
> Does the behaviour of this depend on the signedness of char?
> Is the sign extension of *symbol independent of the signedness
> of *symbol?  eg
> (gdb) p (unsigned int)( (signed char) 0xFF )
> $3 = 4294967295
> (gdb) p (unsigned int)( (unsigned char) 0xFF )
> $4 = 255

There is indeed a problem here, and in fact the ELF ABI for SysV has an unsigned char argument. I wonder how I got it off.

Comment 77

[Mike Hommey [:glandium]]

Attachment: part 2 - Build system glue for the new linker.

Refreshed after bug 709776

Comment 78

[Mike Hommey [:glandium]]

Attachment: part 4 - Use the new ELF linker Zip reader in the old linker.

Refreshed after bug 709776

Comment 79

[Mike Hommey [:glandium]]

Attachment: part 9 - Enable the new linker on Android Native UI

Refreshed against bug 709776

Comment 80

[Mike Hommey [:glandium]]

Landed up to part 4
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f6fad66924
https://hg.mozilla.org/integration/mozilla-inbound/rev/85c7cdc1a916
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea78ef72f47f
https://hg.mozilla.org/integration/mozilla-inbound/rev/52edf4287893

This is basically what we're going to get if we end up disabling the new linker once this bug lands completely (through MOZ_OLD_LINKER in configure.in). I want it tested before (finishing and) landing the rest.

Comment 81

[Mike Hommey [:glandium]]

Comment on attachment 587378 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+

Comment 82

[Mike Hommey [:glandium]]

Comment on attachment 587379 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker.

Carrying over r+

Comment 83

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name.

Changed a couple debugging messages

Comment 84

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name.

Wrong log level in previous patch (sorry for the noise)

Comment 85

[Mike Hommey [:glandium]]

Attachment: part 8 - Allow to load Elf files from a Zip archive.

This adds a few debugging messages, removes the useless madvise (it was applied to the decompressed buffer, not the compressed buffer ; and since we don't know the compressed size of the requested decompressed length...) and fixes cacheflush (was done at the wrong moment, on a slightly wrong range)

Comment 86

[Mike Hommey [:glandium]]

Comment on attachment 587687 [details] [diff] [review]
part 7 - Use a custom Elf linker for libraries given with an absolute path name.

Carrying over r+

Comment 87

[Mike Hommey [:glandium]]

Comment on attachment 587688 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive.

Carrying over r+

Comment 88

[Mike Hommey [:glandium]]

Attachment: part 8 - Allow to load Elf files from a Zip archive.

Grah, there was a missing hunk

Comment 89

[Mike Hommey [:glandium]]

2 fixups for part 4 on Android debug:
https://hg.mozilla.org/mozilla-central/rev/d037afa60874
https://hg.mozilla.org/mozilla-central/rev/d85920b5691b

Comment 90

[Ed Morley [:emorley]]

Parts 1-4: (landed before comment 89)
https://hg.mozilla.org/mozilla-central/rev/12f6fad66924
https://hg.mozilla.org/mozilla-central/rev/85c7cdc1a916
https://hg.mozilla.org/mozilla-central/rev/ea78ef72f47f
https://hg.mozilla.org/mozilla-central/rev/52edf4287893

Comment 91

[Mike Hommey [:glandium]]

Attachment: part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

This allows valgrind to happily find symbols and allows to implement what we currently do with the android debug intent.

Comment 92

[Mike Hommey [:glandium]]

Attachment: part 10 - Allow debug symbols to be found under gdb without extracted libraries

Essentially, this is the same as what has been done in bug 687446, without relying on /proc/self/auxv (which happens not to be readable when android:debuggable is set to false (iow, all mozilla builds)) and without crashing under valgrind.

Comment 93

[Mike Hommey [:glandium]]

Attachment: part 11 - Hook the new linker in Android initialization

This hooks the new linker in APKOpen.cpp.

Comment 94

[Mike Hommey [:glandium]]

Attachment: part 12 - Enable the new linker on Android native UI

This disables the old linker on android native ui (thus enabling the new one). This is the part that is supposed to be backed out if things go wrong, the rest can stay.

Comment 95

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 588896 [details] [diff] [review]
part 11 - Hook the new linker in Android initialization

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

we should probably remove the lib cache code since its not used anymore. It would make your patch a bit cleaner.

Comment 96

[Mike Hommey [:glandium]]

(In reply to Brad Lassey [:blassey] from comment #95)
> we should probably remove the lib cache code since its not used anymore. It
> would make your patch a bit cleaner.

The patch series doesn't remove the old linker, and the old linker still uses it. The old linker is also needed for the xul builds, because the new linker doesn't support separate processes yet.

Comment 97

[(dormant account)]

Comment on attachment 588892 [details] [diff] [review]
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

if (extract && (extract[0] == '1') && (extract[1] == '\0')) <-- use strncmp. I'm not even sure why we care about contents of the env var, should just check for existence?

I think AutoAny should be AutoClean. We should add a class like this to gecko.

Comment 98

[(dormant account)]

Comment on attachment 588894 [details] [diff] [review]
part 10 - Allow debug symbols to be found under gdb without extracted libraries

Seems like you should use your new Auto traits thing for singleton forgetting and possibly GenericMappedPtr.

I think mwu should review the debug section magic in ElfLoader::InitDebugger()

Comment 99

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #97)
> Comment on attachment 588892 [details] [diff] [review]
> part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g.
> valgrind
> 
> if (extract && (extract[0] == '1') && (extract[1] == '\0')) <-- use strncmp.
> I'm not even sure why we care about contents of the env var, should just
> check for existence?

Checking for existence can lead to people getting unexpected behaviour when setting VAR= or VAR=0.

> I think AutoAny should be AutoClean. We should add a class like this to
> gecko.

I'll file a MFBT bug, then.

(In reply to Taras Glek (:taras) from comment #98)
> Comment on attachment 588894 [details] [diff] [review]
> part 10 - Allow debug symbols to be found under gdb without extracted
> libraries
> 
> Seems like you should use your new Auto traits thing for singleton
> forgetting and possibly GenericMappedPtr.

I wanted to, but since the MappedPtr stores more than one value, this is jumping through many hoops for limited added value.

Comment 100

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Minor fix.

Comment 101

[Mike Hommey [:glandium]]

Attachment: part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

The previous version I had sent didn't address comment 75.

Comment 102

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #101)
> Created attachment 589441 [details] [diff] [review]
> part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.
> 
> The previous version I had sent didn't address comment 75.

This comment was meant for a new part 7, not for part 5.

Comment 103

[Mike Hommey [:glandium]]

Attachment: part 7 - Use a custom Elf linker for libraries given with an absolute path name.

The previous version I sent didn't address comment 75.

Comment 104

[Mike Hommey [:glandium]]

Attachment: part 8 - Allow to load Elf files from a Zip archive.

Minor fixes

Comment 105

[Mike Hommey [:glandium]]

Comment on attachment 589441 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Carrying over r+

Comment 106

[Mike Hommey [:glandium]]

Attachment: part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

Adjusted to changes in part 8, and renamed AutoAny to AutoClean.

Comment 107

[Mike Hommey [:glandium]]

Comment on attachment 589442 [details] [diff] [review]
part 7 - Use a custom Elf linker for libraries given with an absolute path name.

Carrying over r+

Comment 108

[Mike Hommey [:glandium]]

Attachment: part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

Using strncmp

Comment 109

[Mike Hommey [:glandium]]

Comment on attachment 589444 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive.

Carrying over r+

Comment 110

[Mike Hommey [:glandium]]

Comment on attachment 589446 [details] [diff] [review]
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

Carrying over r+

Comment 111

[Michael Wu [:mwu]]

Comment on attachment 588894 [details] [diff] [review]
part 10 - Allow debug symbols to be found under gdb without extracted libraries

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

The InitDebugger part looks ok to me, assuming the comments about how it works is accurate.

::: mozglue/linker/ElfLoader.cpp
@@ +20,5 @@
> +#endif
> +
> +#ifndef PAGE_MASK
> +#define PAGE_MASK (~ (PAGE_SIZE - 1))
> +#endif

Hm, this should only be needed for building on normal Linux systems, right? Those would probably want sysconf but I guess it doesn't matter much.

@@ +504,5 @@
> +    if (phdr->p_type == PT_LOAD && phdr->p_offset == 0)
> +      base -= phdr->p_vaddr;
> +    if (phdr->p_type == PT_DYNAMIC) {
> +      dyns.Init(base + phdr->p_vaddr, phdr->p_filesz);
> +    }

Probably want to remove the curly braces here.

Comment 112

[Mike Hommey [:glandium]]

(In reply to Michael Wu [:mwu] from comment #111)
> Comment on attachment 588894 [details] [diff] [review]
> part 10 - Allow debug symbols to be found under gdb without extracted
> libraries
> 
> Review of attachment 588894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The InitDebugger part looks ok to me, assuming the comments about how it
> works is accurate.
> 
> ::: mozglue/linker/ElfLoader.cpp
> @@ +20,5 @@
> > +#endif
> > +
> > +#ifndef PAGE_MASK
> > +#define PAGE_MASK (~ (PAGE_SIZE - 1))
> > +#endif
> 
> Hm, this should only be needed for building on normal Linux systems, right?
> Those would probably want sysconf but I guess it doesn't matter much.

Using sysconf only matters when being completely cross-architecture, which the linker isn't, since it only supports x86, x64 and arm, all of which use 4k pages. If (when?) I port it to other architectures, I'll switch to use sysconf.

Comment 113

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3b66081924
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5a5abec7ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a8b1b25477
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd752f4935d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/229140e62d7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/41c7ad654949
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e66973c882
https://hg.mozilla.org/integration/mozilla-inbound/rev/7469527224bf

Only the latter needs to be backed out if things go wrong. There was a conflict in part 11 after bug 713228 landed, but it was pretty straightforward.

Comment 114

[Mike Hommey [:glandium]]

And a fixup (oops)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cbf4661e21

Comment 115

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dd3b66081924
https://hg.mozilla.org/mozilla-central/rev/fe5a5abec7ed
https://hg.mozilla.org/mozilla-central/rev/54a8b1b25477
https://hg.mozilla.org/mozilla-central/rev/bd752f4935d9
https://hg.mozilla.org/mozilla-central/rev/229140e62d7b
https://hg.mozilla.org/mozilla-central/rev/41c7ad654949
https://hg.mozilla.org/mozilla-central/rev/27e66973c882
https://hg.mozilla.org/mozilla-central/rev/7469527224bf
https://hg.mozilla.org/mozilla-central/rev/f6cbf4661e21

Comment 116

[Mike Hommey [:glandium]]

And yet another fixup (double oops)
https://hg.mozilla.org/integration/mozilla-central/rev/8297a76e0552

This wasn't caught because there is no test for the places migration :(

Comment 117

[Mike Hommey [:glandium]]

Disabled the new linker until I figure out what is wrong on monday, since the previous fixup was apparently not enough.
https://hg.mozilla.org/mozilla-central/rev/244711942710

Comment 118

[Mike Hommey [:glandium]]

Fixed for good, and re-enabled:
https://hg.mozilla.org/mozilla-central/rev/fd8fc492279c
https://hg.mozilla.org/mozilla-central/rev/758005504cab

Comment 119

[Mike Hommey [:glandium]]

Attachment: part 1 - Simple Zip reader for the new ELF Linker, with fixups - for aurora

[Approval Request Comment]
This is the new Elf linker for Android (which also works for Linux, but is not meant to be used in releases there, at least for now). It paves the way for bug 686805, which should give us some nice startup improvements.
There are essentially two parts to this bug, with different impacts:
- part 1 to 4 is a Zip reader class that is being used by the new Elf linker, but also replaces the Zip reading stuff that was in APKOpen.cpp. This landed 2 weeks ago and saw no problem besides build errors what were fixed after landing. It had the side effect of dropping RSS.
- part 5 to 11 is the Elf linker itself, and only adds code. It doesn't change anything in the old linker. Part 12 is the part that enables it, and it can be disabled by backing out this part at any time.

I'm going to attach a few refreshed versions (with the landed fixups, or with context changes for aurora) of a few parts.

Comment 120

[Mike Hommey [:glandium]]

Attachment: part 4 - Use the new ELF linker Zip reader in the old linker - for aurora

Comment 121

[Mike Hommey [:glandium]]

Attachment: part 11 - Hook the new linker in Android initialization - with fixups, for aurora

Comment 122

[Mike Hommey [:glandium]]

Attachment: part 12 - Enable the new linker on Android native UI - for aurora

Comment 123

[Mike Hommey [:glandium]]

Here is a list of all direct and indirect dependencies of this bug that need to land beforehand (and thus require aurora approval if this one has aurora approval):
- bug 701371
- bug 709776
- bug 708570
- bug 717906
- bug 712284
- bug 714029
- bug 712579
- bug 716825
- bug 719253

The refreshed part 11 depends on bug 713228, because of conflicts between the two patches (and because bug 713228 landed first on m-c). The original version of part 11 + a fixup may be used on aurora if bug 713228 is not taken.

Comment 124

[Mike Hommey [:glandium]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3928abd6fd6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/1fcb113281fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/e41bac527398
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f500877bdfa
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a792715f034
https://hg.mozilla.org/releases/mozilla-aurora/rev/a614838758b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d2c29ac8c03
https://hg.mozilla.org/releases/mozilla-aurora/rev/88c009db8205
https://hg.mozilla.org/releases/mozilla-aurora/rev/c390403ee7cb
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d162c7fb464
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ca3fe73e1ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/5458543c789d

Comment 125

[Ed Morley [:emorley]]

Comment on attachment 588896 [details] [diff] [review]
part 11 - Hook the new linker in Android initialization

>Bug 683127 part 11 - Hook the new linker in Android initialization
>diff --git a/configure.in b/configure.in
>@@ -9025,17 +9025,27 @@ if test -z "$MOZ_NATIVE_NSPR"; then
>+    if test -n "$MOZ_LINKER" -a -z "$MOZ_OLD_LINKER" -a $ac_cv_func_dladdr = no ; then

This line causes |c:/mozilla/inbound/configure: line 25335: test: too many arguments| in my local win7 build, MozillaBuild 1.6. (Albeit carries on past the warning)