1078837 - [refactor] Replace IsSystemElf/reinterpret_cast dance with better API

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

From bug 1077384 comment 8:

Also, more of a code cleanliness idea than anything: this IsSystemElf/reinterpret_cast stuff seems to happen fairly often, you could just consider:

  if (CustomElf* ce = (*it)->AsCustomElf()) {
    ...
  }

or similar.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Replace IsSystemElf/reinterpret_cast dance with better API

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8504677 [details] [diff] [review]
Replace IsSystemElf/reinterpret_cast dance with better API

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

Huh, I thought there were more of these.

::: mozglue/linker/ElfLoader.cpp
@@ +458,5 @@
> +ElfLoader::Register(CustomElf *handle)
> +{
> +  Register(static_cast<LibHandle *>(handle));
> +  if (dbg)
> +    dbg.Add(handle);

Braces.

@@ +483,5 @@
> +ElfLoader::Forget(CustomElf *handle)
> +{
> +  Forget(static_cast<LibHandle *>(handle));
> +  if (dbg)
> +    dbg.Remove(handle);

Braces.

@@ +1216,5 @@
>      mozilla::RefPtr<LibHandle> handle =
>        ElfLoader::Singleton.GetHandleByPtr(info->si_addr);
> +    DEBUG_LOG("Within the address space of %s", handle->GetPath());
> +    if (handle && handle->mappable && handle->mappable->ensure(info->si_addr))
> +      return;

Braces.

This seems a little strange insofar as we don't have the IsSystemElf check here anymore.  If we segfault inside of libc or similar, doesn't this just hit the default implementation of |ensure()|, which returns true always:

http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/Mappable.h#55

?  That seems undesirable.  (I mean, we're segfaulting trying to access the text segment, which is bad, but just returning here isn't helping the situation.)

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 1 - Move CustomElf::mappable to BaseElf

Comment 4

[Mike Hommey [:glandium]]

Attachment: Replace IsSystemElf/reinterpret_cast dance with better API

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8505270 [details] [diff] [review]
Replace IsSystemElf/reinterpret_cast dance with better API

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

I guess this is OK.

Comment 6

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b483b12025b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd07aa0b6a04

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b483b12025b0
https://hg.mozilla.org/mozilla-central/rev/fd07aa0b6a04