VP8 source generates non PIC code on ARM

RESOLVED FIXED in mozilla7

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Comment 5

6 years ago
Jacob might have better insight wrt ads. (point being that it would be preferable if our change could be upstreamed)
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
?
(Assignee)

Comment 7

6 years ago
Forget it, we don't have a PC_OFFSET problem, the assembly is completely not thumb and can't be compiled as such.
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #537330 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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.
Attachment #537333 - Flags: review?(tterribe)
(Assignee)

Updated

6 years ago
Attachment #537332 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 622908
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
Attachment #537333 - Flags: review?(tterribe) → review-
(Assignee)

Comment 12

6 years ago
Created attachment 537495 [details] [diff] [review]
Avoid text relocations in libvpx on ARM
Attachment #537495 - Flags: review?(tterribe)
(Assignee)

Updated

6 years ago
Attachment #537333 - Attachment is obsolete: true
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.
Attachment #537495 - Flags: review?(tterribe) → review+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/9c5f91cbdadd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 15

6 years ago
http://review.webmproject.org/2519
You need to log in before you can comment on or make changes to this bug.