Closed Bug 930627 Opened 6 years ago Closed 6 years ago

Implement __gnu_Unwind_Find_exidx in custom linker

Categories

(Core :: mozglue, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(1 file, 2 obsolete files)

__gnu_Unwind_Find_exidx is a routine to find the address and size of the ARM.exidx section corresponding to a module at a specified address,

> _Unwind_Ptr __gnu_Unwind_Find_exidx (_Unwind_Ptr pc, int * pcount)

It's implemented by the system linker, but not by our custom linker. Although it's not used by any of our code, it may be used by external libraries that use C++ exceptions internally.
This makes external libraries happy when they use internal C++ exceptions (i.e. when they catch all exceptions thrown within the library). Uncaught exceptions still cause an abort.
Attachment #821833 - Flags: review?(mh+mozilla)
Comment on attachment 821833 [details] [diff] [review]
Implement __gnu_Unwind_Find_exidx in custom linker on ARM (v1)

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

I think this should be implemented differently:
Add a pure virtual method FindExidx to LibHandle, with implementations in SystemElf and CustomElf.
The SystemElf one would use the system __gnu_Unwind_Find_exidx.
The CustomElf would use the data from PT_ARM_EXIDX that you'd get and store during CustomElf::Load
Then __wrap__gnu_Unwind_Find_exidx can look something like:

RefPtr<LibHandle> handle = ElfLoader::Singleton.GetHandleByPtr(addr);
if (!handle)
  return __gnu_Unwind_Find_exidx(...);
return handle->FindExidx(...);

Note ElfLoader::Singleton.GetHandleByPtr currently doesn't return a handler for SystemElfs, so the FindExidx implementation for SystemElf is more or less useless, but the !handle case would cover that, and I'd rather have the implementation exist for when ElfLoader::Singleton.GetHandleByPtr does return SystemElfs.
Attachment #821833 - Flags: review?(mh+mozilla) → review-
Okay, here's what I have now.
Attachment #821833 - Attachment is obsolete: true
Attachment #822400 - Flags: review?(mh+mozilla)
Comment on attachment 822400 [details] [diff] [review]
Implement __gnu_Unwind_Find_exidx in custom linker on ARM (v2)

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

::: mozglue/linker/ElfLoader.cpp
@@ +308,5 @@
> +void *
> +SystemElf::FindExidx(int *pcount) const
> +{
> +  if (__gnu_Unwind_Find_exidx)
> +    return __gnu_Unwind_Find_exidx(dlhandle, pcount);

Hmm just realized that's not right, I can't use dlhandle here. Might have to pass in the address to FindExidx.
(In reply to Jim Chen [:jchen :nchen] from comment #4)
> 
> Hmm just realized that's not right, I can't use dlhandle here. Might have to
> pass in the address to FindExidx.

Or always return NULL from SystemElf::FindExidx, and always call __gnu_Unwind_Find_exidx if FindExidx returns NULL.
Comment on attachment 822400 [details] [diff] [review]
Implement __gnu_Unwind_Find_exidx in custom linker on ARM (v2)

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

::: mozglue/linker/CustomElf.cpp
@@ +158,5 @@
> +#ifdef __ARM_EABI__
> +      case PT_ARM_EXIDX:
> +        elf->arm_exidx_ptr = phdr->p_vaddr;
> +        // Each ARM.exidx entry consists of two 32-bit values
> +        elf->arm_exidx_count = phdr->p_memsz / (2 * sizeof(uint32_t));

Insteadm you could use a
 Array<uint32_t[2]> arm_exidx
member, and fill it with:
 elf->arm_exidx.InitSize(elf->GetPtr(phdr->p_vaddr), phdr->p_memsz)

::: mozglue/linker/ElfLoader.cpp
@@ +308,5 @@
> +void *
> +SystemElf::FindExidx(int *pcount) const
> +{
> +  if (__gnu_Unwind_Find_exidx)
> +    return __gnu_Unwind_Find_exidx(dlhandle, pcount);

At this point, just ignore it and return NULL. Just add a TODO comment. It won't be called anyways, as mentioned in an earlier comment.
Attachment #822400 - Flags: review?(mh+mozilla) → feedback+
(In reply to Mike Hommey [:glandium] from comment #6)
> @@ +158,5 @@
> > +#ifdef __ARM_EABI__
> > +      case PT_ARM_EXIDX:
> > +        elf->arm_exidx_ptr = phdr->p_vaddr;
> > +        // Each ARM.exidx entry consists of two 32-bit values
> > +        elf->arm_exidx_count = phdr->p_memsz / (2 * sizeof(uint32_t));
> 
> Insteadm you could use a
>  Array<uint32_t[2]> arm_exidx
> member, and fill it with:
>  elf->arm_exidx.InitSize(elf->GetPtr(phdr->p_vaddr), phdr->p_memsz)

Yeah using Array is nice. The base is not set when scanning p-headers though, so the array has to be set later in the function.

> ::: mozglue/linker/ElfLoader.cpp
> @@ +308,5 @@
> > +void *
> > +SystemElf::FindExidx(int *pcount) const
> > +{
> > +  if (__gnu_Unwind_Find_exidx)
> > +    return __gnu_Unwind_Find_exidx(dlhandle, pcount);
> 
> At this point, just ignore it and return NULL. Just add a TODO comment. It
> won't be called anyways, as mentioned in an earlier comment.

Done
Attachment #822400 - Attachment is obsolete: true
Attachment #824445 - Flags: review?(mh+mozilla)
Attachment #824445 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/88ac9143dd30
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.