Last Comment Bug 646815 - VP8 source generates non PIC code on ARM
: VP8 source generates non PIC code on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla7
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: 622908
  Show dependency treegraph
 
Reported: 2011-03-31 06:19 PDT by Mike Hommey [:glandium]
Modified: 2011-06-16 19:12 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoC (4.44 KB, patch)
2011-06-04 01:53 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
PoC (23.51 KB, patch)
2011-06-04 02:49 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Avoid text relocations in libvpx on ARM (27.25 KB, patch)
2011-06-04 03:03 PDT, Mike Hommey [:glandium]
tterribe: review-
Details | Diff | Splinter Review
Avoid text relocations in libvpx on ARM (55.55 KB, patch)
2011-06-05 20:09 PDT, Mike Hommey [:glandium]
tterribe: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-03-31 06:19:10 PDT
This is the output from eu-findtextrel on libxul.so after getting rid of those related to libgcc and libstdc++:

either the file containing the function 'skip_secondpass_hloop' or the file containing the function 'filter8_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'skip_secondpass_filter' or the file containing the function 'bifilter4_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'skip_secondpass_filter' or the file containing the function 'bifilter8x4_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'skip_secondpass_filter' or the file containing the function 'bifilter8_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'filt_blk2d_spo16x16_loop_neon' or the file containing the function 'bifilter16_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_loop_filter_neon' or the file containing the function 'lf_coeff' is not compiled with -fpic/-fPIC
either the file containing the function '_vp8_loop_filter_simple_horizontal_edge_neon' or the file containing the function 'lfhy_coeff' is not compiled with -fpic/-fPIC
either the file containing the function '_vp8_loop_filter_simple_vertical_edge_neon' or the file containing the function 'vlfy_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_mbloop_filter_neon' or the file containing the function 'mblf_coeff' is not compiled with -fpic/-fPIC
either the file containing the function '_vp8_short_idct4x4llm_neon' or the file containing the function 'idct_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'secondpass_filter4x4_only' or the file containing the function 'filter4_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'secondpass_filter8x4_only' or the file containing the function 'filter8_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'filt_blk2d_spo8x8_loop_neon' or the file containing the function 'filter8_coeff' is not compiled with -fpic/-fPIC
either the file containing the function 'secondpass_only_inner_loop_neon' or the file containing the function 'filter16_coeff' is not compiled with -fpic/-fPIC
either the file containing the function '_idct_dequant_dc_full_2x_neon' or the file containing the function 'cospi8sqrt2minus1' is not compiled with -fpic/-fPIC
either the file containing the function '_vp8_dequant_idct_add_neon' or the file containing the function 'cospi8sqrt2minus1' is not compiled with -fpic/-fPIC
either the file containing the function '_idct_dequant_full_2x_neon' or the file containing the function 'cospi8sqrt2minus1' is not compiled with -fpic/-fPIC
Comment 1 Mike Hommey [:glandium] 2011-06-03 20:56:42 PDT
More reliable information with a build with debugging symbols:
either the file containing the function 'vp8_sixtap_predict8x4_armv6' or the file containing the function 'vp8_bilinear_predict4x4_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_bilinear_predict4x4_neon' or the file containing the function 'vp8_bilinear_predict8x4_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_bilinear_predict8x4_neon' or the file containing the function 'vp8_bilinear_predict8x8_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_bilinear_predict8x8_neon' or the file containing the function 'vp8_bilinear_predict16x16_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_bilinear_predict16x16_neon' or the file containing the function 'vp8_copy_mem8x4_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_loop_filter_vertical_edge_uv_neon' or the file containing the function 'vp8_loop_filter_simple_horizontal_edge_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_loop_filter_simple_horizontal_edge_neon' or the file containing the function 'vp8_loop_filter_simple_vertical_edge_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_loop_filter_simple_vertical_edge_neon' or the file containing the function 'vp8_mbloop_filter_horizontal_edge_y_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_mbloop_filter_vertical_edge_uv_neon' or the file containing the function 'vp8_recon2b_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_short_idct4x4llm_neon' or the file containing the function 'vp8_sixtap_predict_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_sixtap_predict_neon' or the file containing the function 'vp8_sixtap_predict8x4_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_sixtap_predict8x4_neon' or the file containing the function 'vp8_sixtap_predict8x8_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_sixtap_predict8x8_neon' or the file containing the function 'vp8_sixtap_predict16x16_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_sixtap_predict16x16_neon' or the file containing the function 'vp8_recon16x16mb_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'idct_dequant_dc_full_2x_neon' or the file containing the function 'idct_dequant_dc_0_2x_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'vp8_dequant_idct_add_neon' or the file containing the function 'idct_dequant_full_2x_neon' is not compiled with -fpic/-fPIC
either the file containing the function 'idct_dequant_full_2x_neon' or the file containing the function 'idct_dequant_0_2x_neon' is not compiled with -fpic/-fPIC
Comment 2 Mike Hommey [:glandium] 2011-06-04 01:19:51 PDT
The list in comment 0 is actually more accurate (eu-findtextrel sucks).

The problem comes with all _*_coeff_ variables, which contain a pointer to a corresponding *_coeff table.

There are two possible way to fix this, one being kind of ugly, and the other being kind of painful. Both also have their kind of ugliness.

It turns out the ads2gas.pl assembly converter does the wrong thing for areas, and converts everything as .text, even things that are meant to be .data or .rodata. If we fix this, we can fix the PIC problem by implementing PC-relative load using the global offset table, which is kind of annoying to do in assembler. If we don't fix the sections, we can fix the PIC problem by implementing PC-relative load using an offset between the position of the table and the position of the instruction loading its address. It however looks like upstream libvpx has removed the different areas for RVCT support (which was the wrong reason to remove them, but whatever), so we could "safely" go the easier way.

In both cases, we need to adjust the PC-relative load with the oh convenient pc register value being different for ARM (pc = current instruction address + 8) and THUMB (pc = current instruction address + 4). Which means we need to find a way for the same source to properly handle being compiled for ARM or THUMB, which means we need to preprocess the assembly files (meaning changing the temporary files suffix)
Comment 3 Mike Hommey [:glandium] 2011-06-04 01:53:55 PDT
Created attachment 537330 [details] [diff] [review]
PoC

This is what this could look like, except it is most probably completely incompatible with the ads syntax, and probably breaks ARM build with other assemblers than GNU as.
I'm tempted to say ads2gas.pl needs some fixing, it actually ends up exporting symbols it shouldn't export, and that would allow to use a syntax that should work in both format. That would still leave the PC_OFFSET problem...
Also note the Makefile.in part is wrong because of bug 662024.
Comment 4 Mike Hommey [:glandium] 2011-06-04 02:08:15 PDT
Instead of using labels, the following might work in ads format:
add r12, pc, #filter8_coeff - . - PC_OFFSET

(that still leaves the PC_OFFSET problem)
Comment 5 Mike Hommey [:glandium] 2011-06-04 02:11:57 PDT
Jacob might have better insight wrt ads. (point being that it would be preferable if our change could be upstreamed)
Comment 6 Timothy B. Terriberry (:derf) 2011-06-04 02:13:14 PDT
Comment on attachment 537330 [details] [diff] [review]
PoC

>-        VPX_ASM_SUFFIX="$ASM_SUFFIX"
>+        VPX_ASM_SUFFIX="S"

Why do you need this change?

>-    ldr         r12, _filter8_coeff_
>+.LC0:
>+    add         r12, pc, filter8_coeff - .LC0 - PC_OFFSET

Can't we just use
      adr         r12, filter8_coeff
?
Comment 7 Mike Hommey [:glandium] 2011-06-04 02:20:40 PDT
Forget it, we don't have a PC_OFFSET problem, the assembly is completely not thumb and can't be compiled as such.
Comment 8 Mike Hommey [:glandium] 2011-06-04 02:24:01 PDT
(In reply to comment #6)
> Comment on attachment 537330 [details] [diff] [review] [review]
> PoC
> 
> >-        VPX_ASM_SUFFIX="$ASM_SUFFIX"
> >+        VPX_ASM_SUFFIX="S"
> 
> Why do you need this change?

For no reason, now.
> 
> >-    ldr         r12, _filter8_coeff_
> >+.LC0:
> >+    add         r12, pc, filter8_coeff - .LC0 - PC_OFFSET
> 
> Can't we just use
>       adr         r12, filter8_coeff
> ?

Yes, you're completely right. I'm not vey familiar with the whole spectrum of the arm assembly language, and didn't check pseudo expressions.
Comment 9 Mike Hommey [:glandium] 2011-06-04 02:49:23 PDT
Created attachment 537332 [details] [diff] [review]
PoC

This one uses adr (and adrl for files where the coeff tables are too far away, we could arrange this by moving the tables) on all the *_coeff tables. This removes all but three vpx text relocations.
Comment 10 Mike Hommey [:glandium] 2011-06-04 03:03:31 PDT
Created attachment 537333 [details] [diff] [review]
Avoid text relocations in libvpx on ARM

This version removes all text relocations related to libvpx. It does build and looks to generate proper machine code, but I can't actually test at the moment.
Comment 11 Timothy B. Terriberry (:derf) 2011-06-05 18:12:23 PDT
Comment on attachment 537333 [details] [diff] [review]
Avoid text relocations in libvpx on ARM

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

Something isn't right, because this crashes the content process on Android (I don't have debugging set up on Android, so I can't tell you where).

Also, any changes to libvpx need to include a patch file in media/libvpx and a line in update.sh to apply it, so it can be updated with the procedure in https://wiki.mozilla.org/WebM/Updating_libvpx
Comment 12 Mike Hommey [:glandium] 2011-06-05 20:09:24 PDT
Created attachment 537495 [details] [diff] [review]
Avoid text relocations in libvpx on ARM
Comment 13 Timothy B. Terriberry (:derf) 2011-06-05 20:22:11 PDT
Comment on attachment 537495 [details] [diff] [review]
Avoid text relocations in libvpx on ARM

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

Okay, looks good to me. Please also file a bug upstream and try to get these changes landed there: http://www.webmproject.org/code/contribute/submitting-patches/

::: media/libvpx/update.sh
@@ +321,5 @@
>  # Patch to fix link with xcode4
>  patch -p1 < xcode4.patch
>  
>  # Patch to fix data race on global function pointers
> +patch -p3 < bug640935.patch

Whoops. Good catch.
Comment 14 Mike Hommey [:glandium] 2011-06-14 20:20:29 PDT
http://hg.mozilla.org/mozilla-central/rev/9c5f91cbdadd
Comment 15 Mike Hommey [:glandium] 2011-06-16 19:12:38 PDT
http://review.webmproject.org/2519

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