Last Comment Bug 683127 - New custom ELF linker
: New custom ELF linker
Status: RESOLVED FIXED
[inbound][qa-]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla12
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 706116 708567 708570 709776 712281 712579 714029 716825 720621 720737 721099 725517 725736 729883
Blocks: 686282 686805
  Show dependency treegraph
 
Reported: 2011-08-30 04:12 PDT by Mike Hommey [:glandium]
Modified: 2012-02-23 10:23 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
[WIP] Proof of concept (238.16 KB, patch)
2011-11-30 07:18 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker (21.35 KB, patch)
2011-12-12 07:58 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Build system glue for the new linker (3.71 KB, patch)
2011-12-12 08:00 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker (21.35 KB, patch)
2011-12-12 08:38 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Build system glue for the new linker (4.92 KB, patch)
2011-12-12 08:39 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Build system glue for the new linker (4.42 KB, patch)
2011-12-12 08:40 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Build system glue for the new linker (4.42 KB, patch)
2011-12-12 08:41 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Build system glue for the new linker (6.26 KB, patch)
2011-12-12 08:55 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker (21.77 KB, patch)
2011-12-12 09:52 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker (23.08 KB, patch)
2011-12-14 09:03 PST, Mike Hommey [:glandium]
mwu.code: review+
Details | Diff | Review
part 3 - Test for the Zip reader (7.67 KB, patch)
2011-12-14 09:04 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 4 - Use the new ELF linker Zip reader in the old linker (16.11 KB, patch)
2011-12-14 09:10 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 3 - Test for the Zip reader (7.82 KB, patch)
2011-12-15 00:27 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker. (23.13 KB, patch)
2011-12-20 06:33 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 2 - Build system glue for the new linker. (6.46 KB, patch)
2011-12-20 06:34 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 1.5 - Add bookkeeping for Zips (6.20 KB, patch)
2011-12-28 00:23 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 2 - Build system glue for the new linker. (8.83 KB, patch)
2011-12-29 06:20 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose (17.50 KB, patch)
2011-12-29 07:41 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 6 - Use wrapped dl* functions in the XPCOM standalone glue when building with MOZ_LINKER on Linux (3.06 KB, patch)
2011-12-29 07:50 PST, Mike Hommey [:glandium]
benjamin: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (17.23 KB, patch)
2011-12-29 08:47 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name (44.71 KB, patch)
2011-12-29 13:43 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (17.57 KB, patch)
2011-12-30 10:53 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name (45.96 KB, patch)
2011-12-30 10:54 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 8 - Allow to load Elf files from a Zip archive (28.43 KB, patch)
2011-12-30 10:55 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 9 - Enable the new linker on Android Native UI (11.51 KB, patch)
2011-12-30 11:06 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker. (17.84 KB, patch)
2012-01-09 00:55 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 2 - Build system glue for the new linker. (8.83 KB, patch)
2012-01-09 00:59 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 3 - Test for the Zip reader. (7.86 KB, patch)
2012-01-09 01:02 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 3 - Test for the Zip reader. (4.88 KB, patch)
2012-01-09 01:05 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 4 - Use the new ELF linker Zip reader in the old linker. (16.12 KB, patch)
2012-01-09 01:07 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (14.31 KB, patch)
2012-01-09 01:38 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker. (17.83 KB, patch)
2012-01-09 01:44 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (14.32 KB, patch)
2012-01-09 01:48 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name. (43.05 KB, patch)
2012-01-09 02:01 PST, Mike Hommey [:glandium]
jseward: review+
Details | Diff | Review
part 8 - Allow to load Elf files from a Zip archive. (25.60 KB, patch)
2012-01-09 02:08 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 2 - Build system glue for the new linker. (8.83 KB, patch)
2012-01-09 03:14 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 2 - Build system glue for the new linker. (8.89 KB, patch)
2012-01-10 10:38 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 4 - Use the new ELF linker Zip reader in the old linker. (16.12 KB, patch)
2012-01-10 10:39 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 9 - Enable the new linker on Android Native UI (11.54 KB, patch)
2012-01-10 10:40 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name. (43.36 KB, patch)
2012-01-11 06:53 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name. (43.36 KB, patch)
2012-01-11 06:58 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 8 - Allow to load Elf files from a Zip archive. (24.58 KB, patch)
2012-01-11 07:10 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 8 - Allow to load Elf files from a Zip archive. (25.74 KB, patch)
2012-01-11 07:44 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind (15.65 KB, patch)
2012-01-16 08:34 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 10 - Allow debug symbols to be found under gdb without extracted libraries (17.71 KB, patch)
2012-01-16 08:39 PST, Mike Hommey [:glandium]
taras.mozilla: review+
mwu.code: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 11 - Hook the new linker in Android initialization (11.27 KB, patch)
2012-01-16 08:42 PST, Mike Hommey [:glandium]
blassey.bugs: review+
Details | Diff | Review
part 12 - Enable the new linker on Android native UI (878 bytes, patch)
2012-01-16 08:44 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (14.32 KB, patch)
2012-01-18 02:26 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose. (14.32 KB, patch)
2012-01-18 02:27 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 7 - Use a custom Elf linker for libraries given with an absolute path name. (43.43 KB, patch)
2012-01-18 02:29 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 8 - Allow to load Elf files from a Zip archive. (25.73 KB, patch)
2012-01-18 02:30 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind (15.70 KB, patch)
2012-01-18 02:32 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind (15.70 KB, patch)
2012-01-18 02:35 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 1 - Simple Zip reader for the new ELF Linker, with fixups - for aurora (17.96 KB, patch)
2012-01-25 09:22 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 4 - Use the new ELF linker Zip reader in the old linker - for aurora (16.36 KB, patch)
2012-01-25 09:27 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 11 - Hook the new linker in Android initialization - with fixups, for aurora (12.94 KB, patch)
2012-01-25 09:34 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
part 12 - Enable the new linker on Android native UI - for aurora (887 bytes, patch)
2012-01-25 09:35 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-08-30 04:12:29 PDT
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] 2011-11-30 07:18:56 PST
Created attachment 577949 [details] [diff] [review]
[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] 2011-11-30 07:22:44 PST
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) 2011-11-30 09:55:29 PST
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) 2011-11-30 09:57:37 PST
This also needs more comments.
Comment 5 Mike Hommey [:glandium] 2011-11-30 23:12:40 PST
(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] 2011-12-12 07:58:14 PST
Created attachment 580910 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker
Comment 7 Mike Hommey [:glandium] 2011-12-12 08:00:30 PST
Created attachment 580911 [details] [diff] [review]
part 2 - Build system glue for the new linker
Comment 8 Mike Hommey [:glandium] 2011-12-12 08:38:11 PST
Created attachment 580919 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

Fixed an erroneous magic number
Comment 9 Mike Hommey [:glandium] 2011-12-12 08:39:48 PST
Created attachment 580920 [details] [diff] [review]
part 2 - Build system glue for the new linker
Comment 10 Mike Hommey [:glandium] 2011-12-12 08:40:56 PST
Created attachment 580921 [details] [diff] [review]
part 2 - Build system glue for the new linker

Oops
Comment 11 Mike Hommey [:glandium] 2011-12-12 08:41:51 PST
Created attachment 580924 [details] [diff] [review]
part 2 - Build system glue for the new linker

End-of-day re-oops
Comment 12 Mike Hommey [:glandium] 2011-12-12 08:44:01 PST
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] 2011-12-12 08:55:15 PST
Created attachment 580929 [details] [diff] [review]
part 2 - Build system glue for the new linker

This should be the one.
Comment 14 Mike Hommey [:glandium] 2011-12-12 09:52:09 PST
Created attachment 580953 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

A few modifications I had forgotten
Comment 15 Mike Hommey [:glandium] 2011-12-13 05:43:07 PST
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] 2011-12-14 09:03:14 PST
Created attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker
Comment 17 Mike Hommey [:glandium] 2011-12-14 09:04:35 PST
Created attachment 581660 [details] [diff] [review]
part 3 - Test for the Zip reader
Comment 18 Mike Hommey [:glandium] 2011-12-14 09:10:56 PST
Created attachment 581662 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker
Comment 19 Mike Hommey [:glandium] 2011-12-15 00:27:39 PST
Created attachment 581904 [details] [diff] [review]
part 3 - Test for the Zip reader
Comment 20 (dormant account) 2011-12-16 11:50:03 PST
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) 2011-12-16 12:01:43 PST
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] 2011-12-16 12:31:32 PST
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] 2011-12-16 12:32:43 PST
Comment on attachment 581659 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker

bugzilla fail
Comment 24 Mike Hommey [:glandium] 2011-12-20 06:33:34 PST
Created attachment 583143 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker.

Addressed mwu and taras' comments
Comment 25 Mike Hommey [:glandium] 2011-12-20 06:34:21 PST
Created attachment 583144 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Refreshed with a few minor changes
Comment 26 Mike Hommey [:glandium] 2011-12-20 06:34:36 PST
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] 2011-12-20 06:35:01 PST
Comment on attachment 583144 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+
Comment 28 Mike Hommey [:glandium] 2011-12-28 00:23:28 PST
Created attachment 584533 [details] [diff] [review]
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] 2011-12-29 06:20:02 PST
Created attachment 584737 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Some changes in configure.in
Comment 30 Mike Hommey [:glandium] 2011-12-29 07:41:28 PST
Created attachment 584746 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose
Comment 31 Mike Hommey [:glandium] 2011-12-29 07:50:19 PST
Created attachment 584750 [details] [diff] [review]
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] 2011-12-29 08:47:24 PST
Created attachment 584766 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.
Comment 33 Mike Hommey [:glandium] 2011-12-29 13:43:16 PST
Created attachment 584836 [details] [diff] [review]
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] 2011-12-30 10:53:04 PST
Created attachment 584995 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.
Comment 35 Mike Hommey [:glandium] 2011-12-30 10:54:18 PST
Created attachment 584996 [details] [diff] [review]
part 7 - Use a custom Elf linker for libraries given with an absolute path name
Comment 36 Mike Hommey [:glandium] 2011-12-30 10:55:57 PST
Created attachment 584997 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive
Comment 37 Mike Hommey [:glandium] 2011-12-30 11:06:25 PST
Created attachment 585000 [details] [diff] [review]
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] 2011-12-30 11:09:54 PST
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 [:khuey] (khuey@mozilla.com) 2011-12-30 11:14:33 PST
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] 2011-12-30 11:23:06 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) 2012-01-03 17:56:17 PST
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] 2012-01-03 23:28:53 PST
(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] 2012-01-03 23:32:19 PST
(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] 2012-01-05 08:07:49 PST
(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] 2012-01-05 08:09:18 PST
(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] 2012-01-05 08:11:20 PST
(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] 2012-01-05 08:13:56 PST
(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] 2012-01-05 23:32:58 PST
(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] 2012-01-06 02:39:46 PST
(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] 2012-01-06 02:45:15 PST
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] 2012-01-06 03:07:49 PST
(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) 2012-01-06 16:54:37 PST
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) 2012-01-06 17:09:53 PST
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) 2012-01-06 17:18:40 PST
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] 2012-01-09 00:55:37 PST
Created attachment 586914 [details] [diff] [review]
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] 2012-01-09 00:56:36 PST
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] 2012-01-09 00:59:15 PST
Created attachment 586916 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Refreshed against bug 701371.
Comment 58 Mike Hommey [:glandium] 2012-01-09 01:00:07 PST
Comment on attachment 586916 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+
Comment 59 Mike Hommey [:glandium] 2012-01-09 01:02:37 PST
Created attachment 586918 [details] [diff] [review]
part 3 - Test for the Zip reader.

Refreshed against bug 701371.
Comment 60 Mike Hommey [:glandium] 2012-01-09 01:03:11 PST
Comment on attachment 586918 [details] [diff] [review]
part 3 - Test for the Zip reader.

Carrying over r+
Comment 61 Mike Hommey [:glandium] 2012-01-09 01:05:43 PST
Created attachment 586919 [details] [diff] [review]
part 3 - Test for the Zip reader.

Forgot to update the license boilerplates.
Comment 62 Mike Hommey [:glandium] 2012-01-09 01:06:48 PST
Comment on attachment 586919 [details] [diff] [review]
part 3 - Test for the Zip reader.

Carrying over r+
Comment 63 Mike Hommey [:glandium] 2012-01-09 01:07:39 PST
Created attachment 586921 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker.

Refreshed against bug 701371.
Comment 64 Mike Hommey [:glandium] 2012-01-09 01:08:11 PST
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] 2012-01-09 01:38:41 PST
Created attachment 586928 [details] [diff] [review]
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] 2012-01-09 01:44:20 PST
Created attachment 586930 [details] [diff] [review]
part 1 - Simple Zip reader for the new ELF Linker.

Better std::find logic
Comment 67 Mike Hommey [:glandium] 2012-01-09 01:45:10 PST
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] 2012-01-09 01:48:47 PST
Created attachment 586931 [details] [diff] [review]
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] 2012-01-09 01:50:22 PST
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] 2012-01-09 02:01:41 PST
Created attachment 586934 [details] [diff] [review]
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] 2012-01-09 02:08:29 PST
Created attachment 586936 [details] [diff] [review]
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] 2012-01-09 02:13:57 PST
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] 2012-01-09 03:14:50 PST
Created attachment 586942 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Re-refreshed against bug 701371.
Comment 74 Mike Hommey [:glandium] 2012-01-09 03:16:44 PST
Comment on attachment 586942 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+
Comment 75 Julian Seward [:jseward] 2012-01-09 08:18:11 PST
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] 2012-01-10 00:37:41 PST
(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] 2012-01-10 10:38:07 PST
Created attachment 587378 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Refreshed after bug 709776
Comment 78 Mike Hommey [:glandium] 2012-01-10 10:39:25 PST
Created attachment 587379 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker.

Refreshed after bug 709776
Comment 79 Mike Hommey [:glandium] 2012-01-10 10:40:57 PST
Created attachment 587380 [details] [diff] [review]
part 9 - Enable the new linker on Android Native UI

Refreshed against bug 709776
Comment 80 Mike Hommey [:glandium] 2012-01-11 02:15:50 PST
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] 2012-01-11 02:18:49 PST
Comment on attachment 587378 [details] [diff] [review]
part 2 - Build system glue for the new linker.

Carrying over r+
Comment 82 Mike Hommey [:glandium] 2012-01-11 02:19:13 PST
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] 2012-01-11 06:53:52 PST
Created attachment 587685 [details] [diff] [review]
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] 2012-01-11 06:58:57 PST
Created attachment 587687 [details] [diff] [review]
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] 2012-01-11 07:10:04 PST
Created attachment 587688 [details] [diff] [review]
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] 2012-01-11 07:36:36 PST
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] 2012-01-11 07:37:30 PST
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] 2012-01-11 07:44:11 PST
Created attachment 587694 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive.

Grah, there was a missing hunk
Comment 89 Mike Hommey [:glandium] 2012-01-11 10:48:02 PST
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 91 Mike Hommey [:glandium] 2012-01-16 08:34:45 PST
Created attachment 588892 [details] [diff] [review]
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] 2012-01-16 08:39:37 PST
Created attachment 588894 [details] [diff] [review]
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] 2012-01-16 08:42:05 PST
Created attachment 588896 [details] [diff] [review]
part 11 - Hook the new linker in Android initialization

This hooks the new linker in APKOpen.cpp.
Comment 94 Mike Hommey [:glandium] 2012-01-16 08:44:33 PST
Created attachment 588898 [details] [diff] [review]
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?) 2012-01-16 21:18:46 PST
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] 2012-01-16 22:42:48 PST
(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) 2012-01-17 13:54:24 PST
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) 2012-01-17 14:07:15 PST
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] 2012-01-17 23:39:28 PST
(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] 2012-01-18 02:26:15 PST
Created attachment 589438 [details] [diff] [review]
part 5 - Initial Elf Loader, wrapping around dlopen/dladdr/dlsym/dlclose.

Minor fix.
Comment 101 Mike Hommey [:glandium] 2012-01-18 02:27:52 PST
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.
Comment 102 Mike Hommey [:glandium] 2012-01-18 02:29:07 PST
(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] 2012-01-18 02:29:24 PST
Created attachment 589442 [details] [diff] [review]
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] 2012-01-18 02:30:32 PST
Created attachment 589444 [details] [diff] [review]
part 8 - Allow to load Elf files from a Zip archive.

Minor fixes
Comment 105 Mike Hommey [:glandium] 2012-01-18 02:30:56 PST
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] 2012-01-18 02:32:24 PST
Created attachment 589445 [details] [diff] [review]
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] 2012-01-18 02:32:39 PST
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] 2012-01-18 02:35:25 PST
Created attachment 589446 [details] [diff] [review]
part 9 - Allow to temporarily extract Elf files from a Zip archive for e.g. valgrind

Using strncmp
Comment 109 Mike Hommey [:glandium] 2012-01-18 02:37:14 PST
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] 2012-01-18 02:37:44 PST
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] 2012-01-19 10:14:16 PST
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] 2012-01-19 10:22:32 PST
(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 114 Mike Hommey [:glandium] 2012-01-20 00:59:44 PST
And a fixup (oops)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cbf4661e21
Comment 116 Mike Hommey [:glandium] 2012-01-21 11:28:11 PST
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] 2012-01-21 12:16:10 PST
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] 2012-01-22 23:44:11 PST
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] 2012-01-25 09:22:28 PST
Created attachment 591500 [details] [diff] [review]
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] 2012-01-25 09:27:32 PST
Created attachment 591502 [details] [diff] [review]
part 4 - Use the new ELF linker Zip reader in the old linker - for aurora
Comment 121 Mike Hommey [:glandium] 2012-01-25 09:34:27 PST
Created attachment 591504 [details] [diff] [review]
part 11 - Hook the new linker in Android initialization - with fixups, for aurora
Comment 122 Mike Hommey [:glandium] 2012-01-25 09:35:39 PST
Created attachment 591505 [details] [diff] [review]
part 12 - Enable the new linker on Android native UI - for aurora
Comment 123 Mike Hommey [:glandium] 2012-01-25 10:05:22 PST
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 125 Ed Morley [:emorley] 2012-02-08 15:51:08 PST
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)

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