Closed
Bug 1078837
Opened 10 years ago
Closed 10 years ago
[refactor] Replace IsSystemElf/reinterpret_cast dance with better API
Categories
(Core :: mozglue, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 1 obsolete file)
2.80 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
10.45 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Blocks: 1081034
Mentor: mh+mozilla
Whiteboard: [good-first-bug][lang=C++]
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8504677 -
Flags: review?(nfroyd)
Comment 2•10 years ago
|
||
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.)
Attachment #8504677 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8505268 -
Flags: review?(nfroyd)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8505270 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8504677 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8505268 -
Flags: review?(nfroyd) → review+
Comment 5•10 years ago
|
||
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.
Attachment #8505270 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b483b12025b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd07aa0b6a04
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b483b12025b0 https://hg.mozilla.org/mozilla-central/rev/fd07aa0b6a04
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•